srielau commented on code in PR #56161:
URL: https://github.com/apache/spark/pull/56161#discussion_r3328409353
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -274,14 +274,59 @@ class SetPathSuite extends SharedSparkSession {
}
}
- test("PATH enabled: SET PATH = SYSTEM_PATH includes system.builtin and
system.session") {
+ test("PATH enabled: SET PATH = SYSTEM_PATH expands to system-managed
namespaces") {
+ // SPARK-57109: SYSTEM_PATH expands to the system-managed namespaces under
the `system`
+ // catalog. Today that is just `system.builtin`; the shortcut is reserved
for future
+ // system-managed schemas.
withPathEnabled {
sql("SET PATH = SYSTEM_PATH")
val entries = pathEntries(currentPath())
- assert(entries.contains("system.builtin"),
- s"SYSTEM_PATH should include system.builtin; got: $entries")
- assert(entries.contains("system.session"),
- s"SYSTEM_PATH should include system.session; got: $entries")
+ assert(entries === Seq("system.builtin"),
+ s"SYSTEM_PATH should expand to exactly [system.builtin]; got:
$entries")
+ }
+ }
+
+ test("PATH enabled: SET PATH = DEFAULT_PATH includes system.builtin,
system.session, " +
+ "and the current schema") {
+ // SPARK-57109: pin the spark-built-in default ordering used when
`spark.sql.defaultPath`
+ // is empty, so a future change to SYSTEM_PATH cannot silently drift the
DEFAULT_PATH
+ // contract. The default `sessionFunctionResolutionOrder` is "second"
(builtin first, then
+ // session, then catalog entries); ordering tests for the other modes live
below.
+ withPathEnabled {
+ sql("SET PATH = DEFAULT_PATH")
Review Comment:
Thanks — addressed in a556680, exactly per your suggestion.
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -274,14 +274,59 @@ class SetPathSuite extends SharedSparkSession {
}
}
- test("PATH enabled: SET PATH = SYSTEM_PATH includes system.builtin and
system.session") {
+ test("PATH enabled: SET PATH = SYSTEM_PATH expands to system-managed
namespaces") {
+ // SPARK-57109: SYSTEM_PATH expands to the system-managed namespaces under
the `system`
+ // catalog. Today that is just `system.builtin`; the shortcut is reserved
for future
+ // system-managed schemas.
withPathEnabled {
sql("SET PATH = SYSTEM_PATH")
val entries = pathEntries(currentPath())
- assert(entries.contains("system.builtin"),
- s"SYSTEM_PATH should include system.builtin; got: $entries")
- assert(entries.contains("system.session"),
- s"SYSTEM_PATH should include system.session; got: $entries")
+ assert(entries === Seq("system.builtin"),
+ s"SYSTEM_PATH should expand to exactly [system.builtin]; got:
$entries")
+ }
+ }
+
+ test("PATH enabled: SET PATH = DEFAULT_PATH includes system.builtin,
system.session, " +
+ "and the current schema") {
+ // SPARK-57109: pin the spark-built-in default ordering used when
`spark.sql.defaultPath`
+ // is empty, so a future change to SYSTEM_PATH cannot silently drift the
DEFAULT_PATH
+ // contract. The default `sessionFunctionResolutionOrder` is "second"
(builtin first, then
+ // session, then catalog entries); ordering tests for the other modes live
below.
+ withPathEnabled {
+ sql("SET PATH = DEFAULT_PATH")
+ val entries = pathEntries(currentPath())
+ assert(entries === Seq("system.builtin", "system.session",
"spark_catalog.default"),
+ s"DEFAULT_PATH should expand to system.builtin, system.session, and
the current " +
+ s"schema; got: $entries")
+ }
+ }
+
+ test("PATH enabled: SET PATH = SYSTEM_PATH, CURRENT_SCHEMA composes
cleanly") {
+ // SPARK-57109: SYSTEM_PATH plus CURRENT_SCHEMA is the canonical "system
functions plus my
+ // working schema" path; verify the expansion is exactly those two entries
in order.
+ withPathEnabled {
+ sql("USE spark_catalog.default")
+ sql("SET PATH = SYSTEM_PATH, CURRENT_SCHEMA")
+ val entries = pathEntries(currentPath())
+ assert(entries === Seq("system.builtin", "spark_catalog.default"),
+ s"SYSTEM_PATH, CURRENT_SCHEMA should expand to [system.builtin, " +
+ s"current schema]; got: $entries")
+ }
+ }
+
+ test("PATH enabled: SET PATH = SYSTEM_PATH, system.session is the documented
migration form") {
+ // SPARK-57109: callers who relied on the old SYSTEM_PATH expansion
(system.builtin +
+ // system.session) can name system.session explicitly. Pre-PR this would
have raised
+ // DUPLICATE_SQL_PATH_ENTRY because SYSTEM_PATH already carried
system.session; post-PR it
+ // is legal and expands to [system.builtin, system.session]. Pinning this
here locks in
+ // the migration contract and guards against a future re-expansion of
SYSTEM_PATH that
+ // would silently turn this back into a duplicate.
Review Comment:
Thanks — addressed in a556680, comment now states the current invariant and
the regression it guards.
--
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]
