andygrove commented on code in PR #7123: URL: https://github.com/apache/datafusion-comet/pull/7123#discussion_r4358003152
########## spark/src/test/scala/org/apache/comet/CometBroadcastBuildSideSuite.scala: ########## @@ -19,20 +19,31 @@ package org.apache.comet -import org.apache.spark.sql.CometTestBase +import java.io.File +import java.nio.charset.StandardCharsets.UTF_8 +import java.nio.file.Files + +import org.apache.spark.SparkConf Review Comment: Fixed in 0e66bc0f7 by dropping the import. The same CI run also failed Spotless on a comment in `CometExecRule.shouldConvertTextScan`, which that commit reformats. A local `compile test-compile scalafix:scalafix -Psemanticdb` and `spotless:apply` now leave the tree unchanged. ########## docs/source/user-guide/latest/compatibility/index.md: ########## @@ -97,9 +97,38 @@ 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. Comet's native Parquet scan never +produces such strings (it rejects them, see below), and neither do most string expressions. They can +reach Comet in these ways, which are the ones to check before relying on a result: + +- **Text files.** Spark's Text reader keeps the bytes of each line as they are, so a Text file in an + encoding other than UTF-8, such as Latin-1, holds strings that are not valid UTF-8. Comet decodes + them when it converts a Text scan to Arrow, which it does only when + `spark.comet.convert.text.enabled=true`, when the scan is on a broadcast build side and + `spark.comet.convert.broadcastBuildSide.text.enabled=true`, or when + `spark.comet.sparkToColumnar.enabled=true` and `spark.comet.sparkToColumnar.supportedOperatorList` + names the scan (see [Data sources](../datasources.md#text)). For example, with a build-side file + holding the bytes `1 caf\xE9`, `hex(value)` returns `3120636166EFBFBD` instead of Spark's + `3120636166E9`, and `length` of `2 na\xEFve` is 7 instead of 5. Spark decodes CSV files as it reads + them, so they never hold such strings, and its JSON reader treats a line with such bytes as + malformed, so only the corrupt record column keeps them. Spark's ORC and Avro readers keep stored + bytes without checking them, but those formats define strings as UTF-8, so only a malformed file is + affected. +- **Comet columnar shuffle.** When Comet shuffles rows that come from a Spark operator, such as a + Text scan or a Spark partial aggregate, it converts them to Arrow and decodes their strings in the + same way. It does this with the default `spark.comet.shuffle.mode=auto`, so, for example, + repartitioning a Latin-1 Text table can return `U+FFFD` where Spark keeps the original bytes. + Setting `spark.comet.shuffle.mode=native` keeps such shuffles on Spark. +- **`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. If the cast input can hold bytes that are not valid UTF-8, disable Comet for the + query, or set `spark.comet.expression.Min.enabled=false`, `spark.comet.expression.Max.enabled=false` Review Comment: Right, the probe only covered the cast as the direct argument of `min` and `max`. With the cast in a projection below the aggregate, as in your example, Comet runs that projection natively and decodes `b` before the Spark aggregate sees it, so the three configs do not help. In 0e66bc0f7 the guide keeps disabling Comet as the general answer, gives the three configs only for the case where the cast is the direct argument of `min` or `max`, and names the projection shape as one they do not cover. -- 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]
