chore: remove dead Parquet parallel-IO configs - #4981
Merged
Conversation
These configs were consumed by the JVM-side Parquet reader that has since been removed. They are referenced nowhere in current native or JVM code and mislead operators tuning benchmarks. Removed configs: - spark.comet.parquet.read.parallel.io.enabled - spark.comet.parquet.read.parallel.io.thread-pool.size - spark.comet.parquet.read.io.mergeRanges - spark.comet.parquet.read.io.mergeRanges.delta - spark.comet.parquet.read.io.adjust.readRange.skew Also removes the stale ParquetReadSuite "test merge scan range" test, which only asserted that the (dead) config propagated to the Hadoop conf. Refs apache#4842.
Contributor
|
Can you nab "spark.comet.use.lazyMaterialization" as well? |
Contributor
|
Actually, CI is green, we can get it in a followup. |
2 tasks
andygrove
added a commit
to andygrove/datafusion-comet
that referenced
this pull request
Jul 27, 2026
- Remove ### Parallel I/O subsection: the four spark.comet.parquet.read.parallel.io.* and .mergeRanges* configs were removed in apache#4981 - Fix maxTempDirectorySize scope: limit is per Spark task, not per executor - Move ### Parquet Native Scans (merged in from main) under new ## Parquet Reader Tuning h2 so all Parquet guidance is co-located - Promote ### Reducing Row/Columnar Conversion Overhead to its own h2 since spark.comet.exec.transitionRevert.* counts C2R transitions across the whole stage and is not shuffle-specific - Clarify shuffle compression: spark.comet.exec.shuffle.compression.codec governs both native and JVM columnar shuffle; spark.shuffle.compress=false disables compression only for the native shuffle - Fix Iceberg wording: spark.comet.scan.icebergNative.enabled defaults to true
andygrove
added a commit
to andygrove/datafusion-comet
that referenced
this pull request
Jul 28, 2026
- Remove ### Parallel I/O subsection: the four spark.comet.parquet.read.parallel.io.* and .mergeRanges* configs were removed in apache#4981 - Fix maxTempDirectorySize scope: limit is per Spark task, not per executor - Move ### Parquet Native Scans (merged in from main) under new ## Parquet Reader Tuning h2 so all Parquet guidance is co-located - Promote ### Reducing Row/Columnar Conversion Overhead to its own h2 since spark.comet.exec.transitionRevert.* counts C2R transitions across the whole stage and is not shuffle-specific - Clarify shuffle compression: spark.comet.exec.shuffle.compression.codec governs both native and JVM columnar shuffle; spark.shuffle.compress=false disables compression only for the native shuffle - Fix Iceberg wording: spark.comet.scan.icebergNative.enabled defaults to true
andygrove
added a commit
that referenced
this pull request
Jul 28, 2026
* 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
#4978 (dots-in-segment): `spill.threshold`,
`memory.factor`, and `batch.size` become `spillThreshold`,
`memoryFactor`, and `batchSize`.
Part of #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: expand tuning guide with performance and memory configs for 1.0
Audited all CometConf entries and added documentation for user-facing
performance and memory knobs that were undocumented in the tuning guide:
batch size, spill disk cap, Parquet parallel I/O and filter pushdown,
Iceberg data-file concurrency, shuffle compression codec/level,
row/columnar transition revert, and metrics overhead. Also fixed an
incorrect claim about the default shuffle compression codec (LZ4, not
ZSTD) and moved spark.comet.maxTempDirectorySize into the tuning
category so it renders in the Memory & Tuning table.
* style: apply spotless formatting to COMET_MAX_TEMP_DIRECTORY_SIZE doc string
* docs: address review feedback on tuning guide
- Remove ### Parallel I/O subsection: the four spark.comet.parquet.read.parallel.io.*
and .mergeRanges* configs were removed in #4981
- Fix maxTempDirectorySize scope: limit is per Spark task, not per executor
- Move ### Parquet Native Scans (merged in from main) under new
## Parquet Reader Tuning h2 so all Parquet guidance is co-located
- Promote ### Reducing Row/Columnar Conversion Overhead to its own h2 since
spark.comet.exec.transitionRevert.* counts C2R transitions across the whole
stage and is not shuffle-specific
- Clarify shuffle compression: spark.comet.exec.shuffle.compression.codec
governs both native and JVM columnar shuffle; spark.shuffle.compress=false
disables compression only for the native shuffle
- Fix Iceberg wording: spark.comet.scan.icebergNative.enabled defaults to true
* docs: use renamed shuffle config keys in tuning guide
---------
Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com>
comphead
pushed a commit
to comphead/arrow-datafusion-comet
that referenced
this pull request
Aug 26, 2026
These configs were consumed by the JVM-side Parquet reader that has since been removed. They are referenced nowhere in current native or JVM code and mislead operators tuning benchmarks. Removed configs: - spark.comet.parquet.read.parallel.io.enabled - spark.comet.parquet.read.parallel.io.thread-pool.size - spark.comet.parquet.read.io.mergeRanges - spark.comet.parquet.read.io.mergeRanges.delta - spark.comet.parquet.read.io.adjust.readRange.skew Also removes the stale ParquetReadSuite "test merge scan range" test, which only asserted that the (dead) config propagated to the Hadoop conf. Refs apache#4842.
comphead
pushed a commit
to comphead/arrow-datafusion-comet
that referenced
this pull request
Aug 26, 2026
…#4908) * 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: expand tuning guide with performance and memory configs for 1.0 Audited all CometConf entries and added documentation for user-facing performance and memory knobs that were undocumented in the tuning guide: batch size, spill disk cap, Parquet parallel I/O and filter pushdown, Iceberg data-file concurrency, shuffle compression codec/level, row/columnar transition revert, and metrics overhead. Also fixed an incorrect claim about the default shuffle compression codec (LZ4, not ZSTD) and moved spark.comet.maxTempDirectorySize into the tuning category so it renders in the Memory & Tuning table. * style: apply spotless formatting to COMET_MAX_TEMP_DIRECTORY_SIZE doc string * docs: address review feedback on tuning guide - Remove ### Parallel I/O subsection: the four spark.comet.parquet.read.parallel.io.* and .mergeRanges* configs were removed in apache#4981 - Fix maxTempDirectorySize scope: limit is per Spark task, not per executor - Move ### Parquet Native Scans (merged in from main) under new ## Parquet Reader Tuning h2 so all Parquet guidance is co-located - Promote ### Reducing Row/Columnar Conversion Overhead to its own h2 since spark.comet.exec.transitionRevert.* counts C2R transitions across the whole stage and is not shuffle-specific - Clarify shuffle compression: spark.comet.exec.shuffle.compression.codec governs both native and JVM columnar shuffle; spark.shuffle.compress=false disables compression only for the native shuffle - Fix Iceberg wording: spark.comet.scan.icebergNative.enabled defaults to true * docs: use renamed shuffle config keys in tuning guide --------- Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com>
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?
Partially addresses #4842 (Item #9 in the remediation section: "Delete or implement the dead IO configs (
parallel.io.*,mergeRanges) so benchmarking isn't misled").Rationale for this change
The following configs are defined in
CometConf.scalabut referenced nowhere in current native or JVM code. They were consumed by the JVM-side Parquet reader that has since been removed:spark.comet.parquet.read.parallel.io.enabled(defaulttrue)spark.comet.parquet.read.parallel.io.thread-pool.size(default16)spark.comet.parquet.read.io.mergeRanges(defaulttrue)spark.comet.parquet.read.io.mergeRanges.delta(default8 MB)spark.comet.parquet.read.io.adjust.readRange.skew(defaultfalse)These are a trap: they appear tunable but change nothing. Anyone benchmarking "Comet with IO tuning" is tuning a no-op.
What changes are included in this PR?
CometConf.scala.ParquetReadSuite"test merge scan range" test, which only asserted that the (dead) config propagated to the Hadoop conf.The auto-generated
configs.mdwill be regenerated on the next docs build and will no longer list these entries.How are these changes tested?
Compile-checked against
spark-3.5. No behavior change — these configs were no-ops in current code.