jackylee-ch commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3949863972
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -531,6 +531,8 @@ object VeloxConfig extends ConfigRegistry {
val COLUMNAR_VELOX_FILE_HANDLE_CACHE_ENABLED =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.fileHandleCacheEnabled")
+ .passToNative()
+ .passDefault()
Review Comment:
Closing the loop on this: `passDefault` is gone entirely, and with it the
question of whether to always pass the default.
What replaced it is the terminal method the conf already had to pick, so
there is no extra opt-in:
| Terminal | Unset key delivers |
|---|---|
| `createOptional` | nothing — native's own fallback applies |
| `createWithDefault(v)` | `v` |
| `createWithDefaultFunction(f)` | `f()`, re-evaluated per delivery |
To your original question — "are there any issues if we always pass the
default" — yes, for a handful of keys, and that is what forced the three-way
split rather than a blanket "always pass":
- `spark.gluten.numTaskSlotsPerExecutor` and
`spark.gluten.memory.task.offHeap.size.in.bytes` cannot be derived without a
`SparkConf` at hand, so their declared defaults were placeholders (`-1`, `0`).
Native rejects the first outright (`GLUTEN_CHECK(numTaskSlotsPerExecutor >=
0)`) and reads absence of the second as *unbounded* —
`WholeStageResultIterator.cc:553` falls back to `kMaxMemory`. Always passing
the placeholder would turn "unbounded" into "zero".
- `spark.gluten.saveDir`: `enableDumping` checks only whether the key is
**present**, so passing `""` enables dumping to an empty path.
- The S3 credential keys: native reads an absent key as "no credentials were
configured" and branches on that, so an empty default is not the same as
nothing.
For the majority the answer is your instinct, though: 41 of the 66
declarations are `createOptional`, i.e. deliver nothing and let native's own
fallback stand, because that fallback already matches what Spark or Hadoop
would apply. Only 21 declare a value of their own, and each of those is a case
where Gluten deliberately departs from both its owner and native (e.g.
`fs.s3a.path.style.access` `true` vs native's `false`).
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -845,7 +862,8 @@ object VeloxConfig extends ConfigRegistry {
.createWithDefault(50)
val CUDF_ENABLE_TABLE_SCAN =
-
buildStaticConf("spark.gluten.sql.columnar.backend.velox.cudf.enableTableScan")
Review Comment:
Confirmed with the read sites, and it is the reverse of static: native reads
`cudf.enableTableScan` from the **runtime** conf at three places and nowhere at
backend init.
- `cpp/velox/substrait/SubstraitToVeloxPlan.cc:1625`
- `cpp/velox/compute/VeloxRuntime.cc:320`
- `cpp/velox/compute/WholeStageResultIterator.cc:227`
All three go through `veloxCfg_`, which is the per-runtime conf. So base
declaring it `buildStaticConf` made `spark.conf.set` raise for a conf that
native re-reads per query anyway. It is `buildConf` now, which also puts it on
both channels — harmless, since the backend-init copy is simply never read.
This is listed in the PR description as one of the two intentional
mutability reclassifications; the other is
`spark.gluten.memoryOverhead.size.in.bytes`, which goes the other way (dynamic
→ static) because `VeloxListenerApi` sets it from the resource configuration
and `VeloxBackend::init` reads it once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
