jackylee-ch commented on PR #12549: URL: https://github.com/apache/gluten/pull/12549#issuecomment-5570831250
Follow-up to the round above — all open review comments are addressed, and one of them turned out to be a live bug. **`spark.sql.decimalOperations.allowPrecisionLoss` was silently inverted on ClickHouse.** Copilot asked why Spark-owned boolean keys were registered as `stringConf`. Chasing it: ClickHouse keeps this key in `BOOL_VALUE_SETTINGS` (`CHUtil.h:46-47`) and `BackendInitializerUtil::toField` compares it with `value == "true" || value == "1"` (`CHUtil.cpp:632`) — case-sensitively. A user writing `TRUE` therefore turned precision loss **off** in ClickHouse while Spark read it as **on**. 13 foreign keys now declare the converter their owner declares (9 `booleanConf`, 4 `longConf` for the bloom-filter sizing keys); the other 12 were latent rather than live, since the Velox read sites use `boost::iequals` / `folly::to<bool>`. Also in this round: - `SPARK_IO_COMPRESSION_CODEC_DEFAULT` no longer restates `"lz4"` — it reads `IO_COMPRESSION_CODEC.defaultValueString`. That was the last restated foreign default left in the PR, so the invariant now holds without exception. - The shuffle-file-buffer KiB count is range-checked before being scaled to bytes, and `declaredDefault` reads a dynamic default exactly once per delivery. - A stale comment on `spark.gluten.memory.offHeap.size.in.bytes` that contradicted its own builder is gone. **The PR description is rewritten, and I want to flag that its previous behaviour-change table was wrong.** I had only checked base's `nativeKeys` set and missed the `Seq((key, default))` blocks next to it, so I claimed `spark.sql.caseSensitive` was "previously not passed at all" and that `mapKeyDedupPolicy` / `ansi.enabled` / `session.timeZone` were "now delivering Spark's default". Base delivered all four unconditionally ([`GlutenConfig.scala:562-580`](https://github.com/apache/incubator-gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala#L562-L580)). The corrected list is: two bug fixes (`spark.shuffle.file.buffer` 1024x, the ClickHouse boolean above), one conf removal (`spark.gluten.velox.fs.s3a.retry.mode`, which had no native reader), and two mutability reclassifications. Everything else is mechanism-only, checked per key against base's six lists and the native read site. @zhztheplayer @philo-he @weiting-chen @zhouyuan @taiyang-li — replies to each of your threads are inline. The two I would most like a second opinion on: 1. `BackendSettingsApi.extraNativeSessionConfKeys()` / `extraNativeBackendConfKeys()` are **removed** rather than deprecated. They are in no release and never overridden in tree; a deprecated no-op would keep compiling for a downstream backend while silently delivering nothing. I think the compile error is the kinder break, but it is your call. 2. `createWithDefaultFunction` — philo-he asked whether it could leave this PR. It now has three users and carries Spark's own method name, and it is what let `createWithForeignDefault` be deleted, so the net API surface is smaller than the round you last looked at. -- 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]
