jackylee-ch commented on PR #12549: URL: https://github.com/apache/gluten/pull/12549#issuecomment-5578651915
@zhztheplayer — continuing the self-review from your angle, two questions came up that I could answer with evidence, and one of them has a factual answer that surprised me. ### "Why do the foreign keys still sit in `gluten-substrait/GlutenConfig` rather than in the backend that reads them?" The natural answer would be "scope", but that would be wrong. Checked it: **ClickHouse consumes every `spark.hadoop.fs.s3a.*` key too.** `BackendInitializerUtil::initSettings` matches on the prefix and copies each one into its own settings by stripping `spark.hadoop.` — `cpp-ch/local-engine/Common/CHUtil.cpp:696-706`, including the four per-bucket forms. So the 12 S3 declarations are genuinely shared, and moving them into `VeloxConfig` would silently stop delivering them to CH. The split turns out to already be along the line you would draw: | Where | What | |---|---| | `GlutenConfig` (42) | keys both backends read — the S3 family, the `spark.sql.*` semantics keys, the shuffle/spill sizes | | `VeloxConfig` (8) | `spark.gluten.velox.*` and the velox file-handle/cache confs | | `CHConfig` (5) | the libhdfs3 timeouts, `dfs.client.log.severity`, and `spark.sql.orc.compression.codec` — your other example is already there | | `GlutenCoreConfig` (5) | the memory sizing confs | The one group that really is Velox-only is GCS: CH matches `s3a` and `delta` prefixes but never `fs.gs`. Those three (`SPARK_GCS_STORAGE_ROOT_URL`, `SPARK_GCS_AUTH_TYPE`, `SPARK_GCS_AUTH_SERVICE_ACCOUNT_JSON_KEYFILE`) could move to `VeloxConfig` today. Happy to do it here if you want the demonstration in this PR; I left it out only because three declarations moving modules is churn that does not change behaviour. ### "Is either manual `ensureRegistered()` call load-bearing?" For the two in-tree backends, no — and I verified the chain rather than assuming: `CHListenerApi.initialize` is private and reached only from `onDriverStart`/`onExecutorStart` (`:49,:66`), which run only from `Component.sorted()` (`GlutenPlugin.scala:63-66,197`), and `Component.sorted()` calls `ensureAllComponentsRegistered()` (`Component.scala:149`) which runs `confs().foreach(_.ensureRegistered())` (`package.scala:54-62`). Same for `VeloxListenerApi.parseConf`. `GlutenConfig.registerNativeConfs`'s inline `GlutenCoreConfig.ensureRegistered()` is likewise redundant, since `val GLUTEN_ENABLED = GlutenCoreConfig.GLUTEN_ENABLED` at `GlutenConfig.scala:715` already forces it (a `ConfigEntry` reference, not a constant-foldable literal). Where I disagree is the conclusion that the latch then has nothing to diagnose. It is not there for the in-tree backends — it is for an out-of-tree component that declares `passToNative` confs and forgets to override `confs()`. That case has no compile error and no other signal; the warning is the only one. So I would keep the latch and drop the redundant calls, not the reverse. I have not dropped them yet because I wrote them into the doc as belt-and-braces for paths that reach conf selection without component discovery — which is true of tests, e.g. `NativeConfPassingSuite` calls `getNativeSessionConf` directly. Say which you prefer and I will make the doc and the code agree either way. ### Nits actioned in `43db5159b` - `convertForNative` now logs a warning when it passes a value through unconverted. You were right that the silence was the problem, and it is exactly how the 1 KiB `spark.shuffle.file.buffer` case stayed invisible. - `NativeConfEntry` is `private[config]`. Not actioned, with reasons: `conf.set(entry, Some(v))` at `GlutenPlugin.scala:149,151` is forced by the entry type, not a style choice — those entries had to become `createOptional` because native rejects their base placeholders (`GLUTEN_CHECK(numTaskSlotsPerExecutor >= 0)`, and `0` for task off-heap collapses the partial-aggregation limit that native otherwise takes from `kMaxMemory`). -- 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]
