Repository navigation
Conversation
Use persisted resolution-path metadata to seed analysis context for views and SQL functions so unqualified lookup stays stable across session PATH changes. Add regression tests that verify view and SQL function bodies keep creation-time PATH semantics.
7b50d3b to
4eec945
Compare
Extend SQLFunctionSuite with a table-function regression that verifies persisted SQL PATH semantics remain stable after session PATH changes.
…nt_path Add regressions for persisted views and SQL scalar/table functions to verify CURRENT_SCHEMA() and CURRENT_PATH() resolve from the invoker session context at query time.
cloud-fan
left a comment
There was a problem hiding this comment.
Summary
Prior state and problem. Persisted views and SQL functions captured a frozen SQL PATH at creation time (via pathEntriesForPersistence + serializePathEntries, in earlier work), but analysis ignored it: AnalysisContext.resolutionPathEntries stayed None for view/function bodies, so RelationResolution.relationResolutionEntries and FunctionResolution.sqlResolutionPathEntriesForAnalysis always fell through to the live session path. After a SET PATH, persisted objects could resolve unqualified names differently than at creation — the explicit "wired up in a follow-up" comments in Analyzer.scala and RelationResolution.scala flagged this gap.
Design approach. Plug the missing read side: (1) deserialize the JSON-encoded path stored in catalog metadata, (2) seed AnalysisContext.resolutionPathEntries from that value when entering a view or SQL function body. The downstream consumers (relationResolutionEntries, sqlResolutionPathEntriesForAnalysis) already prefer the pinned path when set and PATH_ENABLED is true, so no resolver-side changes are needed.
Key design decisions made by this PR.
- Deserializer placement: paired with
serializePathEntriesin theCatalogManager$companion — natural and consistent. - Malformed-input policy: return
Noneand fall back silently to live session resolution. Graceful, but a parse failure on persisted metadata is a corruption signal worth logging — see inline comment. - Function-variant context: stays at
originContext.copy(...)and overwritesresolutionPathEntrieseven when the function has no stored path. Consistent with howcollationis already overwritten on the same line — a function body is path-independent of its caller. Pre-PR this was a no-op since views also didn't set the field, but now that views do, it's a meaningful semantic; worth calling out either in code or in the PR description.
Implementation sketch.
Analyzer.scala: twowithAnalysisContextoverloads now seedresolutionPathEntriesfromviewDesc.viewStoredResolutionPath/function.functionStoredResolutionPath.RelationResolution.scala: docstring updated; a stale "follow-up PR" comment is removed.CatalogManager.scala: newdeserializePathEntries(String): Option[Seq[Seq[String]]]paired with the existing serializer.- Tests: regression tests for view, scalar function, table function, plus three "current_schema/current_path stays invoker-scoped" tests.
General comments (not anchorable to the diff)
-
Stale follow-up reference.
Analyzer.scala:1011(inResolveRelations, unchanged by this PR) still says: "Unqualified relation PATH will be snapshotted inAnalysisContext.resolutionPathEntriesin a follow-up PR." This PR is that follow-up — the companion comment inRelationResolution.scalawas updated, but this one was missed. Worth a small touch-up here for consistency. -
Test placement.
SetPathSuite.scala:29-30explicitly notes: "Resolution-level tests (tables/functions resolving via the stored path) belong in a separate suite once the resolution engine is wired." The new tests land inSQLViewSuite/SQLFunctionSuiteinstead. Either move them (or carve out a smallPathResolutionSuite) or updateSetPathSuite's docstring to point at the new location, otherwise the note inSetPathSuitebecomes misleading. -
Direct unit tests for
deserializePathEntries. The parser has severalNonebranches (empty string, non-JSON, non-array root, non-array entry, non-string parts, empty inner array, empty top-level array). They're only reachable through complex integration paths today. A small unit test inCatalogManagerSuitewould lock the contract down and document the malformed-input behavior.
Tighten frozen-path docs and tests, add direct CatalogManager deserializer coverage, and log malformed persisted path payloads while preserving empty-array semantics.
|
Addressed review follow-ups in 0581047:\n\n- Updated stale Analyzer follow-up wording and documented function-body path isolation intent.\n- Updated SetPathSuite docs to point to SQLViewSuite/SQLFunctionSuite for frozen-path resolution coverage.\n- Added direct CatalogManager.deserializePathEntries unit coverage (valid + malformed payloads).\n- Added malformed-payload warnings in deserializePathEntries while keeping empty-array semantics valid/silent.\n- Added regression-guard comments on CURRENT_SCHEMA/CURRENT_PATH invoker-context tests. |
Clarify the Analyzer comment now that frozen PATH wiring is merged, and add regression coverage that persisted view/function path metadata materializes current_schema entries at create time.
Expand PATH tests to cover case-sensitive duplicate handling, session-variable PATH gating, and current_path argument validation, and add a persisted-view check for session-only path metadata omission.
cloud-fan
left a comment
There was a problem hiding this comment.
Re-review status
7 prior findings addressed, 0 remaining, 1 new (1 newly introduced) — touches code added in commit 342b6af5d0d. LGTM otherwise; one minor test-structure suggestion inline.
Keep the current_path(1) assertion focused by removing unrelated SET PATH setup from the intercepted block, so the test can only fail on argument-count semantics.
…data Switch view/function analysis to fail loudly when stored frozen SQL PATH metadata is malformed instead of silently falling back, and add regression tests for both persisted views and SQL functions.
Set PATH to DEFAULT_PATH in the current_path(1) test setup so routine lookup resolves to built-ins and the assertion consistently validates WRONG_NUM_ARGS behavior.
|
Thanks, merging to master |
What changes were proposed in this pull request?
This PR wires frozen SQL PATH semantics into analysis for persisted views and SQL functions (scalar and table).
Specifically:
AnalysisContext.withAnalysisContext(viewDesc)now readsviewStoredResolutionPathand seedsresolutionPathEntriesfrom persisted metadata when present.AnalysisContext.withAnalysisContext(function)now readsfunctionStoredResolutionPathand seedsresolutionPathEntriesfor SQL function body analysis.CatalogManager.deserializePathEntriesis added to parse stored JSON path entries into analysis-time path entries.SQLViewSuite: persisted view keeps creation-time PATH semantics.SQLFunctionSuite: persisted SQL scalar function keeps creation-time PATH semantics.SQLFunctionSuite: persisted SQL table function keeps creation-time PATH semantics.Why are the changes needed?
Without this wiring, persisted views/functions may resolve unqualified names using the caller's current PATH instead of the PATH captured at creation time. That can cause behavior drift after
SET PATHchanges. This PR makes persisted object resolution stable and deterministic.Does this PR introduce any user-facing change?
Yes.
For persisted views and SQL functions (including SQL table functions) created with PATH enabled, unqualified name resolution now consistently follows the stored creation-time PATH, even if the session PATH changes later.
How was this patch tested?
Added/updated unit tests and ran focused suites locally:
build/sbt 'sql/testOnly org.apache.spark.sql.execution.SimpleSQLViewSuite'build/sbt 'sql/testOnly org.apache.spark.sql.execution.SQLFunctionSuite'Both suites passed with the new regression tests.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor Codex 5.3