Re: [PR] [SPARK-57109] SYSTEM.SESSION should not be part of SYSTEM_PATH [spark]

["srielau (via GitHub)" <gi...@apache.org>]

srielau commented on code in PR #56161:
URL: https://github.com/apache/spark/pull/56161#discussion_r3322662127


##########
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:
   Done in f81ed8b — switched the assertion to ordered `Seq` equality and expanded the comment to note this is the `"second"` (default) order; the `"first"` and `"last"` modes are pinned separately just below.



##########
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:
   Done in f81ed8b — added `SET PATH = SYSTEM_PATH, system.session is the documented migration form`. It pins the expanded path as `[system.builtin, system.session]` and the comment calls out that pre-PR this would have raised `DUPLICATE_SQL_PATH_ENTRY`, so a future re-expansion of `SYSTEM_PATH` would fail this test.



-- 
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