sarutak commented on PR #59743:
URL: https://github.com/apache/spark/pull/59743#issuecomment-6479732457
Thanks for the update, @jzhuge. The implementation looks fine to me, apart
from the small comments below. My remaining concern is about what we commit to.
Once `spark.sql.extensions.defaultList` is released, its user-visible behavior
has to stay, and since you plan a general `<key>.defaultList` convention as a
follow-up, which as you noted would need to define the semantics per config,
the behavior chosen here is likely to be the reference point for other
class-list configs. So I'd like us to agree on it explicitly before this lands,
rather than leaving it to the follow-up. As I read it, this PR makes three
choices:
1. `SET spark.sql.extensions` / `spark.conf.get("spark.sql.extensions")`
show only the value of that key, not the combined list. Showing the effective
list instead would be more informative, but it would differ from what
`SparkConf` and the Environment tab show for the same key, and from what the
user actually set.
2. An extension listed in both lists is applied at its position(s) in the
user list (last-win), with a warning. First-win would keep the operator's order
instead, at the cost of the user's.
3. This differs from `spark.plugins`, where a plugin listed in both lists
keeps its position in the default list (`.distinct` over the combined list).
Either plugins stays as an exception, or one of the two changes later.
I think these choices are reasonable, but since they would be hard to change
once released, I'd like to hear what others think too. If we agree on them as
the convention for class-list configs, a general mechanism could follow later
if more configs need a default list, as this PR and #34856 did, without
changing this config's user-visible behavior. The convention could also be
checked in CI, as `SparkConfigBindingPolicySuite` does for config binding
policies. @pan3793 already suggested last-win for point 2. @cloud-fan
@dongjoon-hyun @pan3793 @HyukjinKwon, what do you think about these three
points?
A few smaller comments on the update:
- Tests: the new applied-once test (default `A`, user `A`) also passes with
first-win (`defaults ++ user.filterNot(defaults.contains)`), and no test sets
the default list together with repeats in `spark.sql.extensions`. So neither
the last-win order nor keeping the user's repeats is pinned. Could you add a
case such as default `MyExtensions` and user
`MyExtensions,MyExtensions2,MyExtensions`, asserting that
`extendedResolutionRules` filtered to `MyRule` / `MyRule2` equals `Seq(MyRule,
MyRule2, MyRule)`? Plain concatenation, first-win, and a last-win `distinct`
over the whole list each give a different result there (note that
`containsSlice` would still accept plain concatenation). A `withLogAppender`
check for the warning would be nice too.
- Docs: the `spark.sql.extensions` doc says the default list is applied
ahead of this list, but a class listed in both is applied only at its
position(s) in this list. It would help to say so there too, since someone
reading only that entry would not see the exception. In the
`spark.sql.extensions.defaultList` doc, "the entry here is dropped" could be
"every occurrence of it here is dropped", since `filterNot` drops all of them.
- Warning: it would help to also say where the class is applied, e.g. "...;
it is applied where it appears in spark.sql.extensions." Building the message
from the two entries' `.key` (e.g. via `MDC(CONFIG, ...)` / `MDC(CONFIG2,
...)`, as `SetCommand` does) would keep it in sync with the key names.
Also, just to note it to avoid confusion later: the behavior @knkarthik01
reproduced offline (as you relayed above: `spark.sql.extensions` set to
Iceberg's extension in spark-defaults.conf and to Delta's via `--conf`, so
Iceberg's extension is not loaded) is expected rather than a bug in Spark.
`spark.sql.extensions` is a single key, so a `--conf` value replaces the one in
spark-defaults.conf rather than merging with it, and that precedence is
documented. What this PR adds is a way for operators and users to configure
extensions separately, so I see it as an improvement (as the JIRA type says),
targeting 4.4.0 (master and branch-4.x) rather than something to backport to
maintenance branches.
--
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]
