You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ganeshashree (via GitHub)" <gi...@apache.org> on 2026/09/17 17:08:36 UTC
[PR] [WIP][SPARK-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution [spark]
ganeshashree opened a new pull request, #58896:
URL: https://github.com/apache/spark/pull/58896
### 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 fully-qualified candidate on the new `UnresolvedFunction.boundOwner`; `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: Isaac
This pull request and its description were written by Isaac.
--
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-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #58896:
URL: https://github.com/apache/spark/pull/58896#issuecomment-5755675318
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/2247cb2690e80af40ba9fd660d0af9922db9c938
- merged into branch-4.x https://github.com/apache/spark/commit/c7267a2db35b99dede99e81b90dff4db209a9fd5
*Posted by `merge_spark_pr.py`*
--
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-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution [spark]
Posted by "ganeshashree (via GitHub)" <gi...@apache.org>.
ganeshashree commented on code in PR #58896:
URL: https://github.com/apache/spark/pull/58896#discussion_r4057398222
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala:
##########
@@ -394,6 +394,10 @@ case class UnresolvedGenerator(name: FunctionIdentifier, children: Seq[Expressio
* Represents an unresolved function that is being invoked. The analyzer will resolve the function
* arguments first, then look up the function by name and arguments, and return an expression that
* can be evaluated to get the result of this function invocation.
+ *
+ * `boundOwner`, when set, is the fully-qualified candidate that direct-star preprocessing selected
Review Comment:
Done.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -456,20 +470,68 @@ class FunctionResolution(
}
}
- // All routed SQL/JSON functions (JSON_ARRAY, JSON_VALUE, JSON_QUERY, JSON_EXISTS) forbid a direct
- // star argument (a bare `*` or a qualified `t.*`). Derived from the single registry list so a
- // newly routed function is covered without editing this file too.
+ // All routed SQL/JSON functions forbid a direct star argument (a bare `*` or a qualified
+ // `t.*`). Derived from the single registry list so a newly routed function is covered without
+ // editing this file too.
private val starDisallowedSqlJsonFunctions = FunctionRegistry.routedSqlJsonFunctionNames
/**
- * True if `nameParts` resolves to Spark's stock built-in routed SQL/JSON function that forbids a
- * direct star. The `isStockBuiltinFunction` check excludes an `injectFunction` replacement of
- * the name, whose expanded star is passed through rather than rejected.
+ * Resolves, once, who owns a routed SQL/JSON call (e.g. `json_array(*)`) that carries a direct
+ * star, so direct-star preprocessing and the later [[resolveFunction]] consume a single SQL PATH
+ * decision. Otherwise each phase probes ownership independently, and a temp/persistent shadow
+ * dropped between them lets preprocessing expand the star for the shadow while resolution falls
+ * through to the stock built-in, which no longer sees a [[Star]] and skips
+ * `INVALID_USAGE_OF_STAR_OR_REGEX`. See [[RoutedSqlJsonStarOwner]] for outcomes.
+ */
+ def selectRoutedSqlJsonDirectStarOwner(nameParts: Seq[String]): RoutedSqlJsonStarOwner = {
+ routedSqlJsonBuiltinName(nameParts) match {
+ case None => RoutedSqlJsonStarOwner.NoBinding
+ case Some(name) => routedSqlJsonStarOwnerFromPath(nameParts, name)
+ }
+ }
+
+ /**
+ * One ordered pass over the SQL PATH candidates: bind the first shadow that owns the call ahead
+ * of the stock `system.builtin`; reach `system.builtin` first and the stock builder owns it
+ * (reject the star, unless an injectFunction replacement holds the slot -- keep pass-through).
+ * A single pass avoids two independent probes disagreeing when a shadow is dropped between them,
+ * which could bind the stock built-in with the [[Star]] already expanded away.
*/
- def resolvesToStarDisallowedSqlJsonFunction(nameParts: Seq[String]): Boolean =
- starDisallowedSqlJsonFunctions.exists { name =>
- functionNameResolvesToBuiltin(nameParts, name) &&
- v1SessionCatalog.isStockBuiltinFunction(name)
+ private def routedSqlJsonStarOwnerFromPath(
+ nameParts: Seq[String],
+ name: String): RoutedSqlJsonStarOwner = {
+ for (candidate <- resolutionCandidates(nameParts)) {
+ if (isSystemBuiltinCandidate(candidate)) {
+ return if (v1SessionCatalog.isStockBuiltinFunction(name)) {
+ RoutedSqlJsonStarOwner.RejectStockBuiltin
+ } else {
Review Comment:
Done.
--
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-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #58896: [SPARK-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution
URL: https://github.com/apache/spark/pull/58896
--
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-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58896:
URL: https://github.com/apache/spark/pull/58896#discussion_r4053408482
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala:
##########
@@ -394,6 +394,10 @@ case class UnresolvedGenerator(name: FunctionIdentifier, children: Seq[Expressio
* Represents an unresolved function that is being invoked. The analyzer will resolve the function
* arguments first, then look up the function by name and arguments, and return an expression that
* can be evaluated to get the result of this function invocation.
+ *
+ * `boundOwner`, when set, is the fully-qualified candidate that direct-star preprocessing selected
Review Comment:
**Nit (P3):** resolutionCandidates can return the original two-part builtin.<name> candidate when persistentCatalogFirst is enabled, and that value is stored here before resolveFunctionCandidate qualifies it against the current catalog. Calling boundOwner fully qualified therefore overstates the representation and can mislead future consumers into assuming the catalog was frozen during preprocessing. Could this describe it as the selected SQL PATH candidate instead?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -456,20 +470,68 @@ class FunctionResolution(
}
}
- // All routed SQL/JSON functions (JSON_ARRAY, JSON_VALUE, JSON_QUERY, JSON_EXISTS) forbid a direct
- // star argument (a bare `*` or a qualified `t.*`). Derived from the single registry list so a
- // newly routed function is covered without editing this file too.
+ // All routed SQL/JSON functions forbid a direct star argument (a bare `*` or a qualified
+ // `t.*`). Derived from the single registry list so a newly routed function is covered without
+ // editing this file too.
private val starDisallowedSqlJsonFunctions = FunctionRegistry.routedSqlJsonFunctionNames
/**
- * True if `nameParts` resolves to Spark's stock built-in routed SQL/JSON function that forbids a
- * direct star. The `isStockBuiltinFunction` check excludes an `injectFunction` replacement of
- * the name, whose expanded star is passed through rather than rejected.
+ * Resolves, once, who owns a routed SQL/JSON call (e.g. `json_array(*)`) that carries a direct
+ * star, so direct-star preprocessing and the later [[resolveFunction]] consume a single SQL PATH
+ * decision. Otherwise each phase probes ownership independently, and a temp/persistent shadow
+ * dropped between them lets preprocessing expand the star for the shadow while resolution falls
+ * through to the stock built-in, which no longer sees a [[Star]] and skips
+ * `INVALID_USAGE_OF_STAR_OR_REGEX`. See [[RoutedSqlJsonStarOwner]] for outcomes.
+ */
+ def selectRoutedSqlJsonDirectStarOwner(nameParts: Seq[String]): RoutedSqlJsonStarOwner = {
+ routedSqlJsonBuiltinName(nameParts) match {
+ case None => RoutedSqlJsonStarOwner.NoBinding
+ case Some(name) => routedSqlJsonStarOwnerFromPath(nameParts, name)
+ }
+ }
+
+ /**
+ * One ordered pass over the SQL PATH candidates: bind the first shadow that owns the call ahead
+ * of the stock `system.builtin`; reach `system.builtin` first and the stock builder owns it
+ * (reject the star, unless an injectFunction replacement holds the slot -- keep pass-through).
+ * A single pass avoids two independent probes disagreeing when a shadow is dropped between them,
+ * which could bind the stock built-in with the [[Star]] already expanded away.
*/
- def resolvesToStarDisallowedSqlJsonFunction(nameParts: Seq[String]): Boolean =
- starDisallowedSqlJsonFunctions.exists { name =>
- functionNameResolvesToBuiltin(nameParts, name) &&
- v1SessionCatalog.isStockBuiltinFunction(name)
+ private def routedSqlJsonStarOwnerFromPath(
+ nameParts: Seq[String],
+ name: String): RoutedSqlJsonStarOwner = {
+ for (candidate <- resolutionCandidates(nameParts)) {
+ if (isSystemBuiltinCandidate(candidate)) {
+ return if (v1SessionCatalog.isStockBuiltinFunction(name)) {
+ RoutedSqlJsonStarOwner.RejectStockBuiltin
+ } else {
Review Comment:
**Nit (P3):** resolutionCandidates eagerly maps the complete effective PATH before this loop can return. With system.builtin first, it still allocates every unused suffix candidate for each routed direct-star call, and that work scales with the configured PATH length. Could candidate generation be lazy while preserving order for the existing consumers, so this owner walk stops allocating after the first owner?
--
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-59600][SQL] Bind the resolved function owner across star preprocessing and function resolution [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58896:
URL: https://github.com/apache/spark/pull/58896#discussion_r4059397384
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala:
##########
@@ -394,6 +394,10 @@ case class UnresolvedGenerator(name: FunctionIdentifier, children: Seq[Expressio
* Represents an unresolved function that is being invoked. The analyzer will resolve the function
* arguments first, then look up the function by name and arguments, and return an expression that
* can be evaluated to get the result of this function invocation.
+ *
+ * `boundOwner`, when set, is the fully-qualified candidate that direct-star preprocessing selected
Review Comment:
Confirmed: the comment now describes boundOwner as the selected SQL PATH candidate and notes that a relative persistent candidate is qualified later.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4053408482","thread_id":"inline:4053408482","verdict_sha256":"2ab988c5a8f23af5afd4e48a68affed90a1edc8fa5ffa31cb437b8276d1a8e42"} -->
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -456,20 +470,68 @@ class FunctionResolution(
}
}
- // All routed SQL/JSON functions (JSON_ARRAY, JSON_VALUE, JSON_QUERY, JSON_EXISTS) forbid a direct
- // star argument (a bare `*` or a qualified `t.*`). Derived from the single registry list so a
- // newly routed function is covered without editing this file too.
+ // All routed SQL/JSON functions forbid a direct star argument (a bare `*` or a qualified
+ // `t.*`). Derived from the single registry list so a newly routed function is covered without
+ // editing this file too.
private val starDisallowedSqlJsonFunctions = FunctionRegistry.routedSqlJsonFunctionNames
/**
- * True if `nameParts` resolves to Spark's stock built-in routed SQL/JSON function that forbids a
- * direct star. The `isStockBuiltinFunction` check excludes an `injectFunction` replacement of
- * the name, whose expanded star is passed through rather than rejected.
+ * Resolves, once, who owns a routed SQL/JSON call (e.g. `json_array(*)`) that carries a direct
+ * star, so direct-star preprocessing and the later [[resolveFunction]] consume a single SQL PATH
+ * decision. Otherwise each phase probes ownership independently, and a temp/persistent shadow
+ * dropped between them lets preprocessing expand the star for the shadow while resolution falls
+ * through to the stock built-in, which no longer sees a [[Star]] and skips
+ * `INVALID_USAGE_OF_STAR_OR_REGEX`. See [[RoutedSqlJsonStarOwner]] for outcomes.
+ */
+ def selectRoutedSqlJsonDirectStarOwner(nameParts: Seq[String]): RoutedSqlJsonStarOwner = {
+ routedSqlJsonBuiltinName(nameParts) match {
+ case None => RoutedSqlJsonStarOwner.NoBinding
+ case Some(name) => routedSqlJsonStarOwnerFromPath(nameParts, name)
+ }
+ }
+
+ /**
+ * One ordered pass over the SQL PATH candidates: bind the first shadow that owns the call ahead
+ * of the stock `system.builtin`; reach `system.builtin` first and the stock builder owns it
+ * (reject the star, unless an injectFunction replacement holds the slot -- keep pass-through).
+ * A single pass avoids two independent probes disagreeing when a shadow is dropped between them,
+ * which could bind the stock built-in with the [[Star]] already expanded away.
*/
- def resolvesToStarDisallowedSqlJsonFunction(nameParts: Seq[String]): Boolean =
- starDisallowedSqlJsonFunctions.exists { name =>
- functionNameResolvesToBuiltin(nameParts, name) &&
- v1SessionCatalog.isStockBuiltinFunction(name)
+ private def routedSqlJsonStarOwnerFromPath(
+ nameParts: Seq[String],
+ name: String): RoutedSqlJsonStarOwner = {
+ for (candidate <- resolutionCandidates(nameParts)) {
+ if (isSystemBuiltinCandidate(candidate)) {
+ return if (v1SessionCatalog.isStockBuiltinFunction(name)) {
+ RoutedSqlJsonStarOwner.RejectStockBuiltin
+ } else {
Review Comment:
Confirmed: resolutionCandidates now maps over a LazyList, so the early owner walk no longer constructs unused PATH suffix candidates.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4053408486","thread_id":"inline:4053408486","verdict_sha256":"2ab988c5a8f23af5afd4e48a68affed90a1edc8fa5ffa31cb437b8276d1a8e42"} -->
--
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