sarutak commented on code in PR #59743:
URL: https://github.com/apache/spark/pull/59743#discussion_r4377499768
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -163,7 +163,10 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ private[spark] val DEFAULT_EXTENSIONS_LIST =
"spark.sql.extensions.defaultList"
+
val SPARK_SESSION_EXTENSIONS = buildStaticConf("spark.sql.extensions")
+ .withPrepended(DEFAULT_EXTENSIONS_LIST, separator = ",")
Review Comment:
Could you also update the `.doc(...)` of `spark.sql.extensions` to mention
that the value of `spark.sql.extensions.defaultList` is prepended to it?
It may also be worth noting that `spark.conf.get("spark.sql.extensions")` /
`SET spark.sql.extensions` do not show the effective list:
`SQLConf.getConfString` returns the raw setting when the key is set, so with
both keys set you see only the user value. Users looking only at
`spark.sql.extensions` might otherwise think the operator's extensions are not
applied.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -163,7 +163,10 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ private[spark] val DEFAULT_EXTENSIONS_LIST =
"spark.sql.extensions.defaultList"
Review Comment:
I understand this follows `spark.plugins.defaultList`, but how about
defining it as a registered static conf instead of a plain string key? For
example:
```scala
val SPARK_SESSION_EXTENSIONS_DEFAULT_LIST =
buildStaticConf("spark.sql.extensions.defaultList")
.doc("...")
.version("4.4.0")
.withBindingPolicy(ConfigBindingPolicy.NOT_APPLICABLE)
.stringConf
.toSequence
.createOptional
```
and then `.withPrepended(SPARK_SESSION_EXTENSIONS_DEFAULT_LIST.key,
separator = ",")` on `SPARK_SESSION_EXTENSIONS` (the new entry needs to be
defined before it).
`spark.plugins` is a core conf, and the core config tables in
`docs/configuration.md` are hand-written, so registering
`spark.plugins.defaultList` would not have produced any documentation by itself
(it is documented in `docs/monitoring.md` instead). For SQL confs, the tables
are generated from the registered entries, so as it stands this key does not
appear in the docs or in `SET -v` at all, and it has no version information.
Registering it would also make it behave like the other static SQL confs:
currently `SQLConf.isStaticConfigKey` returns false for it, so `spark.conf.set`
/ `SET` on it succeed silently with no effect, while the same operation on
`spark.sql.extensions` fails with `CANNOT_MODIFY_STATIC_CONFIG`. It would also
make "a new static SQL config" in the PR description accurate (it currently
contradicts "Implemented as an unregistered config name").
##########
sql/core/src/test/scala/org/apache/spark/sql/SparkSessionExtensionSuite.scala:
##########
@@ -513,6 +514,52 @@ class SparkSessionExtensionSuite extends PlanTest with
AdaptiveSparkPlanHelper {
}
}
+ test("SPARK-60483: use the operator default list ahead of user extensions") {
+ val session = SparkSession.builder()
+ .master("local[1]")
+ .config(DEFAULT_EXTENSIONS_LIST, classOf[MyExtensions].getCanonicalName)
+ .config(SPARK_SESSION_EXTENSIONS.key,
classOf[MyExtensions2].getCanonicalName)
+ .getOrCreate()
+ try {
+ val orderedRules = Seq(MyRule(session), MyRule2(session))
+ val orderedCheckRules = Seq(MyCheckRule(session), MyCheckRule2(session))
+
assert(session.sessionState.analyzer.extendedResolutionRules.containsSlice(orderedRules))
+
assert(session.sessionState.analyzer.extendedCheckRules.containsSlice(orderedCheckRules))
+ assert(session.sessionState.planner.strategies.containsSlice(
+ Seq(MySparkStrategy(session), MySparkStrategy2(session))))
+ } finally {
+ stop(session)
+ }
+ }
+
+ test("SPARK-60483: use the operator default list when user extensions are
unset") {
+ val session = SparkSession.builder()
+ .master("local[1]")
+ .config(DEFAULT_EXTENSIONS_LIST, classOf[MyExtensions].getCanonicalName)
+ .getOrCreate()
+ try {
+
assert(session.sessionState.analyzer.extendedResolutionRules.contains(MyRule(session)))
+
assert(session.sessionState.planner.strategies.contains(MySparkStrategy(session)))
+ } finally {
+ stop(session)
+ }
+ }
+
+ test("SPARK-60483: keep an extension listed by both the operator and the
user") {
+ // Applied twice, matching the existing contract for duplicated user
extensions.
Review Comment:
The PR description says this is "consistent with `spark.plugins.defaultList`
semantics", but plugins are actually deduplicated: `PluginContainer` loads
`conf.get(PLUGINS).distinct`, and `docs/monitoring.md` says "Duplicate plugins
are ignored." (both since SPARK-29397). So this is a divergence from the
precedent rather than consistency with it. (The comment in the test itself,
about matching the existing contract for duplicated `spark.sql.extensions`, is
accurate.)
Also, the JIRA lists de-duplication as an open question with "Proposed:
yes", while this PR does not de-duplicate. Could you share why you changed the
approach?
For what it's worth, I think not de-duplicating is reasonable here. Unlike
plugins, the order of extensions is documented to matter (rules run in order,
the last parser wraps the previous ones, and the last registered function
wins), and duplicates within `spark.sql.extensions` are already allowed and
tested. If we de-duplicated across the two lists, keeping the first occurrence
would change the order the user specified, and keeping the last one would break
the "default list is applied first" semantics, both silently. Simple
concatenation is the most predictable.
That said, an extension listed in both configs is a likely case in practice:
when an operator moves an extension from `spark.sql.extensions` in
spark-defaults to the default list, existing jobs that had copied the operator
value into their own `spark.sql.extensions` (which is exactly today's
workaround) will get it applied twice. So could you document this in the doc of
the new conf (e.g. that an extension in both lists is applied twice, so it
should be removed from `spark.sql.extensions` once it is in the default list),
and fix the PR description since it becomes the commit message? Optionally,
logging a warning in `applyExtensions` when the same class appears in both
lists would make it easier to notice, without changing the behavior.
--
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]
