andygrove commented on PR #7123: URL: https://github.com/apache/datafusion-comet/pull/7123#issuecomment-6289755732
@sunchao @viirya thanks for the reviews. I've reworked this PR based on them, so it needs a fresh look. Instead of turning the build-side bridge off for every format, the bridge stays on by default and Text is the exception. Of Spark's built-in sources that reach the bridge, Text is the only one whose strings carry no UTF-8 guarantee. ORC, Avro and Iceberg define strings as UTF-8, and Parquet, CSV and JSON never reach the bridge because they have their own `spark.comet.convert.*` configs. A Text build side is now bridged only with the new `spark.comet.convert.broadcastBuildSide.text.enabled` (off by default). Without it, the scan reports a fallback reason that names the config. For consistency with CSV, JSON and Parquet, there is also a new `spark.comet.convert.text.enabled` that converts every Text scan. Tests: - The #6008 tests now live in `CometBroadcastBuildSideSuite`, which turns the Text opt-in on for the whole suite, as viirya suggested. That covers the two tests that failed and the five that had stopped testing anything, and the `bridgeTest` helper is gone. The move is its own commit (cd42aeb14a). The bodies are unchanged apart from `bridgeTest(` becoming `test(`. - A new #7118 test covers the default, the Text opt-in and `spark.comet.convert.text.enabled` with a Latin-1 file, through V1 and V2, with AQE on and off. Compatibility guide: I took all four of viirya's points and checked each one with a probe against Spark 4.1. - The Parquet sentence now says that Comet's native Parquet scan never produces such strings. - The bullet names Text rather than CSV and JSON. Spark decodes CSV. For JSON, a line with such bytes is malformed, so the field comes back null and only `_corrupt_record` keeps the bytes. The guide now says that. - Columnar shuffle is on the list now. With default configs, repartitioning a Latin-1 Text table returns `EFBFBD` where Spark keeps `E9`. The `GROUP BY` example matched Spark in my probe only because Comet left that shuffle on Spark, since its parent aggregate stayed on Spark. - The per-expression configs alone don't fix `min`/`max`. With `spark.comet.expression.Min.enabled=false` and `spark.comet.expression.Max.enabled=false`, the aggregate runs in Spark, but the columnar shuffle between the partial and final aggregates still decodes the partial results. With two input partitions, min and max still swap. Adding `spark.comet.shuffle.mode=native` gives Spark's answer, so the guide lists all three. I've updated the PR description and added `run-spark-4.1-tests`, since this changes the planner. -- 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]
