jackylee-ch commented on PR #12549:
URL: https://github.com/apache/gluten/pull/12549#issuecomment-5568585929
@zhztheplayer @philo-he @weiting-chen @zhouyuan @taiyang-li — the API
surface is smaller now, could you take another look?
Two changes since the last round, both from your comments:
**`createWithForeignDefault` is gone.** It existed only for the three keys
where native's fallback is wrong and Spark's default cannot be a literal
(`session.timeZone` follows the JVM default zone, `ansi.enabled` flipped in
4.0, `mapKeyDedupPolicy` must not be restated as `EXCEPTION`), and it resolved
them out of Spark's entry via a helper. `createWithDefaultFunction` — which
this PR already had, and which is Spark's own name for the same idea — covers
all three by reading the value back through Spark's accessor:
```scala
registerConf(SQLConf.SESSION_LOCAL_TIMEZONE.key)
.stringConf
.passToNative()
.createWithDefaultFunction(() => SQLConf.get.sessionLocalTimeZone)
```
Same drift-proofness (nothing is restated on the Gluten side), no new
dependency (the delivery sites already read their conf map from `SQLConf.get`),
and behaviour-preserving — `NativeConfPassingSuite` asserts it with its
existing expectations unchanged, because `getAllConfs` only reports keys that
were set, so the function runs only when the key is unset, where the accessor
yields exactly the declared default the old path resolved. Two failure modes go
away with the mechanism: the helper had to discriminate on
`defaultValue.isDefined` rather than `defaultValueString` or it would ship
`<undefined>` / `<value of other.key>` to native as a value, and it needed a
`require` guard rejecting it on a Gluten-owned conf. Net −87 lines. What is
left is `createOptional` / `createWithDefault` / `createWithDefaultFunction`,
all Spark vocabulary.
**`ConfigRegistry.get` stays abstract.** @philo-he asked why `GlutenPlugin`
needed changing — it was not equivalent: those two entries became
`OptionalConfigEntry`, whose value type is `Option[T]`, so passing a bare `Int`
stopped typechecking. But you were right that it should not degrade into
`entry.key` + `toString`; `SparkConfigUtil.set` already handles `Some(v)`, so
wrapping the value keeps the entry in the path. Fixed in three places (the
third, `VeloxListenerApi`, was the same pattern). Separately I had made
`ConfigRegistry.get` concrete purely so that test-only conf objects need not
supply an accessor — the cost was a source-breaking `override` on every
existing conf object including out-of-tree backends, and it dragged
`VeloxDeltaConfig` and `GlutenIcebergConfig` into the diff for that keyword
alone, which in turn triggered the whole Delta Spark UT pipeline on a PR that
cannot affect it. Reverted; those two files are now untouched.
Also now documented in the PR description, since it was a silent change
before: `spark.sql.caseSensitive` was in none of the old key lists, so a user
setting it got no effect on the native side (Velox kept its `false` fallback
and matched columns case-insensitively at four read sites). It is declared
`createOptional` now, so a user-set value reaches native.
@philo-he still open from your review, will follow up separately rather than
growing this PR: the `VeloxS3.md` tables have five rows that do not match the
code (`path.style.access` is effectively `true`, `retry.limit` `20`,
`connection.maximum` `15`, `connection.establish.timeout` is supported, and the
JVM side declares `fs.s3a.use.instance.credentials` while native reads
`instance.credentials`) — all pre-existing, none introduced here.
Generated-by: Claude claude-opus-5
--
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]
