Repository navigation
fix: [branch-1.2] keep Text build sides on Spark by default and add Text conversion configs (#7123) - #7205
Merged
Conversation
…n configs (apache#7123) * fix: default broadcast build side conversion to off and document invalid UTF-8 cases Converting a Text scan on a broadcast build side to Arrow decodes invalid UTF-8 to U+FFFD, so 1.2.0 could return different results from 1.1.0. Default spark.comet.convert.broadcastBuildSide.enabled to false and name the Text-source and CAST(binary AS STRING) cases in the compatibility guide. * test: move the apache#6008 build-side tests into CometBroadcastBuildSideSuite CometBroadcastBuildSideSuite turns the build-side bridge on for every test, so the tests no longer need the bridgeTest helper, and a test added later cannot miss the setting. The two tests that failed because they were not wrapped in bridgeTest now run with the bridge on, and the five that asserted nothing was bridged now exercise the bridge again. The test bodies are unchanged. * fix: keep Text build sides on Spark by default and add Text conversion configs Converting a Text scan to Arrow replaces bytes that are not valid UTF-8 with U+FFFD, so bridging a Text build side by default (apache#6786) could return different results from 1.1.0 (apache#7118). The other formats that reach the bridge either define strings as UTF-8 (ORC, Avro, Iceberg) or are decoded by Spark, so spark.comet.convert.broadcastBuildSide.enabled goes back to on by default, and Text gets its own configs: - spark.comet.convert.broadcastBuildSide.text.enabled (default false) bridges a Text build side. Without it, the scan records a fallback reason that names the config. - spark.comet.convert.text.enabled (default false) converts every Text scan, like the CSV, JSON and Parquet configs. The sparkToColumnar operator list still converts Text as before. The compatibility guide now names Text rather than CSV and JSON, no longer says that Spark requires Parquet strings to be valid UTF-8, adds Comet columnar shuffle as a path, and gives the configs that keep min/max over CAST(binary AS STRING) in Spark. * test: drop the QueryStageExec import that the test move left unused * fix: convert a Text build side only with spark.comet.convert.text.enabled Drop spark.comet.convert.broadcastBuildSide.text.enabled. A Text scan, on a build side or not, is now converted only by spark.comet.convert.text.enabled, like CSV, JSON and Parquet, and the build-side fallback reason names that config. Text goes through isConversionEnabled, so naming FileSourceScan or BatchScan in spark.comet.sparkToColumnar.supportedOperatorList still converts it but is deprecated. The apache#6008 build-side tests read ORC instead of Text, since a global Text switch would convert their probes and no-bridge assertions too. * docs: describe Text conversion through its own config only Update the Text, build-side and compatibility sections for the removal of spark.comet.convert.broadcastBuildSide.text.enabled, and note in the migration guide that converting Text through sparkToColumnar is deprecated. * fix: drop the unused SparkConf import and narrow the min/max workaround The suite no longer overrides sparkConf, so its SparkConf import failed Scalafix and the strict-warnings build. Reformat a CometExecRule comment that Spotless rejected. The three-config min/max workaround only keeps the decode out of the query when the cast is the aggregate's own argument. A cast in a projection below the aggregate runs natively and decodes first, so the guide now says so and keeps disabling Comet as the general answer. (cherry picked from commit 2b1c6a3)
manuzhang
approved these changes
Oct 26, 2026
Member
Author
|
Thank you for the review @manuzhang! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #7118.
Backport of #7123 (squash commit 2b1c6a3) to
branch-1.2.Rationale for this change
#6786 is already on
branch-1.2. It made Comet convert a Text scan on a broadcast join's build side to Arrow by default, where 1.1.0 left it on Spark. Arrow strings must be valid UTF-8, so Comet replaces each ill-formed sequence withU+FFFD, andhex,length, hashing, equality and ordering over a Latin-1 Text file can then differ from Spark. Without this fix, 1.2.0 would return different results from 1.1.0 for the same query. See #7123 for the full rationale.What changes are included in this PR?
A clean cherry-pick of #7123 with
-x. The changed lines are identical to the source commit, and no file was adapted:spark.comet.convert.text.enabled(defaultfalse), so a Text scan is converted only by its own config, like CSV, JSON and Parquet.spark.comet.convert.broadcastBuildSide.enabledstays on, but a Text build side is bridged only when the Text config is set.datasources.mdand migration guide updates.BroadcastHashJoin#6008 build-side tests move fromCometExecSuitetoCometBroadcastBuildSideSuite, and run against an ORC build side.docs/source/user-guide/latest/configs.mdonbranch-1.2still holds theCONFIG_TABLEmarkers rather than generated tables, so there is no row to update.Backports to other branches: none needed. #6786 is not on
branch-1.0orbranch-1.1, so neither has the behavior this fixes.How are these changes tested?
On this branch, with Spark 4.1 (the default profile):
CometBroadcastBuildSideSuiteandCometExecSuiteran 194 tests: 193 passed, one was canceled (a Spark 3.4-only test) and one failed. The failure is "native scan loads the next file's metadata while it reads the current file" (0 did not equal 3). It exercises perf: load the next Parquet file's footer and page index while the current file is read #7094, which merged after thelibcometI used was built, and this PR does not touch it. I did not rerun it with a freshly built library../mvnw test-compile -Pspark-3.5,delta -Pstrict-warningsand the ScalafixCHECKrun both pass.