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: [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]
