sarutak commented on code in PR #59743:
URL: https://github.com/apache/spark/pull/59743#discussion_r4386017106
##########
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:
Thanks @pan3793, good point. I checked and "Post-Hoc Resolution", "Pre CBO
Rules" and "Early Filter and Projection Push-Down" are `Once` batches, so a
non-idempotent rule injected via e.g. `injectPostHocResolutionRule` or
`injectPreCBORule` can produce a different result when the extension is applied
twice. (Thanks @knkarthik01 for the data point. Iceberg seems fine, but it does
not tell us much about other extensions.)
Duplicates within `spark.sql.extensions` were allowed in SPARK-26493 on the
basis that users who list an extension twice know what they are doing, but a
class listed in both keys is usually not intentional, since the operator and
the user configure them independently. So I now think it is better to
de-duplicate across the two lists.
On the policy, I think it comes down to whose order wins when both lists
contain the same class. Last-win (dropping the entries of the default list that
also appear in `spark.sql.extensions`) respects the order the user wrote, which
is consistent with user configs overriding defaults, and gives today's behavior
for the migration case (default `A`, user value `A,B` copied from the
operator's config gives `A,B` instead of `A,A,B`). On the other hand, the
operator's order is lost when they conflict, e.g. default `B,A` and user `A,B`
gives `A,B`. First-win, as plugins do, has the opposite trade-off. Since either
way one side's intent can be overridden, how about de-duplicating across the
two lists (I'd slightly prefer last-win for the reason above) and logging a
warning that names the class and which position was kept? Duplicates within
`spark.sql.extensions` itself should keep working as tested by "allow an
extension to be duplicated". @jzhuge, what do you think?
As for SPI (SPARK-35380), the same class configured both via SPI and in
`spark.sql.extensions` is already applied twice today, and the relative order
of SPI providers and configured extensions also differs between the builder
path and `SparkSession`'s constructor path. So I think it is better handled
separately from 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]
