jackylee-ch commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3949772058
##########
gluten-core/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala:
##########
@@ -244,6 +392,31 @@ private[gluten] class TypedConfigBuilder[T](
default
)
parent._onCreate.foreach(_(entry))
+ parent.registerToNative(entry)
+ entry
+ }
+
+ /**
+ * Creates an entry whose default value is computed on each read rather than
fixed here, mirroring
+ * Spark's `createWithDefaultFunction`. Use it when the default depends on
JVM or session state,
+ * e.g. a time zone conf defaulting to the current JVM default time zone.
Combined with
+ * `passDefault`, native receives the value resolved at delivery time.
+ */
+ def createWithDefaultFunction(defaultFunc: () => T): ConfigEntry[T] = {
Review Comment:
Revisited this, and your two questions turned out to be connected — the
answer changed the API rather than just explaining it.
**On scope:** the method is no longer an extra concept. Base of this
discussion had three terminal forms for "what reaches native when the user did
not set the key": `createOptional`, `createWithDefault`, and a Gluten-invented
`createWithForeignDefault`. `createWithForeignDefault` is now deleted
(`079222f88`, net -87 lines) and the vocabulary is Spark's own:
| Terminal | Unset key delivers |
|---|---|
| `createOptional` | nothing — native's own fallback applies |
| `createWithDefault(v)` | `v` |
| `createWithDefaultFunction(f)` | `f()`, re-evaluated per delivery |
`createWithDefaultFunction` is the name Spark's own `ConfigBuilder` uses for
exactly this, so there is no Gluten-specific concept left to learn. It has
three users now, not one: `spark.sql.session.timeZone`,
`spark.sql.ansi.enabled`, and `spark.sql.mapKeyDedupPolicy`.
**On "Spark should handle the timezone default":** Spark does handle it —
for Spark. The gap is on the native side. When the key is absent from the conf
map native has no notion of a session time zone at all, so the resolved value
has to be delivered. And it has to be resolved *per delivery*, not snapshotted:
Spark's own default is the current JVM default time zone, which a session or a
test can change between two runtime creations.
What the function form buys is that Gluten never restates the foreign
default:
```scala
registerConf(SQLConf.SESSION_LOCAL_TIMEZONE.key)
.stringConf
.passToNative()
.createWithDefaultFunction(() => SQLConf.get.sessionLocalTimeZone)
```
`ansi.enabled` shows why a literal would not do: it flipped from `false` to
`true` in Spark 4.0, and in 4.x is not a literal at all — it derives from the
`SPARK_ANSI_SQL_MODE` environment variable (verified against
`spark-catalyst_2.13-4.1.1`). Reading it back through `SQLConf.get.ansiEnabled`
cannot drift. This costs no new dependency: the delivery sites already read
their conf map from `SQLConf.get`.
##########
gluten-core/src/main/scala/org/apache/gluten/config/ConfigEntry.scala:
##########
@@ -198,6 +198,50 @@ private[gluten] class ConfigEntryWithDefaultString[T](
override def defaultValueString: String = _defaultVal
}
+/**
+ * A config entry whose default value is computed on each read rather than
fixed at declaration,
+ * mirroring Spark's `createWithDefaultFunction`. Use it when the default
depends on JVM or session
Review Comment:
The scaladoc names `createWithDefaultFunction` on purpose — it is Spark's
`ConfigBuilder` method that this class mirrors, and the class name
`ConfigEntryWithDefaultFunction` is on the line right below. Reworded since
then so the sentence says so explicitly ("mirroring Spark's
`createWithDefaultFunction`"). Let me know if you would still rather it read
the other way round.
--
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]
