viirya commented on code in PR #7123:
URL: https://github.com/apache/datafusion-comet/pull/7123#discussion_r4321748058
##########
docs/source/user-guide/latest/compatibility/index.md:
##########
@@ -97,9 +97,26 @@ underlying bytes rather than on the rendered text:
bytes) but `true` in Comet. Equality, joins, grouping, ordering, and
byte-based string functions
such as `contains` can therefore disagree with Spark when non-UTF-8 bytes
are involved.
-Both differences require string data that is not valid UTF-8, which does not
occur for text read from
-Parquet or produced by string expressions. Consistent handling of invalid
UTF-8 across all native
-string paths is tracked by
+Both differences require string data that is not valid UTF-8. Such data does
not occur for text read
+from Parquet, which Spark requires to be valid UTF-8, or produced by most
string expressions. It
Review Comment:
"which Spark requires to be valid UTF-8" conflicts with the last paragraph
of this section. That paragraph says the native Parquet scan rejects such
columns "rather than reading them like Spark" (#4121). Spark's Parquet reader
wraps the stored bytes with `UTF8String.fromBytes` and doesn't validate them.
Can we drop that clause, or say that Comet's native Parquet scan never produces
such strings?
##########
spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala:
##########
@@ -307,12 +307,24 @@ class CometExecSuite extends CometTestBase {
}
}
+ /**
+ * A test that runs with the broadcast build side bridge on. The bridge is
off by default
+ * because it decodes invalid UTF-8 (see #7118).
+ */
+ private def bridgeTest(name: String)(body: => Unit): Unit =
Review Comment:
Building on sunchao's comment: besides the two tests that now fail, five
more #6008 tests keep passing but no longer test anything.
- `broadcast join with an unsupported probe stays on Spark`
- `broadcast join that stays on Spark leaves no build-side bridge`
- `Spark-staying broadcast join with an aggregate build side leaves no
bridge`
- `Spark-staying broadcast join with a union build side leaves no bridge`
- `Spark 3.4 AQE DPP with a V1 Parquet fact and a text dimension`
Each one asserts that nothing is bridged or tagged. With the default off,
`tagBroadcastBuildSideLeaves` returns early at `CometExecRule.scala:1577`, so
those assertions hold before `hasOnlyNativeScans` or
`clearWastedBuildSideBridges` ever runs. The filter/aggregate/union revert
tests came out of the #6786 review rounds, so it would be a shame to lose them
without anyone noticing. Could these use `bridgeTest` too?
Another option is moving the #6008 tests into
`CometBroadcastBuildSideSuite`, which already turns the bridge on for the whole
suite. Then this helper can go away, and a test added later can't pick up the
wrong default by accident.
##########
docs/source/user-guide/latest/compatibility/index.md:
##########
@@ -97,9 +97,26 @@ underlying bytes rather than on the rendered text:
bytes) but `true` in Comet. Equality, joins, grouping, ordering, and
byte-based string functions
such as `contains` can therefore disagree with Spark when non-UTF-8 bytes
are involved.
-Both differences require string data that is not valid UTF-8, which does not
occur for text read from
-Parquet or produced by string expressions. Consistent handling of invalid
UTF-8 across all native
-string paths is tracked by
+Both differences require string data that is not valid UTF-8. Such data does
not occur for text read
+from Parquet, which Spark requires to be valid UTF-8, or produced by most
string expressions. It
+can reach Comet in these ways, which are the ones to check before relying on a
result:
+
+- **Text and other row-based sources.** A Text, CSV or JSON file in a
non-UTF-8 encoding such as
+ Latin-1 holds bytes that are not valid UTF-8. These sources are read by
Spark, and the bytes are
+ decoded when Comet converts the rows to Arrow. A Text scan on a broadcast
build side is converted
+ only when `spark.comet.convert.broadcastBuildSide.enabled=true`, which is
off by default for this
+ reason (see [Data sources](../datasources.md)). With it on, a build-side
file holding the bytes
+ `1 caf\xE9` gives `hex(value)` = `3120636166EFBFBD` instead of Spark's
`3120636166E9`, and
+ `length` of `2 na\xEFve` is 7 instead of 5. Enabling any other
`spark.comet.convert.*` or
+ `spark.comet.sparkToColumnar.*` switch for a source that holds such bytes
applies the same decode.
+- **`CAST(binary AS STRING)`.** Comet decodes the bytes where Spark keeps
them. Because `min` and `max`
+ over strings run natively, a cast below them is decoded before the
comparison, and the two can swap.
+ With `b` holding `X'80'` and `X'C3A9'`, Spark returns `min(CAST(b AS
STRING))` = the `0x80` string
+ and `max` = `é`, whereas Comet returns `min` = `é` and `max` = `U+FFFD`.
This is the ordering
+ difference above and is not controlled by a config; disable Comet for the
query if the cast input can hold
Review Comment:
"is not controlled by a config; disable Comet for the query" seems broader
than needed. The aggregate serde checks `spark.comet.expression.<Name>.enabled`
(`QueryPlanSerde.scala:830`). Would setting
`spark.comet.expression.Min.enabled=false` and
`spark.comet.expression.Max.enabled=false` keep the aggregate, and the cast
inside its argument, in Spark for this case? If so, that's a much narrower
workaround to document.
##########
docs/source/user-guide/latest/compatibility/index.md:
##########
@@ -97,9 +97,26 @@ underlying bytes rather than on the rendered text:
bytes) but `true` in Comet. Equality, joins, grouping, ordering, and
byte-based string functions
such as `contains` can therefore disagree with Spark when non-UTF-8 bytes
are involved.
-Both differences require string data that is not valid UTF-8, which does not
occur for text read from
-Parquet or produced by string expressions. Consistent handling of invalid
UTF-8 across all native
-string paths is tracked by
+Both differences require string data that is not valid UTF-8. Such data does
not occur for text read
+from Parquet, which Spark requires to be valid UTF-8, or produced by most
string expressions. It
+can reach Comet in these ways, which are the ones to check before relying on a
result:
+
+- **Text and other row-based sources.** A Text, CSV or JSON file in a
non-UTF-8 encoding such as
Review Comment:
I don't think CSV and JSON belong here. Spark's CSV path decodes each line
with `new String(bytes, charset)` and builds the value with
`UTF8String.fromString`. JSON goes through an `InputStreamReader`, or through
Jackson's byte parser, which rejects invalid UTF-8. So a Latin-1 CSV or JSON
file has already been decoded to `U+FFFD` by Spark before the rows reach Comet,
and Comet's decode doesn't change the result. Text keeps the raw bytes, which
is why it's the case that matters. Could this bullet name Text, and other
sources that wrap bytes without validating them, instead?
Separately, the list reads as the complete set of paths to check, but a Text
scan's rows can also reach the decode through Comet columnar shuffle with the
default settings. In `auto` mode, `columnarShuffleFailureReasons` accepts a
row-based Spark child, and the row-to-Arrow writer decodes strings with
`decode_utf8_spark_lossy` in `get_string`. That code has been there since 1.1.0
(#4524). So a `GROUP BY` or sort-merge join over a Latin-1 Text table would see
`U+FFFD` on both releases. The opening paragraph of this section already
mentions columnar shuffle. Could the list include it, so readers don't conclude
that turning off the build-side switch is enough to keep Text bytes intact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
