Skip to content

refactor: unify shuffle configs under spark.comet.shuffle.* prefix - #4986

Merged
mbutrovich merged 9 commits into
apache:mainfrom
andygrove:rename-shuffle-configs
Jul 28, 2026
Merged

refactor: unify shuffle configs under spark.comet.shuffle.* prefix#4986
mbutrovich merged 9 commits into
apache:mainfrom
andygrove:rename-shuffle-configs

Conversation

@andygrove

@andygrove andygrove commented Jul 20, 2026

Copy link
Copy Markdown
Member

Summary

Part of #4978 (Category 2 + Category 4).

Shuffle-related configs live under four disjoint prefixes today:
spark.comet.exec.shuffle.*, spark.comet.columnar.shuffle.*,
spark.comet.native.shuffle.*, and spark.comet.shuffle.*. This PR
consolidates all shuffle configs under spark.comet.shuffle.* with
.jvm. and .native. sub-namespaces for mode-specific settings,
and fixes the dots-in-segment naming for the four JVM-shuffle keys.

The sub-namespaces (.jvm. / .native.) match the values users already
set on spark.comet.shuffle.mode (jvm / native / auto). Note that
both modes are columnar (both use Arrow); the distinction is where the
shuffle logic runs, so .jvm. is the honest name.

Every renamed key is registered via .withAlternative(...), so
existing user configurations keep working (a one-time deprecation
warning is logged per old key). No behavior change.

Rename table

Master / shared (both modes):

Old key New key
spark.comet.exec.shuffle.enabled spark.comet.shuffle.enabled
spark.comet.exec.shuffle.mode spark.comet.shuffle.mode
spark.comet.exec.shuffle.directRead.enabled spark.comet.shuffle.directRead.enabled
spark.comet.exec.shuffle.convertFromSparkPlan.enabled spark.comet.shuffle.convertFromSparkPlan.enabled
spark.comet.exec.shuffle.revertRedundantColumnar.enabled spark.comet.shuffle.revertRedundantColumnar.enabled
spark.comet.exec.shuffle.compression.codec spark.comet.shuffle.compression.codec
spark.comet.exec.shuffle.compression.zstd.level spark.comet.shuffle.compression.zstd.level

Native-shuffle-only (moved from spark.comet.native.shuffle.* and one leaf from spark.comet.exec.shuffle.*):

Old key New key
spark.comet.exec.shuffle.writeBufferSize spark.comet.shuffle.native.writeBufferSize
spark.comet.native.shuffle.partitioning.hash.enabled spark.comet.shuffle.native.partitioning.hash.enabled
spark.comet.native.shuffle.partitioning.range.enabled spark.comet.shuffle.native.partitioning.range.enabled
spark.comet.native.shuffle.partitioning.roundrobin.enabled spark.comet.shuffle.native.partitioning.roundrobin.enabled
spark.comet.native.shuffle.partitioning.roundrobin.maxHashColumns spark.comet.shuffle.native.partitioning.roundrobin.maxHashColumns

JVM-shuffle-only (moved from spark.comet.columnar.shuffle.*; also fixes Category 2 dots-in-segment):

Old key New key
spark.comet.columnar.shuffle.spill.threshold spark.comet.shuffle.jvm.spillThreshold
spark.comet.columnar.shuffle.memory.factor spark.comet.shuffle.jvm.memoryFactor
spark.comet.columnar.shuffle.batch.size spark.comet.shuffle.jvm.batchSize
spark.comet.shuffle.preferDictionary.ratio spark.comet.shuffle.jvm.preferDictionary.ratio

preferDictionary.ratio and writeBufferSize were previously at
mode-agnostic namespaces but their doc/call sites confirm they are
JVM-only and native-only respectively; the rename puts them in their
honest place.

