Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/pr_build_linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ jobs:
org.apache.comet.exec.CometJoinSuite
org.apache.spark.sql.comet.CometMapInBatchSuite
org.apache.comet.CometNativeSuite
org.apache.comet.CometConfSuite
org.apache.comet.CometSetOpWithGroupBySuite
org.apache.comet.CometSparkSessionExtensionsSuite
org.apache.spark.CometPluginsSuite
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/pr_build_macos.yml
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ jobs:
org.apache.comet.exec.CometJoinSuite
org.apache.spark.sql.comet.CometMapInBatchSuite
org.apache.comet.CometNativeSuite
org.apache.comet.CometConfSuite
org.apache.comet.CometSetOpWithGroupBySuite
org.apache.comet.CometSparkSessionExtensionsSuite
org.apache.spark.CometPluginsSuite
Expand Down
2 changes: 1 addition & 1 deletion docs/source/contributor-guide/benchmark-results/tpc-h.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,6 @@ spark.memory.offHeap.size=32G
### Comet (Tuned)

```properties
spark.comet.exec.replaceSortMergeJoin=true
spark.comet.exec.forceShuffledHashJoin=true
spark.comet.memoryPool.fraction=0.8
```
2 changes: 1 addition & 1 deletion docs/source/contributor-guide/benchmarking_macos.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ $SPARK_HOME/bin/spark-submit \
--conf spark.comet.enabled=true \
--conf spark.comet.exec.shuffle.enableFastEncoding=true \
--conf spark.comet.exec.shuffle.fallbackToColumnar=true \
--conf spark.comet.exec.replaceSortMergeJoin=true \
--conf spark.comet.exec.forceShuffledHashJoin=true \
--conf spark.comet.expression.allowIncompatible=true \
$DF_BENCH/runners/datafusion-comet/tpcbench.py \
--benchmark tpch \
Expand Down
115 changes: 115 additions & 0 deletions docs/source/contributor-guide/config_conventions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->

# Configuration Conventions

Comet configuration keys live under `spark.comet.*` and are declared in
`spark/src/main/scala/org/apache/comet/CometConf.scala`. This page describes the naming
conventions those keys follow so that new configs stay consistent with the surface users
already know, and documents the process for renaming an existing key without breaking
deployments.

## Key Naming

A Comet config key is a dotted path of segments. Each segment describes a scope, from
broadest (the prefix) to narrowest (the specific setting).

- **Prefix.** All keys start with `spark.comet.` — never bare `comet.` and never a nested
prefix like `spark.sql.comet.`.
- **Category segment.** The first segment after the prefix names a broad area:
`exec` (execution and expressions), `scan` (source readers), `parquet` (Parquet-specific
settings), `shuffle` (shuffle behavior), `columnar` (columnar shuffle), `explain` (plan
explain/logging), `metrics`, `tracing`, `testing`, `convert` (Spark → Comet source
conversion), or `expression` / `operator` (per-expression / per-operator flags).
- **Segment casing.** Multi-word segments use `camelCase`, not dot- or kebab-separated.
Prefer `spark.comet.foo.maxThreadNum` over `spark.comet.foo.max.thread.num` or
`spark.comet.foo.max-thread-num`.
- **Boolean suffix.** Descriptor-form boolean flags that gate a subsystem or feature end
in `.enabled` — for example `spark.comet.debug.enabled`, `spark.comet.metrics.enabled`,
`spark.comet.parquet.rowFilterPushdown.enabled`. Action-form flags whose name is itself
a verb (`force...`, `allow...`) can omit `.enabled` because the verb already encodes
the action — for example `spark.comet.exec.forceShuffledHashJoin`.
- **Acronyms.** Treat acronyms consistently as `UDF`, `SHJ`, `SMJ`, `IO` — either all-caps
or camelCase, but do not mix within one key (`pyarrowUdf` and `scalaUDF` in the same
cluster is a red flag).

## Symbol Naming (Scala)

Every config declares a Scala `val` in `CometConf.scala`. The symbol name is the
`UPPER_SNAKE_CASE` form of the key's meaningful segments, prefixed with `COMET_`.

Examples:

| Key | Symbol |
| ----------------------------------------------- | --------------------------- |
| `spark.comet.enabled` | `COMET_ENABLED` |
| `spark.comet.exec.forceShuffledHashJoin` | `COMET_FORCE_SHJ` |
| `spark.comet.parquet.rowFilterPushdown.enabled` | `COMET_ROW_FILTER_PUSHDOWN` |

