[GLUTEN-12694][CORE] Replace string-based native conf key lists with a declarative config API - #12549
Open
jackylee-ch wants to merge 4 commits into
Open
[GLUTEN-12694][CORE] Replace string-based native conf key lists with a declarative config API#12549jackylee-ch wants to merge 4 commits into
jackylee-ch wants to merge 4 commits into
Conversation
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
changed the title
|
Run Gluten Clickhouse CI on x86 |
2 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
2 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
July 27, 2026 11:08
0fb8466 to
527243f
Compare
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
July 29, 2026 06:01
527243f to
20e6481
Compare
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
August 3, 2026 08:54
20e6481 to
6e460de
Compare
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
August 3, 2026 11:01
6e460de to
6e84033
Compare
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
August 3, 2026 13:07
6e84033 to
023d94b
Compare
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
August 4, 2026 04:37
023d94b to
f788eca
Compare
jackylee-ch
changed the title
|
Run Gluten Clickhouse CI on x86 |
jackylee-ch
changed the title
jackylee-ch
marked this pull request as ready for review
August 4, 2026 09:29
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
August 6, 2026 03:04
19bad78 to
c34fdef
Compare
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (1)
gluten-substrait/src/main/scala/org/apache/spark/shuffle/GlutenShuffleUtils.scala:55
getCompressionCodecbuilds its provider withnew SQLConfProvider(SQLConf.get), which can bypass the active SparkSession’ssessionState.conf(Gluten already works around this inGlutenCoreConfig.activeSQLConf). That risks ignoring session-scoped overrides (spark.conf.set) and reading only thread-local/global SQLConf.
Consider resolving the SQLConf the same way as GlutenCoreConfig.activeSQLConf (prefer active session when present), then chaining to SparkConf.
val provider =
new ChainedProvider(new SQLConfProvider(SQLConf.get), new SparkConfProvider(conf))
val codecEntry = GlutenConfig.COLUMNAR_SHUFFLE_CODEC
zhztheplayer
reviewed
Aug 6, 2026
Comment on lines
+534
to
+535
| .passToNative() | ||
| .passDefault() |
Member
There was a problem hiding this comment.
Just curious, are there any issues if we always pass the default?
Contributor
Author
There was a problem hiding this comment.
noop. This mechanism should only apply to cases where a default value needs to be generated and passed if the user hasn't specified one, such as spark.sql.session.timeZone. I’ll recheck the code and remove the unnecessary passDefault instances.
Member
There was a problem hiding this comment.
+1 to remove passDefault if it is not necessary to use it.
On the C++ side, I think the code that consumes these configurations could assume that every required configuration has been passed from Scala with its effective value already resolved. If a required configuration is missing, the native code could fail explicitly. If this makes sense, it seems unnecessary to declare a separate native-side default value in C++ (though this is not the scope of this PR).
Contributor
Author
There was a problem hiding this comment.
That’s a good idea. The passDefault mechanism exists primarily for the following three reasons:
- There is no fallback on the C++ side; failing to pass the value results in a key lookup failure;
- The default values differ between the C++ and Java sides;
- Cases like timezones, which need to be generated and passed from the Spark side.
Unified configuration can resolve the first two cases, but the third is the key challenge; I’ll try addressing it usingregisterConf.
philo-he
reviewed
Aug 7, 2026
philo-he
left a comment
philo-he
left a comment
Member
There was a problem hiding this comment.
Thanks for the PR. Just some quick comments. Please check whether they make sense.
|
|
||
| /** | ||
| * A config entry whose default value is computed on each read rather than fixed at declaration, | ||
| * mirroring Spark's `createWithDefaultFunction`. Use it when the default depends on JVM or session |
Member
There was a problem hiding this comment.
createWithDefaultFunction -> ConfigEntryWithDefaultFunction?
| * e.g. a time zone conf defaulting to the current JVM default time zone. Combined with | ||
| * `passDefault`, native receives the value resolved at delivery time. | ||
| */ | ||
| def createWithDefaultFunction(defaultFunc: () => T): ConfigEntry[T] = { |
Member
There was a problem hiding this comment.
Is the session time zone currently the only special case? If so, could we remove this method from this PR and keep the scope of this PR more focused.
BTW, Spark session timezone default value should be handled by Spark config. Do we also need the default function in Gluten?
Comment on lines
+534
to
+535
| .passToNative() | ||
| .passDefault() |
Member
There was a problem hiding this comment.
+1 to remove passDefault if it is not necessary to use it.
On the C++ side, I think the code that consumes these configurations could assume that every required configuration has been passed from Scala with its effective value already resolved. If a required configuration is missing, the native code could fail explicitly. If this makes sense, it seems unnecessary to declare a separate native-side default value in C++ (though this is not the scope of this PR).
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (1)
gluten-core/src/test/scala/org/apache/gluten/config/NativeConfRegistrySuite.scala:31
NativeConfRegistry.selectBackendConflatchesbackendConfDelivered=true. SincewithRegisteredKeysdoesn't reset it, any subsequent registrations in this suite will emit the "declared after backend conf had already been delivered" warning, which is misleading noise and can make the test output flaky if warnings are asserted elsewhere. Reset the latch as part of the test cleanup (and optionally before runningf) so each test starts from a clean registry state.
private def withRegisteredKeys(keys: String*)(f: => Unit): Unit = {
try f
finally keys.foreach(NativeConfRegistry.unregister)
}
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/developers/NativeConfPassing.md:163
- This doc says
createOptionalon Spark/Hadoop-owned keys resolves and delivers the owner's default, but the implementation only resolves foreign defaults for the nullarycreateWithDefault()path (see ConfigBuilder.markDeliverForeignDefault + GlutenConfigUtil.resolveForeignDeclaredDefault). WithcreateOptional, no default is delivered when unset, so this section is misleading.
`createOptional` here does **not** mean "deliver only when set". For a Spark-owned key it means "the
default is Spark's", and Gluten resolves it from Spark's own entry at delivery time — both `SQLConf`
entries and Spark core ones. Nothing is restated on the Gluten side, so the two cannot drift across
Spark versions; `spark.sql.ansi.enabled` alone changed default in Spark 4.0, and Spark's default for
`spark.sql.session.timeZone` is the current JVM default time zone, which a session (or a test) may
gluten-core/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala:183
convertForNativecurrently swallowsIllegalArgumentExceptionfrom the entry's valueConverter and delivers the raw string to native. For numeric native readers (e.g.spark.shuffle.file.buffernow parsed withstd::stoll), this can turn an invalid JVM-side value into a native crash or a silently mis-parsed value. It's safer to fail fast with a clear exception rather than delivering an unchecked raw value to native.
} catch {
// A value Spark or Hadoop would reject is not this mechanism's business to validate: Spark or
// Hadoop raises on it at its own read site, with its own message. Deliver it unchanged rather
// than failing conf selection, which runs per task.
case _: IllegalArgumentException => raw
…a declarative config API Fixes apache#12694. Which configurations get delivered from JVM to native side used to be decided by hard-coded string lists inside GlutenConfig.getNativeSessionConf / getNativeBackendConf: a `nativeKeys` set of 40+ raw keys, two "configs having default values" Seqs duplicating each conf's key and default, and per-key special cases inline in the selection methods. Adding a native conf meant editing central lists far from the conf's definition, defaults could drift, and backend-specific keys (Velox S3 keys) lived in common code. This replaces those lists with a declaration made at each conf's definition. `ConfigRegistry` offers four declaration methods, split along two axes - who owns the key, and whether it is modifiable: | modifiable at any time | set at backend init, then immutable -----------------|------------------------|------------------------------------ owned by Gluten | buildConf | buildStaticConf owned by Spark | registerConf | registerStaticConf `ConfigBuilder.passToNative()` marks a conf for native delivery. There is no scope argument: the channel follows the conf's mutability, which the declaration method already states. - modifiable -> delivered both at native backend initialization and on each native runtime creation, so native observes the current value wherever it reads the key; - static -> delivered once at native backend initialization, which is lossless since the value cannot change afterwards. Two further markers: `passDefault()` also delivers the conf's own default (in parsed form, e.g. a "64MB" bytes conf as "67108864") when the user did not set it, for keys native relies on being present; `nativeTransform(fn)` normalizes a user-set value before delivery, replacing the old inline byte-unit and upper-casing special cases. A default is re-resolved on each delivery rather than snapshotted at declaration, so `createWithDefaultFunction` - mirroring Spark's method of the same name - can express a default that follows JVM or session state. This matters for a conf that mirrors a Spark default which is itself dynamic: `spark.sql.session.timeZone` resolves to the current JVM default time zone, so reading it once at declaration time would pin whatever the zone happened to be while the conf object initialized, and any later `TimeZone.setDefault` would leave native reading a stale zone. `registerConf` / `registerStaticConf` exist because Spark- and Hadoop-owned keys must not get a Gluten `ConfigEntry` - their owner already registered them, and registering again conflicts with `SQLConf`. They declare only the native delivery, so `passToNative()` is mandatory for them. `NativeConfRegistry` remains the mechanism underneath but is no longer an API: `register` is `private[config]` and called only by `ConfigBuilder`. Where a Gluten conf is an override of a Spark one, the relationship is now declared rather than hand-written at the read site: val COLUMNAR_SHUFFLE_CODEC = buildConf("spark.gluten.sql.columnar.shuffle.codec") .stringConf .transform(_.toLowerCase(Locale.ROOT)) .passToNative() .fallbackConf(SPARK_IO_COMPRESSION_CODEC, SPARK_IO_COMPRESSION_CODEC_DEFAULT) Reading the entry yields the Gluten value if set, else the Spark value if set, else Spark's default, so `GlutenShuffleUtils.getCompressionCodec` no longer carries a `None` branch. Its two error messages are preserved, because `readWithSource` returns the value together with whether it came from the Gluten key - a value explicitly set on the Gluten conf is validated against the codec backend in use, while one inherited from Spark is validated against what the backend supports and the error points at how to override it. Taking both from one read means the value and its origin cannot disagree. The fallback is stated by key and default value rather than as Spark's `ConfigEntry`, which is `private[spark]` and cannot appear in a signature outside `org.apache.spark`. A conf object is a Scala object, so declaring one is not enough - its registrations only happen once something touches it. `Component.confs(): Seq[ConfigRegistry]` lets a component declare its conf objects; Gluten initializes them right after component discovery, before any component's onDriverStart / onExecutorStart. This is the only entry point early enough for the backend channel: backends are root nodes of the component DAG, so a backend's onDriverStart - where native backend init happens - runs before any dependent component's. Only runtime-compatible components are visited, so an excluded component's confs never reach native side. VeloxBackend and CHBackend declare VeloxConfig / CHConfig through the hook. `ConfigRegistry.ensureRegistered()` is the supported way to force a conf object's initialization; a reference to a constant val can be constant-folded away by the compiler, which would silently drop a module's registrations. `ConfigRegistry.get` now has a default implementation so a conf object that only declares configurations does not have to implement an accessor it never uses. The whole chain works from outside the org.apache.gluten package - verified by compiling a third-party component and conf object against gluten-core. Confs whose declared mutability did not match where native consumes them were fixed: - spark.gluten.sql.columnar.backend.velox.cudf.enableTableScan: static -> modifiable. Read per query from the runtime conf (WholeStageResultIterator, SubstraitToVeloxPlan, VeloxRuntime), not at backend init. - spark.gluten.memoryOverhead.size.in.bytes: modifiable -> static. Set on SparkConf at driver start and consumed by Velox backend init only. - spark.gluten.velox.s3UseProxyFromEnv / s3PayloadSigningPolicy: promoted from raw string registrations to declarations in VeloxConfig. awsSdkLogLevel, s3UseProxyFromEnv and s3PayloadSigningPolicy stay modifiable rather than becoming static, because native createHiveConnectorConfig is not backend-init-only - it also runs per write on the runtime conf map (VeloxParquetDataSourceS3::initSink, IcebergWriter), so making them static would remove working session-level control. Three confs stay modifiable although native reads two of them at backend init only: GlutenAutoAdjustStageResourceProfile rewrites spark.gluten.numTaskSlotsPerExecutor, spark.gluten.memory.offHeap.size.in.bytes and spark.gluten.memory.task.offHeap.size.in.bytes on SQLConf per stage, and JVM-side readers observe the rewritten values. (SQLConf.setConfString does not itself reject static keys - that guard lives in RuntimeConfig.set and SET - but declaring these static would assert an immutability that does not hold.) - The nativeKeys set, the two default-value Seqs, and all per-key special cases in the two selection methods. Both now do: registry selection + existing prefix rules + UGI tokens (session only). - BackendSettingsApi.extraNativeSessionConfKeys / extraNativeBackendConfKeys. Added shortly before this change and never overridden by any backend; the declaration API supersedes them and, unlike a Set[String], can express per-channel scope, defaults and normalization. An out-of-tree backend migrates by declaring its conf object through Component.confs(). - GlutenConfigUtil.mapByteConfValue, superseded by nativeTransform. - spark.gluten.velox.fs.s3a.retry.mode, which has had no native reader since apache#8123 moved the S3 config path to velox's S3Config: native reads the retry mode from spark.hadoop.fs.s3a.retry.mode. - The hand-written codec fallback in GlutenShuffleUtils, now declared. Prefix rules (spark.gluten.sql.columnar.backend.<backend>, spark.hadoop.fs.s3a. etc.) are kept as-is: they are pattern rules covering open-ended key families rather than enumerable declarations. The selected key/value results are otherwise identical to before, including every default value and value normalization. Four intentional differences: 1. A modifiable conf now reaches both channels, where the old lists often put it on one. No key regresses; some keys are newly present in the other channel, which native ignores when it has no reader there. 2. spark.sql.legacy.timeParserPolicy is upper-cased on both channels rather than only the session one. Velox compares against "LEGACY" and ClickHouse lower-cases the value itself, so both are correct; the previous asymmetry was a latent inconsistency. 3. The spark.hadoop.fs.s3a.* connection confs now carry Gluten's declared default on both channels. Previously the write path fell back to ConfigExtractor's own default, which disagrees for path.style.access (false vs true). 4. spark.gluten.sql.columnar.shuffle.codec's generated documentation now shows lz4 rather than <undefined>, since the fallback makes Spark's default the effective one. - NativeConfRegistrySuite (gluten-core): the declaration API - which channel each of the four methods delivers on, passDefault in parsed form and its constraint checks, re-resolution of a dynamic default on each delivery, nativeTransform, Spark fallback resolution, duplicate declaration rejection. - NativeConfPassingSuite (gluten-substrait, new): the delivered result end to end - what getNativeSessionConf / getNativeBackendConf actually select, including byte-string normalization (spark.shuffle.file.buffer=32k -> "32768"), per-channel scoping, always-present defaults and prefix rules, and that the delivered spark.sql.session.timeZone default tracks TimeZone.setDefault. This restores the byte-string coverage lost with GlutenConfigUtilSuite. - ShuffleCodecConfSuite (gluten-substrait, new): the codec fallback's resolution and reported origin. - ComponentSuite (gluten-core): Component.confs() defaults to empty and is overridable, and initializing a component's conf objects registers their native confs into the expected channels. - Both generated config doc golden files regenerated; the only diff is the codec default noted above. - gluten-core, gluten-substrait, backends-velox compile clean with -Pspark-3.5,backends-velox (scalastyle / spotless / scalafmt clean). backends-clickhouse cannot be compiled locally due to a pre-existing breakage unrelated to this change (src-delta33 needs Scala 2.13; LazyAggregateExpandRule predates ColumnarShuffleExchangeExec gaining mapperStageMode on main), so its coverage relies on the ClickHouse CI. The three MiscOperatorSuite codec tests need a native library that cannot be built on this machine; ShuffleCodecConfSuite covers the resolution they depend on, but their validation path is only exercised by CI.
… opting in per conf
`passDefault()` made "also deliver my default when the user did not set this
conf" an opt-in marker, and each Spark-owned conf that wanted it had to restate
Spark's default via `createWithDefaultFunction(() => SQLConf.X.defaultValueString)`.
That is backwards: delivering the declared default is the normal case, and
restating a Spark default is exactly what can drift across versions.
`passToNative()` now covers both: a user-set value is delivered as is, and an
unset key is delivered with its declared default, resolved per delivery.
Where the default comes from follows who owns the key:
- a Gluten conf (`buildConf`) uses its own `createWithDefault*`;
- a Spark conf (`registerConf`) declares nothing and is resolved from Spark's
own entry - `SQLConf` for `spark.sql.*`, Spark core for `spark.shuffle.*` and
friends - so the two cannot drift. `spark.sql.ansi.enabled` alone changed
default in Spark 4.0;
- a Hadoop key that no Spark entry declares resolves to nothing and is
delivered only when set, which is what the S3 credential keys need since
native branches on whether they are present at all.
Resolution stays per delivery rather than once at declaration because a Spark
default may be dynamic: `spark.sql.session.timeZone` is the current JVM default
time zone. `nativeTransform` now runs over a resolved default too, since a
default may be a raw string - Spark declares "32k" for
`spark.shuffle.file.buffer`.
`createOptional` becomes the way to say "deliver only when set", for a key whose
value native decides. Three confs move to it, because their previous defaults
were placeholders rather than values:
- `spark.gluten.numTaskSlotsPerExecutor` ("-1"): computed by `GlutenPlugin`
from the actual task slots; native rejects a negative value
(`GLUTEN_CHECK(numTaskSlotsPerExecutor >= 0)`) and already warns and falls
back to 1 when the key is absent.
- `spark.gluten.saveDir` (""): `VeloxRuntime::enableDumping` checks the key is
present, not that it is non-empty, so an empty default would let it proceed
with an empty dump path.
- A `fallbackConf` entry reports its target's default as its own, so
delivering it under this key would contradict the user when only the target
is set. The target is delivered under its own key.
`spark.gluten.memory{,.task}.offHeap.size.in.bytes` and
`spark.gluten.memoryOverhead.size.in.bytes` keep their "0" placeholder for now:
making them optional means deciding what their JVM-side accessors return, which
is left to a follow-up.
Behavior change worth noting for review: about a dozen Spark keys that used to
reach native only when set now arrive with Spark's default. Each was checked
against its native read site; none of them branches on key presence, and where
both sides declare a default the two already agree (the four
`spark.sql.optimizer.runtime.bloomFilter.*` keys match Velox's
`SparkQueryConfig` exactly), so the JVM simply becomes the single source.
…very
Five review passes over the two preceding commits found that making "deliver the
declared default" unconditional exposed placeholder defaults that the previous
opt-in marker had kept away from native. Fixes, in order of severity:
1. Three memory confs shipped a placeholder `0`. Under the new rule they were
delivered on every backend init, and native reads all three as "absent means
unbounded":
- `spark.gluten.memoryOverhead.size.in.bytes` -> `createOptional`. It has no
JVM reader at all, only `VeloxListenerApi`'s set, so nothing else changes.
`VeloxBackend::init` took `sparkOverhead.value() * ratio` == 0 for the
Velox global memory manager instead of its `kMaxMemory` branch, which the
delivery had turned into dead code.
- `spark.gluten.memory.task.offHeap.size.in.bytes` -> `createOptional`, with
the accessor returning `Option` and `getOrElse(0)` at its single caller so
JVM behavior is unchanged. `WholeStageResultIterator` fell back to
`kMaxMemory` before; `0` collapsed the partial-aggregation limits to their
16MB/64MB floors. Two ClickHouse `contains()` checks were likewise turned
always-true.
- `spark.gluten.memory.offHeap.size.in.bytes` -> drop `passToNative()`.
Native declares `kSparkOffHeapMemory` but reads it nowhere; ClickHouse
reads it JVM-side off the conf map.
Reachable in-tree: `MockVeloxBackend` calls `onExecutorStart` without
`onDriverStart`, and `onDriverStart` is the only setter for the overhead.
2. `resolveSparkDeclaredDefault` filtered the `<undefined>` sentinel but not
`FallbackConfigEntry`'s, whose `defaultValueString` is the literal
`"<value of other.key>"`. It now discriminates on `defaultValue.isDefined`,
which is `None` for exactly the two subclasses that have no real default. No
currently registered key is backed by a fallback entry, but Spark converts
plain entries into fallback ones across releases - `spark.shuffle.file.buffer`
gained two such neighbours in 4.0 - and a sentinel reaching a key with a
`nativeTransform` would throw on every task, not merely mis-configure.
3. `NativeConfPassingSuite` asserted Spark's `timeParserPolicy` default as the
literal `"EXCEPTION"`, which is `CORRECTED` from Spark 4.0 on, so the suite
failed on two of the five supported profiles. It now reads the expectation
from Spark's entry - the same rule this change applies to production code.
The neighbouring assertions were converted likewise.
4. `docs/Configuration.md` was not regenerated after
`spark.gluten.numTaskSlotsPerExecutor` became `createOptional`, so the
`AllGlutenConfiguration` golden test failed. Regenerated via
`dev/gen-all-config-docs.sh`.
5. `NativeConfRegistry` had no notion of the backend channel being closed: a conf
object initialized after `getNativeBackendConf` reached the runtime channel
only, silently. The channel is now latched on first delivery and a later
declaration is logged, naming the key and pointing at `Component.confs()`.
Reachable today via `VeloxDeltaConfig`, which no component declares and which
`OffloadDeltaCommand` initializes at planning time.
Also drops `NativeScope`: `registerToNative` only ever produced BACKEND or ALL,
so the enum's third value was unreachable and its scaladoc advertised per-scope
semantics the API cannot express. `register` now takes `isStatic` directly.
Deferred items - pre-existing defects and separate behavior changes - are
recorded in TODO-native-conf-followups.md rather than folded into a refactor
meant to be behavior-preserving. The two that matter most: two memory confs
native reads but the JVM has never delivered (`memory.manager.capacity.ratio`,
`memory.reservationBlockSize`), and `registerConf` silently ignoring `.doc()` /
`.internal()` / `.withAlternative()`.
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
August 13, 2026 13:22
2969bed to
5290091
Compare
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (3)
gluten-substrait/src/test/scala/org/apache/gluten/config/NativeConfPassingSuite.scala:131
- This test expects several Spark-owned keys (e.g. CASE_SENSITIVE, IGNORE_MISSING_FILES, DECIMAL_OPERATIONS_ALLOW_PREC_LOSS) to be present in getNativeSessionConf() even when unset, but those keys are declared via registerConf(...).createOptional in GlutenConfig.registerNativeConfs(), which means NativeConfRegistry will not deliver any value when the key is absent. As written, sessionConf()(e.key) will throw NoSuchElementException for those entries. Align the test with the current createOptional contract by asserting absence for createOptional foreign keys and only asserting defaults for keys declared with createWithDefault().
gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala:470 - SPARK_IO_COMPRESSION_CODEC_DEFAULT is hard-coded as "lz4" even though the intent of this PR is to avoid duplicating owner-declared defaults. Since
spark.io.compression.codecis a Spark core config, you can reference Spark's own declared key/default instead of restating them here, reducing drift risk across Spark versions.
docs/developers/NativeConfPassing.md:166 - This section’s semantics and example code contradict the implementation:
- The snippet shows
SQLConf.ANSI_ENABLEDusingcreateOptional, but GlutenConfig.registerNativeConfs() declares it with the nullarycreateWithDefault(). - The paragraph states that
createOptionalresolves and delivers Spark's default, but ConfigBuilder.declaredDefault treatscreateOptionalas "deliver nothing when unset"; resolving Spark/Hadoop defaults happens only for the nullarycreateWithDefault()path.
Please update this section to match the actual behavior and the current recommended declarations.
`createOptional` here does **not** mean "deliver only when set". For a Spark-owned key it means "the
default is Spark's", and Gluten resolves it from Spark's own entry at delivery time - both `SQLConf`
entries and Spark core ones. Nothing is restated on the Gluten side, so the two cannot drift across
Spark versions; `spark.sql.ansi.enabled` alone changed default in Spark 4.0, and Spark's default for
`spark.sql.session.timeZone` is the current JVM default time zone, which a session (or a test) may
…ntry's own converter
Value normalization at delivery (nativeTransform) duplicated what a conf's
own value converter already states at declaration - stringConf.transform(...)
or bytesConf(unit). NativeConfRegistry.select now runs every delivered value
(user-set or defaulted) through the entry's converter directly, so the 4
sites that used nativeTransform are declared identically to how Spark itself
declares them:
- spark.sql.legacy.timeParserPolicy: stringConf.transform(toUpperCase),
matching Spark's own entry
- spark.unsafe.sorter.spill.reader.buffer.size,
spark.shuffle.spill.diskWriteBufferSize: bytesConf(BYTE)
- spark.shuffle.file.buffer: bytesConf(KiB), matching Spark's KiB unit;
native now multiplies by 1024 itself rather than the JVM guessing
native's unit
spark.shuffle.file.buffer moves from a JNI argument to the conf map as part
of this: the JNI arg carried the raw KiB count into a native field that
reads it as bytes, silently 1024x smaller than native's own default
whenever the argument was passed. Native now reads the key from its conf
map and converts to bytes itself.
Also reworks default-value delivery for foreign (registerConf) keys from
implicit two-state behavior into an explicit three-way choice stated by the
terminal method:
- createOptional: never deliver a default; native's own fallback decides.
Previously this silently resolved Spark's declared default when native's
fallback happened to differ, which is wrong for absence-branching keys
(S3 credentials) and adds an unstated dependency for the 34 keys where
native's fallback already matches Spark's default.
- createWithDefault() (new, foreign-only): explicitly deliver the value
Spark declares for the key, re-resolved at each delivery. Used for the
3 keys where native's own fallback is wrong - mapKeyDedupPolicy (native
defaults to non-throwing, Spark to EXCEPTION), ansi.enabled (native
fallback is stale for Spark 4.0+), session.timeZone (native has no time
zone concept at all).
- createWithDefault(value): Gluten's own chosen value, unchanged - the 6
keys where Gluten deliberately departs from both Spark and native.
Renames the Spark-specific vocabulary this introduced to reflect that the
mechanism applies to any foreign owner (Spark or Hadoop), not Spark alone:
resolveSparkDeclaredDefault -> resolveForeignDeclaredDefault,
ConfigEntrySparkFallback -> ConfigEntryForeignFallback.
jackylee-ch
force-pushed
the
config-pass-to-native
branch
from
August 13, 2026 13:38
5290091 to
2dea4ba
Compare
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (3)
gluten-substrait/src/test/scala/org/apache/gluten/config/NativeConfPassingSuite.scala:131
- This test currently assumes many foreign Spark/Spark-core keys are present in
getNativeSessionConfeven when the user did not set them. However,registerConf(...).createOptionaldeclares that nothing is delivered when unset (native fallback applies), andNativeConfRegistrySuiteexplicitly asserts that behavior. As written,sessionConf()(e.key)will throw for keys likeCASE_SENSITIVEandIGNORE_MISSING_FILESwhen they’re unset, making the suite inconsistent with the registry contract.
docs/developers/NativeConfPassing.md:163 - This documentation contradicts the implemented API semantics:
createOptionalon a foreign key does not resolve/deliver the owner-declared default (it delivers nothing when unset). Delivering the owner default is done via the nullarycreateWithDefault(). The current text/example will mislead contributors and conflicts withNativeConfRegistrySuiteexpectations.
```scala
registerConf(SQLConf.ANSI_ENABLED.key).stringConf.passToNative().createOptional
registerConf(SPARK_S3_PATH_STYLE_ACCESS)
.doc("Read by the native S3 file system.")
.booleanConf
cpp/core/jni/JniWrapper.cc:854
std::stoll(it->second)can throw (e.g. if an invalid value reaches native). The JVM-sideconvertForNativeintentionally falls back to delivering the raw string onIllegalArgumentException, so a malformedspark.shuffle.file.buffercould make native terminate here instead of safely using the default.
auto shuffleFileBufferSize = kDefaultShuffleFileBufferSize;
auto& conf = ctx->getConfMap();
if (auto it = conf.find(kShuffleFileBufferSize); it != conf.end()) {
shuffleFileBufferSize = std::stoll(it->second) * 1024;
}
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.
What changes were proposed in this pull request?
Move the "this conf is passed to native" declaration next to the conf's own definition, so the fact lives in one place instead of being duplicated in central hardcoded lists in
GlutenConfig(previously 4 lists, 95 entries — all removed).Core API — four declaration methods, split by owner × mutability:
buildConf(...).passToNative()...buildStaticConf(...).passToNative()...registerConf(...).passToNative()...registerStaticConf(...).passToNative()...Which delivery channel a conf lands on is derived from its mutability (static → backend init only; modifiable → both channels), so
passToNative()takes no arguments.For
registerConf/registerStaticConf(foreign keys), the terminal method chooses whether and how to deliver a default when the user did not set it — this is the only decision the caller needs to make about defaults:createOptionalcreateWithDefault()createWithDefault(value)Full design in
docs/developers/NativeConfPassing.md.Behavior changes vs base
mapKeyDedupPolicy,ansi.enabled,session.timeZonespark.shuffle.file.buffercreateOptionalforeign keys (native fallback matches Spark's default, verified per key)Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-sonnet-4-20250514
Related issue: #12694