cloud-fan commented on code in PR #58450:
URL: https://github.com/apache/spark/pull/58450#discussion_r4024451316
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -404,6 +388,110 @@ class FunctionResolution(
}
}
+ /**
+ * Returns whether an unqualified function name reaches `system.builtin`
before any temp or
+ * persistent function in the effective SQL PATH. When a temp or persistent
function shadows the
+ * builtin, special-syntax handling that only applies to Spark's builtins
must not fire, since the
+ * name no longer refers to the builtin -- e.g. rejecting a direct star
(bare `*` or qualified
+ * `t.*`) in a routed SQL/JSON function or the `count(tbl.*)` guard.
Parser-built `count(*)` is
+ * normalized to `count(1)` in `AstBuilder` so it skips this probe, but a
DataFrame `count("*")`
+ * keeps its star and does reach the probe during analyzer normalization.
+ *
+ * Precondition: `functionName` must already be known to be a stock built-in
name (as
+ * `functionNameResolvesToBuiltin` ensures by checking
`FunctionRegistry.functionSet` first). This
+ * returns true as soon as the PATH reaches `system.builtin`, without
verifying that
+ * `system.builtin` actually defines a function of this name, so calling it
for a non-builtin name
+ * would wrongly report builtin ownership.
+ */
+ def unqualifiedFunctionResolvesToBuiltinBeforeAnyShadow(functionName:
String): Boolean = {
+ // Walk the PATH in order and stop at the first entry that owns the name.
The default order puts
+ // system.builtin first, so the common case returns on the first entry
with no catalog lookup;
+ // only a custom PATH that lists a persistent catalog ahead of
system.builtin reaches the probe
+ // below (one lookup per such preceding entry, recomputed on each call --
not cached).
+ sqlResolutionPathEntriesForAnalysis.foreach { pathEntry =>
+ val candidate = pathEntry :+ functionName
+ FunctionResolution.sessionNamespaceKind(candidate) match {
+ case
Some(org.apache.spark.sql.catalyst.catalog.SessionCatalog.Builtin) =>
+ return true
+ case Some(org.apache.spark.sql.catalyst.catalog.SessionCatalog.Temp) =>
+ // A visible temp scalar function shadows the builtin; a visible
temp *table* function
+ // makes scalar resolution terminal at this PATH entry
(NOT_A_SCALAR_FUNCTION). Either way
+ // the name never reaches system.builtin, mirroring
`resolveFunctionCandidate`.
+ val ident = FunctionIdentifier(functionName)
+ if (v1SessionCatalog.isTemporaryScalarFunctionVisible(ident) ||
+ v1SessionCatalog.isTemporaryTableFunctionVisible(ident)) {
+ return false
+ }
+ case None =>
+ if (persistentFunctionExists(candidate)) {
+ return false
+ }
+ }
+ }
+ false
+ }
+
+ /**
+ * Returns true when a function reference resolves to the system built-in
with the requested name.
+ * This mirrors [[resolveFunction]] for special parser/analyzer rewrites
that must run only for
+ * Spark's built-ins. In particular, two-part `builtin.name` is not always a
system built-in:
+ * with `spark.sql.legacy.persistentCatalogFirst=true`, an existing
persistent
+ * `current_catalog.builtin.name` takes precedence.
+ */
+ def functionNameResolvesToBuiltin(nameParts: Seq[String], expectedName:
String): Boolean = {
+ if (!FunctionRegistry.functionSet.contains(
+ FunctionRegistry.builtinFunctionIdentifier(expectedName)) ||
+ !FunctionResolution.isUnqualifiedOrBuiltinFunctionName(nameParts,
expectedName)) {
+ return false
+ }
+ nameParts.length match {
+ case 1 =>
+ unqualifiedFunctionResolvesToBuiltinBeforeAnyShadow(nameParts.head)
+ case 2 =>
+ conf.prioritizeSystemCatalog || !persistentFunctionExists(nameParts)
Review Comment:
**Non-blocking (P2):** Function ownership is read here before direct-star
expansion, but later `ResolveFunctions` performs another lookup. If a visible
temporary or persistent shadow is dropped between the two, this pass expands
away `*`; the later lookup falls through to the stock SQL/JSON builder, which
no longer sees a `Star` and therefore skips `INVALID_USAGE_OF_STAR_OR_REGEX`.
Please bind the selected owner across preprocessing and resolution (or
otherwise preserve the direct-star fact), and cover the mutation-between-phases
interleaving deterministically in both analyzer paths.
**Recommended change:** Return and bind the selected function candidate when
star preprocessing makes an owner-dependent decision. When a shadow owns the
call, expand its star arguments while retaining that selected temp or fully
qualified persistent candidate (or an equivalent stable owner token) for later
resolution; if it disappears, fail that candidate instead of falling through to
the stock builtin. Keep immediate rejection when the selected owner is Spark's
stock SQL/JSON builder, and add deterministic owner-change regression coverage
for both analyzer strategies.
**Why this works:** Replace the Boolean-only probe/handoff with an
owner-aware result shared by Analyzer and FunctionResolverUtils. Persist the
chosen candidate on the unresolved call or consume an equivalent stable
resolution object so later ResolveFunctions cannot re-enter the PATH fallback
chain after direct-star information has been destroyed.
**Scope:** Make owner-dependent direct-star preprocessing and later routine
resolution consume one bound PATH decision.
**Compatibility:** Eligible flat clause-free SQL/JSON calls retain existing
shadowing, qualification, direct-star, view, and injected-replacement
semantics; the documented nested/routed FORMAT JSON limitation remains
unchanged.
**Risks:** Binding a candidate must preserve view-frozen catalog/namespace
expansion and not change normal shadow error messages unnecessarily. The
fixed-point and single-pass analyzers must encode the same bound owner and must
not accidentally bind parser-normalized count(*) or unrelated functions.
**Constraints:** Do not hold SessionCatalog or CatalogManager locks across
analysis or external catalog calls. Preserve scalar-versus-table terminal
ownership, stock injected-function identity, persistentCatalogFirst, and
original multipart qualification. Do not broaden this repair into the
SPARK-59243 nested FORMAT JSON transport change.
**Success:** A direct star is rejected whenever the bound selected owner is
Spark's stock routed SQL/JSON builder. A shadow selected during preprocessing
receives ordinary expanded arguments while it remains available. Removing or
replacing that shadow before later resolution cannot make the stock builtin
consume the already-expanded arguments; the query either uses the bound owner
or fails that owner cleanly. Fixed-point and single-pass analysis produce the
same owner-dependent outcome.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -404,6 +388,87 @@ class FunctionResolution(
}
}
+ /**
+ * Returns whether an unqualified function name reaches `system.builtin`
before any temp or
+ * persistent function in the effective SQL PATH. When a temp or persistent
function shadows the
+ * builtin, special-syntax rewrites (e.g. `count(*) -> count(1)`) must not
fire, since the name no
Review Comment:
You're right: AstBuilder already performed this owner-blind count(*)
normalization before the PR, so it is not a regression here. The revised
comments and retained-Star coverage accurately separate that existing behavior
from this change.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:3926332217","thread_id":"inline:3926332217","verdict_sha256":"099a08e1f5fb8b3aa3541849912f78385fb176647654fbb037b4eb3635852ff3"}
-->
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -4160,23 +4160,46 @@ class AstBuilder extends DataTypeAstBuilder
(JsonValueBehavior.Default, Some(expression(d.defaultExpr)))
}
+ // A clause-free JSON_ARRAY / JSON_QUERY that is a top-level JSON_ARRAY
element stays on the
+ // direct path. Those expressions emit JSON text implicitly, and the parent
JSON_ARRAY must see
+ // that lexical fact before analyzer rewrites can wrap the child in a Cast.
+ private def isTopLevelJsonArrayElement(ctx: RuleContext): Boolean = {
+ @scala.annotation.tailrec
+ def loop(parent: RuleContext): Boolean = parent match {
+ case null => false
+ case _: JsonArrayValueContext => true
+ case _: ExpressionContext | _: ValueExpressionDefaultContext |
+ _: ParenthesizedExpressionContext | _: CollateContext =>
+ loop(parent.getParent)
+ case p: PredicatedContext if p.predicate() == null =>
+ loop(parent.getParent)
+ case _ => false
+ }
+ loop(ctx.getParent)
+ }
+
/**
* Create a [[JsonValue]] expression for the SQL:2016 `JSON_VALUE` scalar
function. The `ON EMPTY`
* / `ON ERROR` clauses default to `NULL` when absent, per the standard.
*/
override def visitJsonValue(ctx: JsonValueContext): Expression =
withOrigin(ctx) {
val jsonExpr = expression(ctx.jsonExpr)
val path = string(visitStringLit(ctx.path))
- // Default RETURNING type is STRING. Normalize CHAR/VARCHAR to STRING for
the cast, as the value
- // is produced by a `Cast` to the declared type (a raw CHAR/VARCHAR target
has no encoder).
- val returning = Option(ctx.returning)
- .map(dt =>
CharVarcharUtils.replaceCharVarcharWithStringForCast(typedVisit[DataType](dt)))
- .getOrElse(StringType)
- val (onEmpty, emptyDefault) = Option(ctx.emptyBehavior)
- .map(buildJsonValueBehavior).getOrElse((JsonValueBehavior.Null, None))
- val (onError, errorDefault) = Option(ctx.errorBehavior)
- .map(buildJsonValueBehavior).getOrElse((JsonValueBehavior.Null, None))
- JsonValue(jsonExpr, path, returning, onEmpty, onError, emptyDefault,
errorDefault)
+ if (ctx.returning == null && ctx.emptyBehavior == null &&
ctx.errorBehavior == null) {
Review Comment:
Thanks for clarifying the boundary. The PR now documents the
canonical-SQL/routed-nesting limitation and tracks it in SPARK-59243, so I'm
okay deferring that broader rendering change from this shadowing-only patch.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:3933755510","thread_id":"inline:3933755510","verdict_sha256":"099a08e1f5fb8b3aa3541849912f78385fb176647654fbb037b4eb3635852ff3"}
-->
--
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]