The symbol name is what appears in code; the key is what appears in user configuration.
The two do not have to match segment-for-segment — brevity in the symbol is fine as long
as the key remains descriptive.

## Categories

Every `ConfigEntry` must call `.category(...)`. The category is used to route the key into
the right table in the user guide's `configs.md`. Available categories are declared as
`CATEGORY_*` constants at the top of `CometConf.scala`. If a new config does not fit an
existing category, discuss adding a new one before landing the config.

## Renaming an Existing Config

Configs under `spark.comet.*` are stable across minor releases: users may have set them in
production `spark-defaults.conf` files, Spark job submissions, or notebooks. Renaming a key
must not silently break those deployments.

Use the `withAlternative` builder on `ConfigBuilder` to keep the old key working as a
deprecated alias:

```scala
val COMET_FORCE_SHJ: ConfigEntry[Boolean] =
conf(s"$COMET_EXEC_CONFIG_PREFIX.forceShuffledHashJoin")
.withAlternative(s"$COMET_EXEC_CONFIG_PREFIX.replaceSortMergeJoin")
.category(CATEGORY_EXEC)
.doc("...")
.booleanConf
.createWithDefault(false)
```

Reading a value from the alternative logs a one-time deprecation warning per JVM per
alternative key, pointing users at the current key. The primary key always wins if both
are set.

`withAlternative` accepts multiple alternatives (checked in order), for keys that have
been renamed more than once.

The rename checklist for a single config:

1. Update the key string on the `conf(...)` call and add `.withAlternative(oldKey)`.
2. Rename the Scala `val` (for example `COMET_REPLACE_SMJ` → `COMET_FORCE_SHJ`).
3. Update every callsite of the Scala symbol — grep the whole repo.
4. Update every documented reference to the old key string — grep `docs/source/` for the
old key and update to the new key. The auto-generated `configs.md` table will refresh
on the next release-docs regeneration and does not need manual editing.
5. Add or update a test in `CometConfSuite` if the rename covers a new type of alias
pattern (single-hop, multiple alternatives, etc.).

Removing a deprecated alias is a follow-up step that belongs to a later major release —
typically the next Comet major after the rename first ships. See the
[versioning policy](../about/versioning_policy.md) for the timing rules.
1 change: 1 addition & 0 deletions docs/source/contributor-guide/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ Tracing <tracing>

Expression Audits <expression-audits/index>
Supported Spark Configurations <spark_configs_support>
Configuration Conventions <config_conventions>
```

```{toctree}
Expand Down
2 changes: 1 addition & 1 deletion docs/source/user-guide/latest/tuning.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ to configure Comet to convert `SortMergeJoin` to `ShuffledHashJoin`. Comet does
`ShuffledHashJoin` so this could result in OOM. Also, `SortMergeJoin` may still be faster in some cases. It is best
to test with both for your specific workloads.

To configure Comet to convert `SortMergeJoin` to `ShuffledHashJoin`, set `spark.comet.exec.replaceSortMergeJoin=true`.
To configure Comet to convert `SortMergeJoin` to `ShuffledHashJoin`, set `spark.comet.exec.forceShuffledHashJoin=true`.

## Shuffle

Expand Down
84 changes: 72 additions & 12 deletions spark/src/main/scala/org/apache/comet/CometConf.scala
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,9 @@ object CometConf extends ShimCometConf {
.booleanConf
.createWithDefault(false)

val COMET_REPLACE_SMJ: ConfigEntry[Boolean] =
conf(s"$COMET_EXEC_CONFIG_PREFIX.replaceSortMergeJoin")
val COMET_FORCE_SHJ: ConfigEntry[Boolean] =
conf(s"$COMET_EXEC_CONFIG_PREFIX.forceShuffledHashJoin")
.withAlternative(s"$COMET_EXEC_CONFIG_PREFIX.replaceSortMergeJoin")
Comment on lines +375 to +377

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.

This is an example of renaming a config and preserving the old deprecated name

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.

Should we mention that replaceSortMergeJoin is deprecated somewhere?

.category(CATEGORY_EXEC)
.doc("Experimental feature to force Spark to replace SortMergeJoin with ShuffledHashJoin " +
s"for improved performance. This feature is not stable yet. $TUNING_GUIDE.")
Expand Down Expand Up @@ -1072,7 +1073,8 @@ private class TypedConfigBuilder[T](
parent._doc,
parent._category,
parent._public,
parent._version)
parent._version,
parent._alternatives)
CometConf.register(conf)
conf
}
Expand All @@ -1088,7 +1090,9 @@ private class TypedConfigBuilder[T](
parent._doc,
parent._category,
parent._public,
parent._version)
parent._version,
None,
parent._alternatives)
CometConf.register(conf)
conf
}
Expand Down Expand Up @@ -1118,7 +1122,8 @@ private class TypedConfigBuilder[T](
parent._category,
parent._public,
parent._version,
Some(envVar))
Some(envVar),
parent._alternatives)
CometConf.register(conf)
conf
}
Expand All @@ -1131,7 +1136,8 @@ abstract class ConfigEntry[T](
val doc: String,
val category: String,
val isPublic: Boolean,
val version: String) {
val version: String,
val alternatives: Seq[String] = Nil) {

/**
* Retrieves the config value from the given [[SQLConf]].
Expand Down Expand Up @@ -1169,16 +1175,25 @@ private[comet] class ConfigEntryWithDefault[T](
category: String,
isPublic: Boolean,
version: String,
_envVar: Option[String] = None)
extends ConfigEntry(key, valueConverter, stringConverter, doc, category, isPublic, version) {
_envVar: Option[String] = None,
_alternatives: Seq[String] = Nil)
extends ConfigEntry(
key,
valueConverter,
stringConverter,
doc,
category,
isPublic,
version,
_alternatives) {
override def defaultValue: Option[T] = Some(_defaultValue)

override def defaultValueString: String = stringConverter(_defaultValue)

override def envVar: Option[String] = _envVar

def get(conf: SQLConf): T = {
val tmp = conf.getConfString(key, null)
val tmp = CometConfDeprecations.readWithAlternatives(conf, key, alternatives)
if (tmp == null) {
_defaultValue
} else {
Expand All @@ -1194,20 +1209,23 @@ private[comet] class OptionalConfigEntry[T](
doc: String,
category: String,
isPublic: Boolean,
version: String)
version: String,
_alternatives: Seq[String] = Nil)
extends ConfigEntry[Option[T]](
key,
s => Some(rawValueConverter(s)),
v => v.map(rawStringConverter).orNull,
doc,
category,
isPublic,
version) {
version,
_alternatives) {

override def defaultValueString: String = ConfigEntry.UNDEFINED

override def get(conf: SQLConf): Option[T] = {
Option(conf.getConfString(key, null)).map(rawValueConverter)
Option(CometConfDeprecations.readWithAlternatives(conf, key, alternatives))
.map(rawValueConverter)
}
}

Expand All @@ -1219,12 +1237,23 @@ private[comet] case class ConfigBuilder(key: String) {
var _doc = ""
var _version = ""
var _category = ""
var _alternatives: Seq[String] = Nil

def internal(): ConfigBuilder = {
_public = false
this
}

/**
* Registers deprecated config keys that Comet will read as fall-backs when the primary `key` is
* unset. Reading a value from an alternative logs a one-time deprecation warning pointing users
* at the current `key`. Alternatives are checked in the order provided.
*/
def withAlternative(alt: String, more: String*): ConfigBuilder = {
_alternatives = alt +: more
this
}

def doc(s: String): ConfigBuilder = {
_doc = s
this
Expand Down Expand Up @@ -1272,3 +1301,34 @@ private[comet] case class ConfigBuilder(key: String) {
private object ConfigEntry {
val UNDEFINED = "<undefined>"
}

private object CometConfDeprecations extends org.apache.spark.internal.Logging {

private val warned = java.util.concurrent.ConcurrentHashMap.newKeySet[String]()

/**
* Reads the config value for `key`, falling back to each key in `alternatives` (in order) when
* the primary key is unset. When the value comes from an alternative, logs a deprecation
* warning once per JVM per alternative key.
*
* @return
* the resolved string value, or `null` if neither the primary key nor any alternative is set.
*/
def readWithAlternatives(conf: SQLConf, key: String, alternatives: Seq[String]): String = {
val primary = conf.getConfString(key, null)
if (primary != null) return primary
if (alternatives.isEmpty) return null
val it = alternatives.iterator
while (it.hasNext) {
val alt = it.next()
val v = conf.getConfString(alt, null)
if (v != null) {
if (warned.add(alt)) {
logWarning(s"Comet configuration '$alt' is deprecated; use '$key' instead.")
}
return v
}
}
null
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ case class CometExecRule(session: SparkSession)
} else {
val normalizedPlan = normalizePlan(plan)

val planWithJoinRewritten = if (CometConf.COMET_REPLACE_SMJ.get()) {
val planWithJoinRewritten = if (CometConf.COMET_FORCE_SHJ.get()) {
normalizedPlan.transformUp { case p =>
RewriteJoin.rewrite(p)
}
Expand Down
Loading
Loading