Copilot commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3868066030
##########
gluten-core/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala:
##########
@@ -77,6 +82,152 @@ private[gluten] case class ConfigBuilder(key: String) {
this
}
+ /** Marks this config as a static (non-session-mutable) conf. Set by
`buildStaticConf`. */
+ private[config] def markStatic(): ConfigBuilder = {
+ _isStatic = true
+ this
+ }
+
+ /**
+ * Marks this config as owned by Spark / Hadoop rather than by Gluten. Set
by `registerConf` /
+ * `registerStaticConf`.
+ *
+ * A foreign config is not registered as a Gluten config entry and not
registered to `SQLConf`
+ * (Spark or Hadoop already did that); the builder is only used to declare
how the key is
+ * delivered to native side.
+ */
+ private[config] def markForeign(): ConfigBuilder = {
+ _isForeign = true
+ this
+ }
+
+ /**
+ * Marks this foreign config as delivering the default declared by Spark /
Hadoop for the key,
+ * resolved freshly at each delivery. Set by `createWithForeignDefault`,
which is only meaningful
+ * for a foreign config: a Gluten config states its default in
`createWithDefault(value)` instead.
+ */
+ private[config] def markDeliverForeignDefault(): ConfigBuilder = {
+ require(
+ _isForeign,
+ s"Config $key: createWithForeignDefault is only valid for registerConf()
/ " +
+ s"registerStaticConf(), since a Gluten config has no Spark / Hadoop
declaration to " +
+ s"resolve a default from. Use createWithDefault(value) instead."
+ )
+ _deliverForeignDefault = true
+ this
+ }
+
+ /**
+ * Marks this config to be passed to native side. A value set by the user is
always delivered;
+ * what happens when it is not set is stated by the terminal method, which
is the whole of the
+ * rule:
+ *
+ * - `createOptional`: nothing is delivered, leaving native's own fallback
in charge. This is
+ * the common case for a foreign key, since native usually declares the
same fallback Spark /
+ * Hadoop does, or branches on the key being absent at all.
+ * - `createWithForeignDefault` (foreign only): the default Spark / Hadoop
declares for the key
+ * is delivered, resolved freshly at each delivery. Use it when native's
fallback is wrong or
+ * missing, and the foreign default is either computed at runtime
+ * (`spark.sql.session.timeZone` follows the JVM default time zone) or
has changed across
+ * Spark versions (`spark.sql.ansi.enabled` flipped its default in 4.0).
Never restate such a
+ * default on the Gluten side - that is exactly what drifts.
+ * - `createWithDefault(value)`: the stated value is delivered. For a
Gluten config this is its
+ * own default; for a foreign one it says Gluten deliberately departs
from what both Spark /
+ * Hadoop and native would apply, e.g. `fs.s3a.path.style.access` where
native falls back to
+ * `false` and Gluten wants `true`.
+ *
+ * The config is registered to [[NativeConfRegistry]] on entry creation.
+ *
+ * Which delivery channels it lands on follows the conf's mutability, so
there is no argument:
+ * - `buildConf` / `registerConf`: modifiable at any time and usable at
any time. Delivered both
+ * during native backend initialization and on each native runtime
creation, so native
+ * observes the current value wherever it reads the key.
+ * - `buildStaticConf` / `registerStaticConf`: set while the native
backend is initialized and
+ * not modifiable afterwards. Delivered once during native backend
initialization.
+ */
+ def passToNative(): ConfigBuilder = {
+ _passToNative = true
+ this
+ }
+
+ /**
+ * Normalizes a value for native side through the conf's own value
converter, i.e. the one chosen
+ * by `stringConf` / `bytesConf(unit)` / `intConf` / ... plus any
`transform`. A conf therefore
+ * states how its value is parsed exactly once, at its declaration, and both
a user-set value and
+ * a resolved default go through it.
+ *
+ * This is what makes a size conf reach native as a number rather than as
"64k": a
+ * `bytesConf(ByteUnit.KiB)` yields the KiB count the foreign entry would
yield, and a
+ * `bytesConf(ByteUnit.BYTE)` the byte count. A foreign conf declares the
same converter Spark /
+ * Hadoop declares, so JVM and native agree on the value's meaning and
native applies whatever
+ * unit conversion it needs on top - `spark.shuffle.file.buffer` is KiB on
both sides, and native
+ * multiplies by 1024.
+ *
+ * Falls back to the raw string when the entry has no usable converter (e.g.
a fallback entry),
+ * since delivering the value unchanged is always better than dropping it.
+ */
+ private def convertForNative(entry: ConfigEntry[_], raw: String): String = {
+ try {
+ entry.valueConverter(raw) match {
+ // An `OptionalConfigEntry` wraps its converter's result in `Option`.
+ case o: Option[_] => o.map(_.toString).getOrElse(raw)
+ case null => raw
+ case v => v.toString
+ }
+ } catch {
+ // A value Spark or Hadoop would reject is not this mechanism's business
to validate: Spark or
+ // Hadoop raises on it at its own read site, with its own message.
Deliver it unchanged rather
+ // than failing conf selection, which runs per task.
+ case _: IllegalArgumentException => raw
+ }
+ }
+
+ private[config] def registerToNative(entry: ConfigEntry[_]): Unit = {
+ require(
+ !_isForeign || _passToNative,
+ s"Config $key: a config declared by registerConf() /
registerStaticConf() must be marked " +
+ s"with passToNative(), otherwise declaring it has no effect"
+ )
+ if (!_passToNative) {
+ return
+ }
+ // The channel follows the conf's mutability. A modifiable conf is
delivered on both channels so
+ // native observes the current value wherever it reads the key; a static
conf is set while the
+ // native backend is initialized and not modifiable afterwards, so
delivering it once there is
+ // lossless.
+ NativeConfRegistry.register(
+ key,
+ _isStatic,
+ convert = convertForNative(entry, _),
+ declaredDefault = declaredDefault(entry))
+ }
+
+ /**
+ * The default delivered to native for a key the user did not set, or `None`
to deliver nothing
+ * and leave native's own fallback in charge. Read per delivery rather than
snapshotted, so a
+ * default that follows JVM or session state keeps delivering its current
value.
+ *
+ * Which of the three the caller gets is stated by the terminal method - see
[[passToNative]].
+ */
+ private def declaredDefault(entry: ConfigEntry[_]): Option[String] = entry
match {
+ // A fallback entry reports the *target* conf's default as its own, and
the target is delivered
+ // under its own key. Delivering it here would also contradict the user:
with only the target
+ // conf set, this key would carry the target's default rather than the
value the user chose.
+ case _: ConfigEntryFallback[_] | _: ConfigEntryForeignFallback[_] => None
+ // `createWithForeignDefault` on a foreign key: take the foreign
declaration, resolved
+ // now rather than restated on the Gluten side, so the two cannot drift
across
+ // versions. The foreign default is a raw string ("32k" for
+ // `spark.shuffle.file.buffer`), so it goes through this conf's own
converter just
+ // as a user-set value does.
+ case e if _deliverForeignDefault =>
+
GlutenConfigUtil.resolveForeignDeclaredDefault(key).map(convertForNative(e, _))
+ // `createWithDefault(value)`. Reading the parsed default rather than the
raw
+ // default string means e.g. a "64MB" bytes conf reaches native as
"67108864".
+ case e if e.defaultValue.isDefined => e.defaultValue.map(_.toString)
+ // `createOptional`: nothing is delivered when the key is not set.
+ case _ => None
Review Comment:
`declaredDefault` calls `e.defaultValue` twice (`isDefined` in the guard,
then again in the body). For `ConfigEntryWithDefaultFunction` this re-evaluates
the dynamic default multiple times per delivery, which can be inconsistent (if
the default depends on mutable JVM state) and adds unnecessary overhead. Prefer
evaluating `defaultValue` only once (or just `map` it directly, which calls it
once).
--
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]
