cloud-fan commented on code in PR #56161:
URL: https://github.com/apache/spark/pull/56161#discussion_r3315830026
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -274,14 +274,42 @@ 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.
+ withPathEnabled {
+ sql("SET PATH = DEFAULT_PATH")
+ val entries = pathEntries(currentPath())
+ assert(entries.toSet === Set("system.builtin", "system.session",
"spark_catalog.default"),
+ s"DEFAULT_PATH should expand to system.builtin, system.session, and
the current " +
+ s"schema; got: $entries")
Review Comment:
The comment above (line 291) says "pin the spark-built-in default
**ordering**", and the test name says `system.builtin, system.session, and the
current schema` (an ordered list). But `entries.toSet === Set(...)` only checks
set membership -- a future change that reorders the default path (e.g. swapping
the session/builtin slots) would not fail this test. The other two new tests in
this block use ordered `Seq` equality. Suggest tightening so the assertion
matches the stated intent:
```suggestion
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")
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -128,8 +128,9 @@ class SessionCatalog(
* applied).
*
* When unset (e.g. standalone [[SessionCatalog]] in tests), kinds derive
from
- * [[SQLConf.systemPathOrder]] -- the seeded default path -- without
assuming other legacy
- * resolution-order conf beyond seeding `defaultPathOrder`.
+ * [[SQLConf.defaultPathOrder]] with no catalog entries -- equivalent to the
system slots of
Review Comment:
Minor wording: `system slots` is a coined term not used elsewhere in this
file. Reading it requires inferring that "slots" means the `system.*` entries.
Consider:
```suggestion
* [[SQLConf.defaultPathOrder]] with no catalog entries -- equivalent to
the system-namespace entries of
```
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -274,14 +274,42 @@ 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.
+ withPathEnabled {
+ sql("SET PATH = DEFAULT_PATH")
+ val entries = pathEntries(currentPath())
+ assert(entries.toSet === Set("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")
}
}
Review Comment:
Consider adding a positive test for the documented migration path `SET PATH
= SYSTEM_PATH, system.session`. The PR description names this as the explicit
replacement for users who relied on the old behavior; pre-PR it would have been
`DUPLICATE_SQL_PATH_ENTRY`, post-PR it is legal and expands to
`[system.builtin, system.session]`. Pinning it here would both lock in the
migration contract and catch a future re-expansion of `SYSTEM_PATH` that would
silently turn this back into a duplicate.
--
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]