Not in this PR

  • spark.comet.columnar.shuffle.max.writers.per.executor isn't
    renamed here — the val COMET_COLUMNAR_SHUFFLE_MAX_WRITERS_PER_EXECUTOR
    is introduced by Bug triage results: 2026-07-20 #4980 (async columnar shuffle removal), which hasn't
    landed on main yet. A tiny follow-up PR can add its rename once Bug triage results: 2026-07-20 #4980
    merges.
  • The spark.comet.columnar.shuffle.async.* keys are being removed by
    Bug triage results: 2026-07-20 #4980 — left alone here.
  • The Scala/Java class names (CometColumnarShuffle, CometColumnarShuffleSuite,
    etc.) still use "columnar" — that is a separate rename to consider once
    the config vocabulary settles.

Test plan

  • ./mvnw -Pspark-3.5 test -pl spark -Dsuites="org.apache.comet.CometConfSuite" — 10 tests pass (including 3 new alias tests for the renames)
  • ./mvnw -Pspark-3.5 spotless:check — clean
  • Full compile test-compile under Spark 3.5 — clean
  • CI runs the broader test matrix

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.
@andygrove
andygrove marked this pull request as draft July 20, 2026 22:10
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.
@andygrove
andygrove marked this pull request as ready for review July 20, 2026 22:55
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.
@andygrove andygrove added this to the 1.0.0 milestone Jul 21, 2026
# 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"

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andygrove Athough we used exec.shuffle before, I checked spark conf and they used just spark.shuffle.

However this is a super breaking change, people using Comet would have to reconfigure their jobs. I'm not sure about migration path

@comphead

Copy link
Copy Markdown
Contributor

Thanks @andygrove Athough we used exec.shuffle before, I checked spark conf and they used just spark.shuffle.

However this is a super breaking change, people using Comet would have to reconfigure their jobs. I'm not sure about migration path

We should prob start the same idea we have started in DF with upgrade guide, otherwise migration would be painful

@andygrove

Copy link
Copy Markdown
Member Author

Thanks @andygrove Athough we used exec.shuffle before, I checked spark conf and they used just spark.shuffle.

However this is a super breaking change, people using Comet would have to reconfigure their jobs. I'm not sure about migration path

This is not a breaking changes. All the original configs are still supported but deprecated.

@andygrove

Copy link
Copy Markdown
Member Author

Thanks @andygrove Athough we used exec.shuffle before, I checked spark conf and they used just spark.shuffle.
However this is a super breaking change, people using Comet would have to reconfigure their jobs. I'm not sure about migration path

This is not a breaking changes. All the original configs are still supported but deprecated.

Example from this PR:

val COMET_SHUFFLE_DIRECT_READ_ENABLED: ConfigEntry[Boolean] =
    conf("spark.comet.shuffle.directRead.enabled")
      .withAlternative(s"$COMET_EXEC_CONFIG_PREFIX.shuffle.directRead.enabled")
      ...

# Conflicts:
#	benchmarks/pyspark/run_all_benchmarks.sh
#	spark/src/test/scala/org/apache/comet/CometConfSuite.scala

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change, I think. Thanks @andygrove!

| `spark.comet.shuffle.mode` | `auto` | Shuffle mode: `native`, `jvm`, or `auto` |
| `spark.comet.shuffle.compression.codec` | `zstd` | Compression codec |
| `spark.comet.shuffle.compression.zstd.level` | `1` | Zstd compression level |
| `spark.comet.shuffle.write.buffer.size` | `1MB` | Write buffer size |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Configuration table's write-buffer-size row still shows spark.comet.shuffle.write.buffer.size, which is not a valid config key (the actual key was spark.comet.exec.shuffle.writeBufferSize, now spark.comet.shuffle.native.writeBufferSize). Every other row in this same table was correctly renamed by this PR; this one was missed. Update to spark.comet.shuffle.native.writeBufferSize.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbutrovich could we merge this one and then do a separate docs-only follow up? Save some CI time...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. You have some stacked docs-only PRs behind this one, I think,

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with the caveat of fixing the followup in one of the stacked PRs.

@mbutrovich
mbutrovich merged commit f4ae9e0 into apache:main Jul 28, 2026
72 checks passed
@andygrove
andygrove deleted the rename-shuffle-configs branch July 28, 2026 16:34
comphead pushed a commit to comphead/arrow-datafusion-comet that referenced this pull request Aug 26, 2026
…pache#4986)

* 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"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants