docs: fix references to configuration keys that do not exist - #5063
Merged
Conversation
Shuffle-related configs were spread across four disjoint prefixes:
`spark.comet.exec.shuffle.*`, `spark.comet.columnar.shuffle.*`,
`spark.comet.native.shuffle.*`, and `spark.comet.shuffle.*`. This
consolidates all shuffle configs under `spark.comet.shuffle.*` with
`.columnar.` and `.native.` sub-namespaces for mode-specific settings.
Every renamed key is registered via `.withAlternative(...)`, so
existing user configurations continue to work (a deprecation warning
is logged when an old key is read). Per-mode leaves that were
previously misplaced move to their honest namespace:
- `spark.comet.exec.shuffle.writeBufferSize` (native-only) becomes
`spark.comet.shuffle.native.writeBufferSize`.
- `spark.comet.shuffle.preferDictionary.ratio` (JVM-only) becomes
`spark.comet.shuffle.columnar.preferDictionary.ratio`.
The `spark.comet.columnar.shuffle.*` keys also fix Category 2 in
apache#4978 (dots-in-segment): `spill.threshold`,
`memory.factor`, and `batch.size` become `spillThreshold`,
`memoryFactor`, and `batchSize`.
Part of apache#4978.
The JVM-side shuffle path in Comet is not the only columnar shuffle — native shuffle is also columnar (both use Arrow). The distinguishing axis is *where* the shuffle logic runs. Renaming the sub-namespace to `.jvm.` aligns it with the mode value users already set: `spark.comet.shuffle.mode = jvm`. Keys renamed: - spark.comet.shuffle.columnar.spillThreshold → spark.comet.shuffle.jvm.spillThreshold - spark.comet.shuffle.columnar.memoryFactor → spark.comet.shuffle.jvm.memoryFactor - spark.comet.shuffle.columnar.batchSize → spark.comet.shuffle.jvm.batchSize - spark.comet.shuffle.columnar.preferDictionary.ratio → spark.comet.shuffle.jvm.preferDictionary.ratio The pre-existing deprecated aliases (`spark.comet.columnar.shuffle.*`) are untouched — they were never `.columnar.`-scoped under the new prefix. Val names follow: `COMET_SHUFFLE_COLUMNAR_*` → `COMET_SHUFFLE_JVM_*`. The Scala/Java class names (`CometColumnarShuffle`, `CometColumnarShuffleSuite`, etc.) still use "columnar" and are left alone — that is a separate rename to consider once the config vocabulary settles.
Table column alignment tweaks after the `.columnar.` → `.jvm.` key rename. Content unchanged.
Resolves conflicts introduced by apache/main apache#4985 (remove untested async columnar shuffle) against this branch's shuffle-config renames: - Accept deletion of CometShuffleExternalSorterAsync/Sync and the merged-back CometShuffleExternalSorter; propagate this branch's jvm.* renames to the merged class (COMET_SHUFFLE_JVM_SPILL_THRESHOLD, COMET_SHUFFLE_JVM_PREFER_DICTIONARY_RATIO, COMET_SHUFFLE_COMPRESSION_*). - Drop async-config rows from jvm_shuffle.md and async-mode benchmark cases from CometShuffleBenchmark; drop the async-branch conditionals from CometColumnarShuffleSuite. - Fold the newly-introduced COMET_COLUMNAR_SHUFFLE_MAX_WRITERS_PER_EXECUTOR into this branch's naming: rename to COMET_SHUFFLE_JVM_MAX_WRITERS_PER_EXECUTOR (key spark.comet.shuffle.jvm.maxWritersPerExecutor), keeping both prior names as .withAlternative fallbacks. - Rename remaining spark.comet.exec.shuffle.mode occurrences in three make_time_shuffle*.sql test files to the new spark.comet.shuffle.mode.
# Conflicts: # spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/CometNativeShuffleWriter.scala
Regenerate the Spark SQL test diffs to reference shuffle configs by their string keys rather than CometConf constants, so the config rename in this PR no longer breaks test compilation. - spark.comet.exec.shuffle.enabled -> spark.comet.shuffle.enabled - CometConf.COMET_EXEC_SHUFFLE_WITH_RANGE_PARTITIONING_ENABLED.key -> "spark.comet.shuffle.native.partitioning.range.enabled"
# Conflicts: # benchmarks/pyspark/run_all_benchmarks.sh # spark/src/test/scala/org/apache/comet/CometConfSuite.scala
…pache#5062) Audit the user guide and contributor guide for references to issues that have since been closed, and make the remaining issue and PR links use a single format. Thirteen places described a limitation that no longer exists, verified against the current serde and native code rather than the issue state alone: size() over MapType, try_mod / EvalMode.TRY, CAST(map AS map), spark.sql.legacy.castComplexTypesToString, replace with an empty search string, initcap, str_to_map with the legacy truncate flag, AVG(decimal) over a window, Percentile, the pyarrow-udf row round-trip, the datafusion-spark migration epic, the awslabs TPC-DS epic, and the interval type epic. Also drop or repoint links that pointed at a closed or unrelated tracker: the apache#4098 rows in expressions.md, translate, the TimestampNTZ scan note, and the from_utc_timestamp timezone-parser note. Convert bare URLs and bare #NNNN mentions to inline [#NNNN](url), keeping reference-style definitions where a file already used them, and point URLs at /pull/ where the number is a pull request.
Several docs referenced configuration keys that were never registered or were renamed. Corrected the expression/operator opt-in prefix (spark.comet.expr.* -> spark.comet.expression.* / spark.comet.operator.*), removed references to a global allowIncompatible key that does not exist, and updated stale shuffle, memory pool, and metrics keys.
# Conflicts: # benchmarks/README.md # docs/source/contributor-guide/benchmarking_spark_sql_perf.md # docs/source/contributor-guide/expression-audits/predicate_funcs.md # docs/source/contributor-guide/expression-audits/string_funcs.md # docs/source/contributor-guide/native_shuffle.md
andygrove
marked this pull request as ready for review
July 28, 2026 16:14
Member
Author
|
@mbutrovich this PR includes the fix for the stale |
mbutrovich
approved these changes
Jul 28, 2026
mbutrovich
left a comment
mbutrovich
left a comment
Contributor
There was a problem hiding this comment.
Thanks @andygrove! Nice cleanup!
comphead
pushed a commit
to comphead/arrow-datafusion-comet
that referenced
this pull request
Aug 26, 2026
…5063) * refactor: unify shuffle configs under `spark.comet.shuffle.*` prefix Shuffle-related configs were spread across four disjoint prefixes: `spark.comet.exec.shuffle.*`, `spark.comet.columnar.shuffle.*`, `spark.comet.native.shuffle.*`, and `spark.comet.shuffle.*`. This consolidates all shuffle configs under `spark.comet.shuffle.*` with `.columnar.` and `.native.` sub-namespaces for mode-specific settings. Every renamed key is registered via `.withAlternative(...)`, so existing user configurations continue to work (a deprecation warning is logged when an old key is read). Per-mode leaves that were previously misplaced move to their honest namespace: - `spark.comet.exec.shuffle.writeBufferSize` (native-only) becomes `spark.comet.shuffle.native.writeBufferSize`. - `spark.comet.shuffle.preferDictionary.ratio` (JVM-only) becomes `spark.comet.shuffle.columnar.preferDictionary.ratio`. The `spark.comet.columnar.shuffle.*` keys also fix Category 2 in apache#4978 (dots-in-segment): `spill.threshold`, `memory.factor`, and `batch.size` become `spillThreshold`, `memoryFactor`, and `batchSize`. Part of apache#4978. * refactor: rename `.columnar.` sub-namespace to `.jvm.` The JVM-side shuffle path in Comet is not the only columnar shuffle — native shuffle is also columnar (both use Arrow). The distinguishing axis is *where* the shuffle logic runs. Renaming the sub-namespace to `.jvm.` aligns it with the mode value users already set: `spark.comet.shuffle.mode = jvm`. Keys renamed: - spark.comet.shuffle.columnar.spillThreshold → spark.comet.shuffle.jvm.spillThreshold - spark.comet.shuffle.columnar.memoryFactor → spark.comet.shuffle.jvm.memoryFactor - spark.comet.shuffle.columnar.batchSize → spark.comet.shuffle.jvm.batchSize - spark.comet.shuffle.columnar.preferDictionary.ratio → spark.comet.shuffle.jvm.preferDictionary.ratio The pre-existing deprecated aliases (`spark.comet.columnar.shuffle.*`) are untouched — they were never `.columnar.`-scoped under the new prefix. Val names follow: `COMET_SHUFFLE_COLUMNAR_*` → `COMET_SHUFFLE_JVM_*`. The Scala/Java class names (`CometColumnarShuffle`, `CometColumnarShuffleSuite`, etc.) still use "columnar" and are left alone — that is a separate rename to consider once the config vocabulary settles. * chore: re-run prettier on shuffle docs Table column alignment tweaks after the `.columnar.` → `.jvm.` key rename. Content unchanged. * style: apply spotless formatting to CometShuffleExternalSorter * refactor: move maxBufferBytes under spark.comet.shuffle.native prefix * test: update Spark diffs for renamed shuffle configs Regenerate the Spark SQL test diffs to reference shuffle configs by their string keys rather than CometConf constants, so the config rename in this PR no longer breaks test compilation. - spark.comet.exec.shuffle.enabled -> spark.comet.shuffle.enabled - CometConf.COMET_EXEC_SHUFFLE_WITH_RANGE_PARTITIONING_ENABLED.key -> "spark.comet.shuffle.native.partitioning.range.enabled" * docs: refresh stale issue references and normalize issue link format (apache#5062) Audit the user guide and contributor guide for references to issues that have since been closed, and make the remaining issue and PR links use a single format. Thirteen places described a limitation that no longer exists, verified against the current serde and native code rather than the issue state alone: size() over MapType, try_mod / EvalMode.TRY, CAST(map AS map), spark.sql.legacy.castComplexTypesToString, replace with an empty search string, initcap, str_to_map with the legacy truncate flag, AVG(decimal) over a window, Percentile, the pyarrow-udf row round-trip, the datafusion-spark migration epic, the awslabs TPC-DS epic, and the interval type epic. Also drop or repoint links that pointed at a closed or unrelated tracker: the apache#4098 rows in expressions.md, translate, the TimestampNTZ scan note, and the from_utc_timestamp timezone-parser note. Convert bare URLs and bare #NNNN mentions to inline [#NNNN](url), keeping reference-style definitions where a file already used them, and point URLs at /pull/ where the number is a pull request. * docs: fix references to configuration keys that do not exist Several docs referenced configuration keys that were never registered or were renamed. Corrected the expression/operator opt-in prefix (spark.comet.expr.* -> spark.comet.expression.* / spark.comet.operator.*), removed references to a global allowIncompatible key that does not exist, and updated stale shuffle, memory pool, and metrics keys.
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
2 participants
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?
No issue filed; this is a docs cleanup found by sweeping every tracked markdown file for
spark.comet.*keys and validating them againstCometConf.scala.Rationale for this change
Several docs reference configuration keys that do not exist. A user copying these into a
spark-submitcommand gets a silently ignored setting, which is worse than an error because the doc implies the setting took effect.The largest group is the expression opt-in prefix. The real namespace is
spark.comet.expression.*(COMET_EXPR_CONFIG_PREFIXinCometConf.scala), but the contributor guide, the versioning policy, and two expression-audit pages all usespark.comet.expr.*. Those same places also describe a globalallowIncompatiblekey, which has never existed: the opt-in is always per-expression or per-operator.While verifying the remaining
spark.comet.expression.<Name>.*references against the serde registry, three of them turned out to name something that is not a valid expression config, and the surrounding prose was stale in the same way, so those claims were corrected too.What changes are included in this PR?
Wrong config prefix, corrected to
spark.comet.expression.*:contributor-guide/adding_a_new_expression.md(3 references, plus removal of the nonexistent global key from theIncompatibledescription)about/versioning_policy.md.claude/skills/audit-comet-expression/SKILL.mdcontributor-guide/benchmarking_macos.md, which passedspark.comet.expression.allowIncompatible=trueExplode is gated at the operator level, not the expression level, so
expression-audits/generator_funcs.mdnow points atspark.comet.operator.GenerateExec.allowIncompatible.Config keys naming an expression that does not exist:
spark.comet.expression.regexp.allowIncompatible(predicate_funcs.md,string_funcs.md). There is noregexpexpression; the real keys areRLikeandRegExpReplace.spark.comet.expression.Upper.allowIncompatible/Lower.allowIncompatible(string_funcs.md). Case conversion overrides the opt-in key vianativeOptInConfigKeyOverride, so the key isspark.comet.caseConversion.enabled.Correcting those four also meant correcting the claims around them, which predate the codegen dispatcher:
rlike,regexp_replace,upperandlowerareCompatibleby default now, with the native regexp / case-mapping path opt-in, not "unconditionallyIncompatible". Theregexp_replaceentry also citedRegExp.isSupportedPattern, a symbol that no longer exists.Renamed or never-registered keys:
spark.comet.metrics.detailed(user-guide/latest/metrics.md) was never registered; those metrics are always reportedspark.comet.memoryPool.fractiontospark.comet.exec.memoryPool.fraction(benchmark-results/tpc-ds.md,tpc-h.md)spark.comet.shuffle.write.buffer.sizetospark.comet.shuffle.native.writeBufferSize(native_shuffle.md)spark.comet.exec.shuffle.enableFastEncodingandspark.comet.exec.shuffle.fallbackToColumnar, both removed, dropped frombenchmarking_macos.mdspark.comet.exec.all.enabled, removed, dropped frombenchmarking_spark_sql_perf.mdandbenchmarks/README.mdspark.comet.columnar.shuffle.enabledtospark.comet.shuffle.mode=jvm(benchmarking_spark_sql_perf.md)spark.comet.cast.allowIncompatibletospark.comet.expression.Cast.allowIncompatible(benchmarks/README.md)Deliberately left alone:
docs/source/changelog/*, which records historical PR titles mentioning since-removed keys, thedocs/comet-0.1x/version archives, and placeholder forms such asspark.comet.exec.<yourOperator>.enabled.How are these changes tested?
Docs only, no code changes. Verified by extracting every
spark.comet.*token from all tracked.mdand.rstfiles and diffing that set against the keys registered inCometConf.scala, expanded for the dynamicexpression.<Name>.*,operator.<Name>.*andexec.<name>.enabledpatterns. Every expression name appearing in a config key was checked against theclassOf[...]entries inQueryPlanSerde.scala, and each corrected claim was read back against the relevant serde. After the change the only unmatched keys are the three changelog entries noted above.prettierreports no formatting changes.