You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cloud-fan (via GitHub)" <gi...@apache.org> on 2026/05/28 06:45:22 UTC
Re: [PR] [SPARK-57109][SQL] SYSTEM.SESSION should not be part of SYSTEM_PATH [spark]
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org