cloud-fan commented on code in PR #58450:
URL: https://github.com/apache/spark/pull/58450#discussion_r3926332207
##########
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
+ * longer refers to Spark's builtin.
+ */
+ 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) =>
+ if
(v1SessionCatalog.isTemporaryFunction(FunctionIdentifier(functionName))) {
Review Comment:
**Blocking (P1):** This raw `isTemporaryFunction` check does not mirror
actual resolution inside a stored view: `SessionCatalog.handleViewContext`
hides a temporary function unless the view recorded it. A view whose frozen
PATH starts with `system.session` can therefore see a temp shadow here, skip
the JSON star guard, then hide that temp during resolution and fall through to
`system.builtin.json_array`, accepting an expanded `*` that the builtin should
reject. Please use the same view-aware session lookup as
`resolveScalarFunctionByIdentifier` and add a stored-view regression with an
unrelated temp function created after the view.
**Recommended change:** Make the builtin-ownership probe reuse
SessionCatalog's stored-view-aware temporary-function visibility semantics.
**Why this works:** Replace the raw registry existence check with a
side-effect-free lookup that applies AnalysisContext.referredTempFunctionNames
exactly as actual scalar resolution does.
**Scope:** FunctionResolution's system.session branch plus focused catalyst
and SQL stored-view coverage.
**Compatibility:** This preserves normal session lookup behavior and only
restores builtin-only syntax handling when a temporary function is not visible
to the stored view.
**Risks:** The probe must not build or execute a function expression merely
to test ownership. Scalar and table-function visibility must remain consistent
with the actual resolver path.
**Constraints:** Honor the view's recorded temporary-function names and
frozen SQL PATH. Keep fixed-point and single-pass analyzer ownership decisions
aligned.
**Success:** For stored views, the precheck and actual resolver choose the
same first visible owner, and builtin JSON star syntax is rejected even when an
unrelated invisible temp function exists.
##########
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:
**Blocking (P1):** This path-aware count gate is unreachable for an
unqualified SQL `count(*)`: `AstBuilder` has already replaced the Star with
`Literal(1)`. With a persistent `count(x INT) = x + 100` before
`system.builtin`, `SELECT count(*) FROM VALUES (7) AS t(a)` consequently
returns 101 instead of expanding the user routine's argument to `a` and
returning 107. Please leave the Star unresolved in the parser, normalize to
`count(1)` only after these ownership checks select the builtin, and update the
parser/SetPath tests with a non-1 input so they distinguish the two paths.
**Recommended change:** Move unqualified count(*) normalization from
AstBuilder to the ownership-aware analyzer paths.
**Why this works:** Preserve UnresolvedStar through parsing, then convert it
to Literal(1) only when functionNameResolvesToBuiltin confirms that Spark's
builtin count owns the call; otherwise allow ordinary star expansion for the
selected routine.
**Scope:** AstBuilder, fixed-point and single-pass count-star handling,
parser expectations, and SetPathSuite coverage.
**Compatibility:** Builtin count(*) keeps its existing semantics, while
user-defined count routines on SQL PATH receive the arguments ordinary function
resolution specifies.
**Risks:** Changing the unresolved parse shape can expose ordering
differences between the two analyzers. Qualified stars and the legacy
single-table-star count configuration must retain their existing validation.
**Constraints:** Preserve distinct-count and qualified-function behavior.
Use a non-1 and nullable input in regression coverage so count(1),
count(column), and star expansion remain distinguishable.
**Success:** Builtin count(*) still counts rows, while a persistent count
routine before system.builtin receives the expanded input column in both
analyzer modes.
--
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]
