philo-he commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3732582948
##########
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:
`createWithDefaultFunction` -> `ConfigEntryWithDefaultFunction`?
##########
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:
Is the session time zone currently the only special case? If so, could we
remove this method from this PR and keep the scope of this PR more focused.
BTW, Spark session timezone default value should be handled by Spark config.
Do we also need the default function in Gluten?
##########
backends-velox/src/main/scala/org/apache/gluten/config/VeloxConfig.scala:
##########
@@ -531,6 +531,8 @@ object VeloxConfig extends ConfigRegistry {
val COLUMNAR_VELOX_FILE_HANDLE_CACHE_ENABLED =
buildStaticConf("spark.gluten.sql.columnar.backend.velox.fileHandleCacheEnabled")
+ .passToNative()
+ .passDefault()
Review Comment:
+1 to remove passDefault if it is not necessary to use it.
On the C++ side, I think the code that consumes these configurations could
assume that every required configuration has been passed from Scala with its
effective value already resolved. If a required configuration is missing, the
native code could fail explicitly. If this makes sense, it seems unnecessary to
declare a separate native-side default value in C++ (though this is not the
scope of this PR).
--
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]
