[SPARK-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution - #58896
ganeshashree wants to merge 2 commits into
Conversation
21db1db to
f9f344a
Compare
…ocessing and function resolution Direct-star preprocessing and `ResolveFunctions` resolved a routed SQL/JSON call's owner independently, so a temp/persistent shadow dropped between the two phases could expand the star for the shadow while resolution fell through to the stock built-in, skipping `INVALID_USAGE_OF_STAR_OR_REGEX`. Resolve the owner once in a single ordered PATH pass and bind the winner on `UnresolvedFunction.boundOwner` so both phases share one decision; `resolveFunction` then resolves only that candidate. `count(*)` and injectFunction-replacement handling are unchanged. Tested in `JsonArraySuite` for both the fixed-point and single-pass analyzers. Co-authored-by: Isaac <no-reply@databricks.com>
f9f344a to
393fe6f
Compare
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The selected-owner design and deterministic two-mode regression coverage look sound overall. I found two non-blocking cleanup issues: the new carrier documentation overstates catalog qualification, and the routed owner check now materializes the full PATH before it can short-circuit.
Findings
2 total: 0 P0, 0 P1, 0 P2, 2 P3.
Nit (P3)
- Describe boundOwner as the selected PATH candidate —
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala:398— see inline. - Build resolution candidates lazily for early-return owner checks —
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:507— see inline.
PR description suggestions
- Replace the PR description's claim that boundOwner records a fully-qualified candidate with wording that it records the selected SQL PATH candidate, since the persistent-first two-part form can remain relative until later resolution.
…ution candidates - boundOwner may hold a relative persistent-catalog candidate that only gets qualified during later resolution, so the doc no longer calls it "fully-qualified". - resolutionCandidates builds single-part PATH candidates lazily so an early-returning consumer (built-in hit, routed direct-star owner walk) skips concatenating the unused PATH suffix. Co-authored-by: Isaac <no-reply@databricks.com>
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The selected-owner implementation and its two-analyzer regression coverage look sound. The two P3 concerns from the prior review are resolved: boundOwner is now documented as the selected SQL PATH candidate with later qualification made explicit, and resolutionCandidates now uses LazyList so early owner walks do not materialize unused PATH suffixes. I found no new issues in this revision.
Findings
0 total: 0 P0, 0 P1, 0 P2, 0 P3.
No findings.
Re-review status
Prior AI findings: 2 addressed, 0 still present; additional unresolved findings in this review: 0.
New attribution: 0 newly introduced, 0 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
…ocessing and function resolution ### What changes were proposed in this pull request? Direct-star preprocessing (`ResolveReferences.expandStarExpression` in the fixed-point analyzer and `FunctionResolverUtils.handleStarInArguments` in the single-pass resolver) and the later `ResolveFunctions` each resolved the owner of a routed SQL/JSON call (`json_array`, `json_value`, `json_query`, `json_exists`) independently against the SQL PATH. This resolves the owner once and binds it: - Replace the Boolean `FunctionResolution.resolvesToStarDisallowedSqlJsonFunction` with `selectRoutedSqlJsonDirectStarOwner`, returning `RoutedSqlJsonStarOwner` (`RejectStockBuiltin` / `BindShadowOwner(candidate)` / `NoBinding`). - When a shadow owns the call, expand its star and record the winning SQL PATH candidate on the new `UnresolvedFunction.boundOwner` (a relative persistent-catalog candidate stays relative until later resolution qualifies it); `resolveFunction` then resolves only that candidate, failing with `UNRESOLVED_ROUTINE` if it disappeared, instead of falling back to the stock built-in. - Stock-builtin rejection, `count(*)` / `count(tbl.*)` handling, and `injectFunction`-replacement semantics are unchanged. The check is shared by both analyzer strategies. ### Why are the changes needed? If a visible temporary or persistent shadow is dropped between the two phases, preprocessing expands the direct star for the shadow while resolution then falls through to the stock built-in, which no longer sees a `Star` and so skips `INVALID_USAGE_OF_STAR_OR_REGEX`. ### Does this PR introduce _any_ user-facing change? No, beyond fixing the above analysis inconsistency, which is only reachable under concurrent DDL between a single query's analysis phases. ### How was this patch tested? New deterministic interleaving test in `JsonArraySuite`, run for both the fixed-point and single-pass analyzers, using a catalog that reports the shadow as existing during preprocessing but fails to load it at resolution. Existing routed-function shadowing, star-rejection, `count`, `persistentCatalogFirst`, temp-table-terminal, injected-function, and view-frozen-catalog tests continue to pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8) Closes #58896 from ganeshashree/SPARK-59600. Lead-authored-by: Ganesha S <ganeshashree2@gmail.com> Co-authored-by: Ganesha S <ganesha.s@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 2247cb2) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
Direct-star preprocessing (
ResolveReferences.expandStarExpressionin the fixed-point analyzer andFunctionResolverUtils.handleStarInArgumentsin the single-pass resolver) and the laterResolveFunctionseach resolved the owner of a routed SQL/JSON call (json_array,json_value,json_query,json_exists) independently against the SQL PATH.This resolves the owner once and binds it:
FunctionResolution.resolvesToStarDisallowedSqlJsonFunctionwithselectRoutedSqlJsonDirectStarOwner, returningRoutedSqlJsonStarOwner(RejectStockBuiltin/BindShadowOwner(candidate)/NoBinding).UnresolvedFunction.boundOwner(a relative persistent-catalog candidate stays relative until later resolution qualifies it);resolveFunctionthen resolves only that candidate, failing withUNRESOLVED_ROUTINEif it disappeared, instead of falling back to the stock built-in.count(*)/count(tbl.*)handling, andinjectFunction-replacement semantics are unchanged. The check is shared by both analyzer strategies.Why are the changes needed?
If a visible temporary or persistent shadow is dropped between the two phases, preprocessing expands the direct star for the shadow while resolution then falls through to the stock built-in, which no longer sees a
Starand so skipsINVALID_USAGE_OF_STAR_OR_REGEX.Does this PR introduce any user-facing change?
No, beyond fixing the above analysis inconsistency, which is only reachable under concurrent DDL between a single query's analysis phases.
How was this patch tested?
New deterministic interleaving test in
JsonArraySuite, run for both the fixed-point and single-pass analyzers, using a catalog that reports the shadow as existing during preprocessing but fails to load it at resolution. Existing routed-function shadowing, star-rejection,count,persistentCatalogFirst, temp-table-terminal, injected-function, and view-frozen-catalog tests continue to pass.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)