cloud-fan commented on code in PR #56161:
URL: https://github.com/apache/spark/pull/56161#discussion_r3326303515
##########
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:
Once this merges, "Pre-PR"/"post-PR" lose their referent. Suggest stating
the current invariant and the regression this guards against instead:
```suggestion
// SPARK-57109: callers who relied on the old SYSTEM_PATH expansion
(system.builtin +
// system.session) can name system.session explicitly. Because
SYSTEM_PATH now expands to
// only system.builtin, listing system.session alongside it is legal and
yields
// [system.builtin, system.session]. If SYSTEM_PATH ever re-expanded to
carry system.session
// again, this entry would collide and raise DUPLICATE_SQL_PATH_ENTRY --
which is the
// regression this test guards against.
```
##########
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:
This test asserts the path ends in `spark_catalog.default`, but unlike the
`SYSTEM_PATH, CURRENT_SCHEMA` test just below it never runs `USE` to establish
the current schema — it passes only because the preceding test happens to leave
the namespace at default (there's no `beforeEach` reset). Pinning it makes the
test self-contained and consistent with its neighbor:
```suggestion
sql("USE spark_catalog.default")
sql("SET PATH = DEFAULT_PATH")
```
--
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]
