You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "srielau (via GitHub)" <gi...@apache.org> on 2026/05/13 19:41:22 UTC
[PR] [SPARK-56853] Improve PATH Tests [spark]
srielau opened a new pull request, #55866:
URL: https://github.com/apache/spark/pull/55866
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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
Re: [PR] [SPARK-56853] Improve PATH Tests [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #55866:
URL: https://github.com/apache/spark/pull/55866#discussion_r3242930459
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/SqlPathV2CatalogSuite.scala:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector
+
+import java.util.Collections
+
+import org.apache.spark.sql.{AnalysisException, Row}
+import org.apache.spark.sql.connector.catalog.{Identifier, InMemoryCatalog, SupportsNamespaces}
+import org.apache.spark.sql.connector.catalog.functions.UnboundFunction
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.test.SharedSparkSession
+
+/**
+ * End-to-end coverage of [[SQLConf.PATH_ENABLED]] resolution through non-session V2 catalogs.
+ *
+ * Other path tests live in `SetPathSuite` (session catalog) and `ProcedureSuite`
+ * (procedures via CALL). This suite specifically exercises:
+ * - unqualified table resolution across two V2 catalogs in SET PATH,
+ * - first-match ordering when both catalogs hold the same name,
+ * - unqualified V2 function resolution across two V2 catalogs in SET PATH,
+ * - the negative case where the unqualified name only lives in a catalog
+ * that is NOT on the path.
+ */
+class SqlPathV2CatalogSuite extends SharedSparkSession {
+
+ private val emptyProps: java.util.Map[String, String] = Collections.emptyMap()
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ spark.conf.set("spark.sql.catalog.pathcat", classOf[InMemoryCatalog].getName)
+ spark.conf.set("spark.sql.catalog.pathcat2", classOf[InMemoryCatalog].getName)
+ }
+
+ override def afterAll(): Unit = {
+ try {
+ spark.sessionState.catalogManager.reset()
+ spark.sessionState.conf.unsetConf("spark.sql.catalog.pathcat")
+ spark.sessionState.conf.unsetConf("spark.sql.catalog.pathcat2")
+ } finally {
+ super.afterAll()
+ }
+ }
+
+ private def v2Catalog(name: String): InMemoryCatalog =
+ spark.sessionState.catalogManager.catalog(name).asInstanceOf[InMemoryCatalog]
+
+ private def createV2Namespace(catalog: String, ns: String): Unit = {
+ v2Catalog(catalog).asInstanceOf[SupportsNamespaces]
+ .createNamespace(Array(ns), emptyProps)
+ }
+
+ private def addV2Function(
+ catalog: String,
+ ns: String,
+ name: String,
+ fn: UnboundFunction): Unit = {
+ v2Catalog(catalog).createFunction(Identifier.of(Array(ns), name), fn)
+ }
+
+ test("V2 catalogs on SET PATH: unqualified table follows first match") {
+ withSQLConf(SQLConf.PATH_ENABLED.key -> "true") {
+ // pathcat and pathcat2 each have a namespace `ns` and a table `path_v2_t` with
+ // different contents, so we can tell which catalog supplied the row.
+ createV2Namespace("pathcat", "ns")
+ createV2Namespace("pathcat2", "ns")
+ sql("CREATE TABLE pathcat.ns.path_v2_t (id INT) USING foo")
+ sql("INSERT INTO pathcat.ns.path_v2_t VALUES (10)")
+ sql("CREATE TABLE pathcat2.ns.path_v2_t (id INT) USING foo")
+ sql("INSERT INTO pathcat2.ns.path_v2_t VALUES (20)")
+
+ try {
+ sql("SET PATH = pathcat.ns, pathcat2.ns, system.builtin")
+ checkAnswer(sql("SELECT id FROM path_v2_t"), Row(10))
+
+ sql("SET PATH = pathcat2.ns, pathcat.ns, system.builtin")
+ checkAnswer(sql("SELECT id FROM path_v2_t"), Row(20))
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("DROP TABLE IF EXISTS pathcat.ns.path_v2_t")
+ sql("DROP TABLE IF EXISTS pathcat2.ns.path_v2_t")
+ }
+ }
+ }
+
+ test("V2 catalogs on SET PATH: unqualified table only in a non-path catalog is not found") {
+ withSQLConf(SQLConf.PATH_ENABLED.key -> "true") {
+ createV2Namespace("pathcat", "ns_only_here")
+ sql("CREATE TABLE pathcat.ns_only_here.hidden_t (id INT) USING foo")
+ try {
+ // Path does not include pathcat.ns_only_here; bare `hidden_t` must not resolve.
+ sql("SET PATH = pathcat2.ns, system.builtin")
+ val e = intercept[AnalysisException] {
+ sql("SELECT id FROM hidden_t").collect()
+ }
+ assert(e.getCondition == "TABLE_OR_VIEW_NOT_FOUND" ||
+ e.getMessage.contains("TABLE_OR_VIEW_NOT_FOUND"),
+ s"Expected TABLE_OR_VIEW_NOT_FOUND; got: ${e.getCondition}: ${e.getMessage}")
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("DROP TABLE IF EXISTS pathcat.ns_only_here.hidden_t")
+ }
+ }
+ }
+
+ test("V2 catalogs on SET PATH: unqualified function follows first match") {
Review Comment:
Fixed in 542eee8. Added a small `StrLenTimes100` `ScalarFunction[Int]` next to the suite that returns `s.length * 100`, registered it on `pathcat2`, and swapped the assertions so `strlen('abc')` returns 3 or 300 depending on which catalog leads the path. Now structurally symmetric with the parallel table test (id=10 vs id=20).
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala:
##########
@@ -1453,6 +1453,106 @@ abstract class SQLViewSuite extends QueryTest {
}
}
+ test("stored view path is ignored when PATH is disabled at read time") {
Review Comment:
Done in 542eee8 -- the `SPARK-56853:` prefix is back on both new tests in this file. Good empirical point; the surrounding SPARK-56639 tests are feature tests too and the convention is consistent throughout the suite.
--
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
Re: [PR] [SPARK-56853] Improve PATH Tests [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #55866:
URL: https://github.com/apache/spark/pull/55866#discussion_r3242931940
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala:
##########
@@ -1453,6 +1453,106 @@ abstract class SQLViewSuite extends QueryTest {
}
}
+ test("stored view path is ignored when PATH is disabled at read time") {
+ // A view created with PATH enabled persists two things in metadata: the frozen
+ // resolution path AND the creator session's current catalog+namespace at CREATE
+ // VIEW time (the view's `viewCatalogAndNamespace` property). If the reader's
+ // session has `spark.sql.path.enabled=false`, the pinned entries are intentionally
+ // dropped (`CatalogManager.resolutionPathEntriesForAnalysis`); the view body's
+ // unqualified references fall back to that captured catalog+namespace, which is
+ // the creator's USE state at CREATE time -- NOT the schema the view physically
+ // lives in (the two coincide below only because the test runs
+ // `USE spark_catalog.compat_view_b` before CREATE VIEW). Verify both directions:
+ // - fully-qualified bodies keep working (qualification doesn't depend on PATH),
+ // - unqualified bodies that relied on the frozen path now resolve via the
+ // captured viewCatalogAndNamespace.
+ withDatabase("compat_view_a", "compat_view_b") {
+ sql("CREATE DATABASE compat_view_a")
+ sql("CREATE DATABASE compat_view_b")
+ withTable(
+ "compat_view_a.compat_t",
+ "compat_view_b.compat_t") {
+ sql("CREATE TABLE compat_view_a.compat_t USING parquet AS SELECT 1 AS id")
+ sql("CREATE TABLE compat_view_b.compat_t USING parquet AS SELECT 2 AS id")
+ withView(
+ "compat_view_b.v_unq_path",
+ "compat_view_b.v_fq_path") {
+ // Create both views with USE compat_view_b in effect so the stored
+ // viewCatalogAndNamespace points at compat_view_b, then SET PATH=a so the
+ // frozen path pins compat_view_a.
+ withSQLConf(PATH_ENABLED.key -> "true") {
+ try {
+ sql("USE spark_catalog.compat_view_b")
+ sql("SET PATH = spark_catalog.compat_view_a, system.builtin")
+ sql(
+ """
+ |CREATE VIEW compat_view_b.v_unq_path AS
+ |SELECT id FROM compat_t
+ |""".stripMargin)
+ sql(
+ """
+ |CREATE VIEW compat_view_b.v_fq_path AS
+ |SELECT id FROM spark_catalog.compat_view_a.compat_t
+ |""".stripMargin)
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("USE spark_catalog.default")
+ }
+ }
+
+ // Now read with PATH disabled. The fully-qualified view body is independent of
+ // PATH and must keep returning rows from compat_view_a. The unqualified-body view
+ // drops its frozen-path pin and falls back to viewCatalogAndNamespace
+ // (compat_view_b), so unqualified `compat_t` resolves to compat_view_b.compat_t.
+ withSQLConf(PATH_ENABLED.key -> "false") {
+ checkAnswer(sql("SELECT id FROM compat_view_b.v_fq_path"), Row(1))
+ checkAnswer(sql("SELECT id FROM compat_view_b.v_unq_path"), Row(2))
+ }
+ }
+ }
+ }
+ }
+
+ test("stored view path with no fallback target fails clearly when PATH is off") {
Review Comment:
Done in 542eee8.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogManagerSuite.scala:
##########
@@ -150,6 +152,115 @@ class CatalogManagerSuite extends SparkFunSuite with SQLHelper {
assert(CatalogManager.deserializePathEntries(payload).isEmpty, s"payload=$payload")
}
}
+
+ test("serializePathEntries round-trips through deserialize for typical inputs") {
+ val cases = Seq(
+ Seq(Seq("spark_catalog", "default"), Seq("system", "builtin")),
+ Seq(Seq("system", "session")),
+ Seq.empty[Seq[String]])
+ cases.foreach { entries =>
+ val payload = CatalogManager.serializePathEntries(entries)
+ val parsed = CatalogManager.deserializePathEntries(payload)
+ .getOrElse(fail(s"Expected payload to round-trip: $payload"))
+ assert(parsed === entries, s"Round-trip mismatch for $entries; got $parsed")
+ }
+ }
+
+ test("serializePathEntries round-trips multi-level and quoted identifiers") {
+ val entries = Seq(
+ Seq("cat", "ns1", "ns2"),
+ Seq("spark_catalog", "sch.with.dots"),
+ Seq("spark_catalog", "schema with spaces"))
+ val payload = CatalogManager.serializePathEntries(entries)
+ val parsed = CatalogManager.deserializePathEntries(payload)
+ .getOrElse(fail(s"Expected payload to round-trip: $payload"))
+ assert(parsed === entries)
+ }
+
+ test("deserializePathEntriesOrFail raises a clear AnalysisException for bad payloads") {
+ val e = intercept[AnalysisException] {
+ CatalogManager.deserializePathEntriesOrFail(
+ storedPathStr = "{bad-json",
+ objectType = "view",
+ objectName = "default.v_broken")
+ }
+ assert(e.getMessage.contains("Invalid stored SQL path metadata for view"))
+ assert(e.getMessage.contains("default.v_broken"))
+ }
+
+ // ---------------------------------------------------------------------------
+ // Direct unit tests for [[PathElement.validateNoStaticDuplicates]]. The end-to-end
Review Comment:
Choosing the "alternatively, just update the PR description" option -- the description has been edited to accurately say "additional `CatalogManagerSuite` cases ... covering `PathElement.validateNoStaticDuplicates`". `PathElement` is a small, single-purpose helper whose only consumer is `CatalogManager`, so keeping the seven pure-data tests in the sibling suite (where the existing `serializePathEntries` / `deserializePathEntries` cases already live) felt more natural than spinning up a dedicated 7-test file. Happy to re-extract if you prefer the strict source-mirror pattern -- the tests are self-contained and easy to move.
--
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
Re: [PR] [SPARK-56853] Improve PATH Tests [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #55866:
URL: https://github.com/apache/spark/pull/55866#issuecomment-4452791878
Thanks for the careful review @cloud-fan! All four items addressed in 542eee8:
- **`SqlPathV2CatalogSuite` first-match function test** — your read was right, the two `StrLen` impls returned identical lengths so swapping path order proved nothing. Now uses a tiny `StrLenTimes100` `ScalarFunction[Int]` on `pathcat2` so `strlen('abc')` returns 3 or 300 depending on path order. Structurally symmetric with the table test (id=10 vs id=20).
- **`SQLViewSuite` test names** — `SPARK-56853:` prefix restored on both new tests; matches the file-local convention (your SPARK-56639 frozen-path tests right next to them are feature tests with the prefix too, so the convention isn't bug-regression-only).
- **`CatalogManagerSuite` / `PathElementSuite`** — taking the "update the PR description" option. `PathElement` is a small single-purpose helper whose only consumer is `CatalogManager`, so the seven validate-no-duplicates tests sit naturally alongside the existing `serialize/deserialize` cases in the sibling suite. PR description corrected to match. Happy to re-extract if you'd prefer the strict source-mirror pattern.
- **"Explicitly deferred" claim about the single-pass resolver** — you were right, my claim was stale. `FunctionResolverUtils.isUnqualifiedCountShadowedByTemp` IS live and wired into `isNonDistinctCount` / `handleStarInArguments`; only the `spark.sql.analyzer.singlePassResolver.enabled` conf default keeps the path dormant in CI. Added a parallel `SetPathSuite` test that enables that conf for the SELECT only (CREATE TEMPORARY FUNCTION and SET PATH stay on the fixed-point analyzer because the single-pass analyzer doesn't yet support those operators) and asserts the analyzed-plan output dataType (BIGINT when the shortcut fires, INT when the gate suppresses it and the temp `count(INT) RETURN x + 100` wins). Bypassing `checkAnswer` avoids the `DeserializeToObject` operator the single-pass analyzer also doesn't yet support, but the gate predicate is exercised end-to-end through the analyzer. The "deferred" bullet for this item has been removed from the PR description.
--
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
Re: [PR] [SPARK-56853] Improve PATH Tests [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #55866:
URL: https://github.com/apache/spark/pull/55866#discussion_r3242643197
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/SqlPathV2CatalogSuite.scala:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector
+
+import java.util.Collections
+
+import org.apache.spark.sql.{AnalysisException, Row}
+import org.apache.spark.sql.connector.catalog.{Identifier, InMemoryCatalog, SupportsNamespaces}
+import org.apache.spark.sql.connector.catalog.functions.UnboundFunction
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.test.SharedSparkSession
+
+/**
+ * End-to-end coverage of [[SQLConf.PATH_ENABLED]] resolution through non-session V2 catalogs.
+ *
+ * Other path tests live in `SetPathSuite` (session catalog) and `ProcedureSuite`
+ * (procedures via CALL). This suite specifically exercises:
+ * - unqualified table resolution across two V2 catalogs in SET PATH,
+ * - first-match ordering when both catalogs hold the same name,
+ * - unqualified V2 function resolution across two V2 catalogs in SET PATH,
+ * - the negative case where the unqualified name only lives in a catalog
+ * that is NOT on the path.
+ */
+class SqlPathV2CatalogSuite extends SharedSparkSession {
+
+ private val emptyProps: java.util.Map[String, String] = Collections.emptyMap()
+
+ override def beforeAll(): Unit = {
+ super.beforeAll()
+ spark.conf.set("spark.sql.catalog.pathcat", classOf[InMemoryCatalog].getName)
+ spark.conf.set("spark.sql.catalog.pathcat2", classOf[InMemoryCatalog].getName)
+ }
+
+ override def afterAll(): Unit = {
+ try {
+ spark.sessionState.catalogManager.reset()
+ spark.sessionState.conf.unsetConf("spark.sql.catalog.pathcat")
+ spark.sessionState.conf.unsetConf("spark.sql.catalog.pathcat2")
+ } finally {
+ super.afterAll()
+ }
+ }
+
+ private def v2Catalog(name: String): InMemoryCatalog =
+ spark.sessionState.catalogManager.catalog(name).asInstanceOf[InMemoryCatalog]
+
+ private def createV2Namespace(catalog: String, ns: String): Unit = {
+ v2Catalog(catalog).asInstanceOf[SupportsNamespaces]
+ .createNamespace(Array(ns), emptyProps)
+ }
+
+ private def addV2Function(
+ catalog: String,
+ ns: String,
+ name: String,
+ fn: UnboundFunction): Unit = {
+ v2Catalog(catalog).createFunction(Identifier.of(Array(ns), name), fn)
+ }
+
+ test("V2 catalogs on SET PATH: unqualified table follows first match") {
+ withSQLConf(SQLConf.PATH_ENABLED.key -> "true") {
+ // pathcat and pathcat2 each have a namespace `ns` and a table `path_v2_t` with
+ // different contents, so we can tell which catalog supplied the row.
+ createV2Namespace("pathcat", "ns")
+ createV2Namespace("pathcat2", "ns")
+ sql("CREATE TABLE pathcat.ns.path_v2_t (id INT) USING foo")
+ sql("INSERT INTO pathcat.ns.path_v2_t VALUES (10)")
+ sql("CREATE TABLE pathcat2.ns.path_v2_t (id INT) USING foo")
+ sql("INSERT INTO pathcat2.ns.path_v2_t VALUES (20)")
+
+ try {
+ sql("SET PATH = pathcat.ns, pathcat2.ns, system.builtin")
+ checkAnswer(sql("SELECT id FROM path_v2_t"), Row(10))
+
+ sql("SET PATH = pathcat2.ns, pathcat.ns, system.builtin")
+ checkAnswer(sql("SELECT id FROM path_v2_t"), Row(20))
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("DROP TABLE IF EXISTS pathcat.ns.path_v2_t")
+ sql("DROP TABLE IF EXISTS pathcat2.ns.path_v2_t")
+ }
+ }
+ }
+
+ test("V2 catalogs on SET PATH: unqualified table only in a non-path catalog is not found") {
+ withSQLConf(SQLConf.PATH_ENABLED.key -> "true") {
+ createV2Namespace("pathcat", "ns_only_here")
+ sql("CREATE TABLE pathcat.ns_only_here.hidden_t (id INT) USING foo")
+ try {
+ // Path does not include pathcat.ns_only_here; bare `hidden_t` must not resolve.
+ sql("SET PATH = pathcat2.ns, system.builtin")
+ val e = intercept[AnalysisException] {
+ sql("SELECT id FROM hidden_t").collect()
+ }
+ assert(e.getCondition == "TABLE_OR_VIEW_NOT_FOUND" ||
+ e.getMessage.contains("TABLE_OR_VIEW_NOT_FOUND"),
+ s"Expected TABLE_OR_VIEW_NOT_FOUND; got: ${e.getCondition}: ${e.getMessage}")
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("DROP TABLE IF EXISTS pathcat.ns_only_here.hidden_t")
+ }
+ }
+ }
+
+ test("V2 catalogs on SET PATH: unqualified function follows first match") {
Review Comment:
Test name claims "follows first match", but the body can't verify that: both `StrLenDefault` (`DataSourceV2FunctionSuite.scala:743`) and `StrLenMagic` (`DataSourceV2FunctionSuite.scala:754`) return `s.length`. Swapping path order yields identical results — the comment at lines 129-130 admits this and downgrades the assertion to "neither catalog raised not found". The parallel table test above (lines 75-98) properly uses distinguishable contents (`id=10` vs `id=20`).
Either rename to reflect what's actually verified, or pick function impls that return different values — e.g., a tiny `ScalarFunction` registered against `pathcat2` that returns `s.length * 100`. Then `strlen('abc')` returns 3 or 300 depending on which catalog supplied the function, and the test would prove first-match for V2 functions.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogManagerSuite.scala:
##########
@@ -150,6 +152,115 @@ class CatalogManagerSuite extends SparkFunSuite with SQLHelper {
assert(CatalogManager.deserializePathEntries(payload).isEmpty, s"payload=$payload")
}
}
+
+ test("serializePathEntries round-trips through deserialize for typical inputs") {
+ val cases = Seq(
+ Seq(Seq("spark_catalog", "default"), Seq("system", "builtin")),
+ Seq(Seq("system", "session")),
+ Seq.empty[Seq[String]])
+ cases.foreach { entries =>
+ val payload = CatalogManager.serializePathEntries(entries)
+ val parsed = CatalogManager.deserializePathEntries(payload)
+ .getOrElse(fail(s"Expected payload to round-trip: $payload"))
+ assert(parsed === entries, s"Round-trip mismatch for $entries; got $parsed")
+ }
+ }
+
+ test("serializePathEntries round-trips multi-level and quoted identifiers") {
+ val entries = Seq(
+ Seq("cat", "ns1", "ns2"),
+ Seq("spark_catalog", "sch.with.dots"),
+ Seq("spark_catalog", "schema with spaces"))
+ val payload = CatalogManager.serializePathEntries(entries)
+ val parsed = CatalogManager.deserializePathEntries(payload)
+ .getOrElse(fail(s"Expected payload to round-trip: $payload"))
+ assert(parsed === entries)
+ }
+
+ test("deserializePathEntriesOrFail raises a clear AnalysisException for bad payloads") {
+ val e = intercept[AnalysisException] {
+ CatalogManager.deserializePathEntriesOrFail(
+ storedPathStr = "{bad-json",
+ objectType = "view",
+ objectName = "default.v_broken")
+ }
+ assert(e.getMessage.contains("Invalid stored SQL path metadata for view"))
+ assert(e.getMessage.contains("default.v_broken"))
+ }
+
+ // ---------------------------------------------------------------------------
+ // Direct unit tests for [[PathElement.validateNoStaticDuplicates]]. The end-to-end
Review Comment:
These 6 tests (lines 192-263) target `PathElement.validateNoStaticDuplicates` rather than `CatalogManager`. The PR description even says "new `PathElementSuite`", but no such file is in the patch — the tests went here instead.
`PathElement.scala` is its own self-contained class with no dedicated suite today; moving these into a sibling `PathElementSuite.scala` would mirror exactly the pattern this PR already uses for `SqlPathFormat.scala` / `SqlPathFormatSuite.scala`. The `serializePathEntries` / `deserializePathEntriesOrFail` cases would stay here since they genuinely test `CatalogManager`.
Alternatively, just update the PR description to match where the tests actually live.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala:
##########
@@ -1453,6 +1453,106 @@ abstract class SQLViewSuite extends QueryTest {
}
}
+ test("stored view path is ignored when PATH is disabled at read time") {
+ // A view created with PATH enabled persists two things in metadata: the frozen
+ // resolution path AND the creator session's current catalog+namespace at CREATE
+ // VIEW time (the view's `viewCatalogAndNamespace` property). If the reader's
+ // session has `spark.sql.path.enabled=false`, the pinned entries are intentionally
+ // dropped (`CatalogManager.resolutionPathEntriesForAnalysis`); the view body's
+ // unqualified references fall back to that captured catalog+namespace, which is
+ // the creator's USE state at CREATE time -- NOT the schema the view physically
+ // lives in (the two coincide below only because the test runs
+ // `USE spark_catalog.compat_view_b` before CREATE VIEW). Verify both directions:
+ // - fully-qualified bodies keep working (qualification doesn't depend on PATH),
+ // - unqualified bodies that relied on the frozen path now resolve via the
+ // captured viewCatalogAndNamespace.
+ withDatabase("compat_view_a", "compat_view_b") {
+ sql("CREATE DATABASE compat_view_a")
+ sql("CREATE DATABASE compat_view_b")
+ withTable(
+ "compat_view_a.compat_t",
+ "compat_view_b.compat_t") {
+ sql("CREATE TABLE compat_view_a.compat_t USING parquet AS SELECT 1 AS id")
+ sql("CREATE TABLE compat_view_b.compat_t USING parquet AS SELECT 2 AS id")
+ withView(
+ "compat_view_b.v_unq_path",
+ "compat_view_b.v_fq_path") {
+ // Create both views with USE compat_view_b in effect so the stored
+ // viewCatalogAndNamespace points at compat_view_b, then SET PATH=a so the
+ // frozen path pins compat_view_a.
+ withSQLConf(PATH_ENABLED.key -> "true") {
+ try {
+ sql("USE spark_catalog.compat_view_b")
+ sql("SET PATH = spark_catalog.compat_view_a, system.builtin")
+ sql(
+ """
+ |CREATE VIEW compat_view_b.v_unq_path AS
+ |SELECT id FROM compat_t
+ |""".stripMargin)
+ sql(
+ """
+ |CREATE VIEW compat_view_b.v_fq_path AS
+ |SELECT id FROM spark_catalog.compat_view_a.compat_t
+ |""".stripMargin)
+ } finally {
+ sql("SET PATH = DEFAULT_PATH")
+ sql("USE spark_catalog.default")
+ }
+ }
+
+ // Now read with PATH disabled. The fully-qualified view body is independent of
+ // PATH and must keep returning rows from compat_view_a. The unqualified-body view
+ // drops its frozen-path pin and falls back to viewCatalogAndNamespace
+ // (compat_view_b), so unqualified `compat_t` resolves to compat_view_b.compat_t.
+ withSQLConf(PATH_ENABLED.key -> "false") {
+ checkAnswer(sql("SELECT id FROM compat_view_b.v_fq_path"), Row(1))
+ checkAnswer(sql("SELECT id FROM compat_view_b.v_unq_path"), Row(2))
+ }
+ }
+ }
+ }
+ }
+
+ test("stored view path with no fallback target fails clearly when PATH is off") {
Review Comment:
Same convention as the other test added in this file.
```suggestion
test("SPARK-56853: stored view path with no fallback target fails clearly when PATH is off") {
```
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala:
##########
@@ -1453,6 +1453,106 @@ abstract class SQLViewSuite extends QueryTest {
}
}
+ test("stored view path is ignored when PATH is disabled at read time") {
Review Comment:
Every other test in this file uses a `SPARK-xxxx:` prefix (32374, 23519, 33141, 37932, 52521, 56639, ...). The PR description's own command `build/sbt "sql/testOnly *SimpleSQLViewSuite -- -z SPARK-56853"` won't match this test as a result.
```suggestion
test("SPARK-56853: stored view path is ignored when PATH is disabled at read time") {
```
--
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
Re: [PR] [SPARK-56853] Improve PATH Tests [spark]
Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor closed pull request #55866: [SPARK-56853] Improve PATH Tests
URL: https://github.com/apache/spark/pull/55866
--
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