jackylee-ch commented on PR #12549: URL: https://github.com/apache/gluten/pull/12549#issuecomment-5571951945
@philo-he @weiting-chen @zhouyuan — I ran your review lenses over the whole diff the way I ran @zhztheplayer's. Both turned up real things; `eaa891a69` fixes the code-and-comment ones, and the two compatibility items are now disclosed in the PR description rather than fixed, because I think they are calls for you to make. ## @philo-he's lens: classification against the native read site, and comment truth Two more keys were declared `passToNative()` without native reading them off either channel, so they go the way of the four in `369623589`: **`spark.sql.parquet.compression.codec`** and **`spark.sql.parquet.writeLegacyFormat`**. Native does read both (`VeloxWriterUtils.cc:56,70`) — but never from a conf channel. `VeloxParquetWriterInjects.nativeConf` puts them into the datasource options explicitly, and `createDataSource` merges the runtime map *underneath* with `insert`, which does not overwrite (`VeloxJniWrapper.cc:535-537`); the substrait write path takes them from the write rel. `VeloxBackend::init` reads neither, so my comment calling them "read once during native backend initialization" was wrong as well. Declarations: 62 → 60. Four claims I had made that do not survive their read sites: 1. **`spark.sql.legacy.parquet.returnNullStructIfAllFieldsMissing`** — I wrote that native's fallback matches Spark's default. It does not. Native falls back to `true` (`ConfigExtractor.cc:279`); Spark 4.1 declares the entry `booleanConf.createWithDefault(false)` (SPARK-53535, verified by decompiling `spark-catalyst_2.13-4.1.1`). So on 4.1 an unset key leaves Velox on `true` while vanilla Spark defaults to `false` — a read-result divergence. It predates this PR (base delivered the key only when set too) and closing it changes Parquet output, so I only corrected the comment and listed it as a follow-up. Happy to be told it should be fixed here instead. 2. **`spark.memory.offHeap.enabled`** — documented as read by native backend init. The key appears in **no** `cpp/` or `cpp-ch/` source; the consumer is `CHTransformerApi`, JVM-side, off the delivered backend map. Same justification `spark.gluten.memory.offHeap.size.in.bytes` carries, now stated the same way. 3. **`createHiveConnectorConfig`** — the doc said `IcebergWriter` reads the runtime conf "with no backend fallback merged in". It does merge (`VeloxJniWrapper.cc:877-879`). The path that genuinely does not is the data source sink. Rewritten — and the invariant now names its one deliberate exception: `fileHandleCacheEnabled` / `numCacheFileHandles` / `fileHandleExpirationDurationMs` stay `buildStaticConf` even though that function reads them, because native's own fallbacks (`true` / `10000` / `600000`) are byte-identical to Gluten's declared defaults, so the sink path lands on the same values. Your original question — "used at backend init time, should we keep it static?" — was the right question to keep asking; this is the one place the answer is "static, and here is why the gap is harmless". 4. The both-channels table listed `spark.sql.legacy.statisticalAggregate` as having a backend-init consumer. Neither backend reads it there. ## @weiting-chen / @zhouyuan's lens: two behavior changes I had not disclosed Both are now items 9 and 10 in the PR description. I did not "fix" either, and want your read: **A session-level `SET spark.io.compression.codec=...` now reaches Gluten's shuffle codec.** Base resolved the Gluten key through `SQLConf` but the Spark fallback key only out of the `SparkConf` argument, so a runtime `SET` had no effect. HEAD resolves both through one chained provider, SQLConf first — which is what makes the fallback a single read. Invisible in the normal case, since `mergeSparkConf` copies SparkConf into SQLConf at session creation. But `SET spark.io.compression.codec=snappy` on Velox now throws `Gluten shuffle does not support codec 'snappy' inherited from spark.io.compression.codec` where it used to be ignored. I kept it because the alternative — the Gluten key following the session while the Spark key it falls back to does not — is the same split this PR exists to remove, and the error message names the fix. Say so and I will restore base's scoping. **`spark.gluten.velox.s3UseProxyFromEnv` is now validated at session creation.** Base had it only as a bare string literal in `nativeKeys` — never a `ConfigEntry`, never registered to `SQLConf`, never validated — and Velox parses it with `folly::to<bool>`, which accepts `1`. As a real `booleanConf` entry, `SQLConf.setConfString` runs the converter while `sessionState` is built, so a cluster carrying `spark.gluten.velox.s3UseProxyFromEnv=1` in `spark-defaults.conf` now fails `getOrCreate()` with `should be boolean, but was 1`. It is an `internal()` conf and the docs only ever showed `false`, so I think fail-fast is right, but it is a hard startup break for anyone who guessed `1`. Also added to the upgrade notes: three members that **did** ship in a release are removed with no deprecation cycle — `GlutenConfig.SPARK_SHUFFLE_SPILL_COMPRESS_DEFAULT` (v1.5.0), `GlutenConfigUtil.mapByteConfValue` (v1.4.0), `VeloxConfig.AWS_S3_RETRY_MODE` (v1.2.0) — plus two startup-time failure modes an out-of-tree conf object can hit that no compile error catches: the duplicate-declaration `require` makes the foreign-key namespace globally exclusive, and the backend channel is latched on first delivery. -- 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]
