cloud-fan commented on code in PR #58450:
URL: https://github.com/apache/spark/pull/58450#discussion_r3946771542
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2352,6 +2352,20 @@ class SessionCatalog(
}
}
+ /**
+ * Returns whether a temporary function is visible in the current resolution
context, applying the
+ * same stored-view filtering as actual resolution ([[handleViewContext]])
but WITHOUT its
+ * side effect of recording the name as a referred temp function. Inside a
stored view a temp
+ * function is visible only if the view captured it; outside a view this
matches
+ * [[isTemporaryFunction]]. Ownership probes use this so they agree with the
resolver on which
+ * routine owns a name inside a view.
+ */
+ def isTemporaryFunctionVisible(name: FunctionIdentifier): Boolean = {
+ isTemporaryFunction(name) &&
Review Comment:
**Non-blocking (P2):** `isTemporaryFunctionVisible` delegates to
`isTemporaryFunction`, which reports entries from either the scalar or
table-function registry. This predicate controls scalar builtin star handling,
but `resolveScalarFunctionByIdentifier` checks only the scalar registry. With
only a temporary table function named `count`, the probe can expand a DataFrame
`count("*")` before scalar resolution falls through to builtin `count`,
producing `count(column)` instead of `count(1)`; `json_array(*)` can similarly
bypass the builtin bare-star error. Please make this visibility check
scalar-specific while retaining the stored-view filter, and cover a table-only
name collision.
**Recommended change:** Introduce a scalar-specific temporary-function
visibility check and use it for scalar builtin ownership decisions.
**Why this works:** Apply the existing stored-view visibility filter to
`functionRegistry` only, then exercise table-only collisions through the scalar
count and SQL/JSON star paths.
**Scope:** A small SessionCatalog ownership-helper change plus focused
analyzer tests.
**Compatibility:** This aligns preprocessing with existing scalar
resolution; table-function resolution and valid temporary scalar shadowing
remain unchanged.
**Risks:** The scalar-specific helper must preserve captured-function
filtering inside stored-view analysis.
**Constraints:** Keep the probe side-effect-free and consistent with
`resolveScalarFunctionByIdentifier`. Do not change temporary table-function
lookup semantics.
**Success:** A table-only temporary routine cannot alter scalar builtin star
preprocessing, while visible temporary scalar routines still shadow the builtin.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/FunctionResolverUtils.scala:
##########
@@ -57,18 +56,32 @@ trait FunctionResolverUtils {
*/
protected def handleStarInArguments(
unresolvedFunction: UnresolvedFunction): UnresolvedFunction = {
- val functionContainsStarInArguments = unresolvedFunction.arguments.exists {
+ val functionContainsDirectStarInArguments =
unresolvedFunction.arguments.exists {
case _: Star => true
case _ => false
}
- if (!functionContainsStarInArguments) {
+ // Whether the call resolves to the builtin `count` (distinct-agnostic).
This owner probe can
+ // hit an external FunctionCatalog.functionExists lookup on a
persistent-first SQL PATH, so
+ // compute it once and reuse it for both the count(*) normalization and
the count(tbl.*) guard.
+ // Lazy so the non-star and JSON-constructor paths never pay for it.
+ lazy val resolvesToCountBuiltin =
+
functionResolution.functionNameResolvesToBuiltin(unresolvedFunction.nameParts,
"count")
+
+ if (functionContainsDirectStarInArguments &&
+
functionResolution.resolvesToStarDisallowedJsonConstructor(unresolvedFunction.nameParts))
{
+ // Only a bare `*` argument is rejected in a JSON constructor; a star
nested in another
Review Comment:
**Nit (P3):** This branch also handles `JSON_VALUE`, `JSON_QUERY`, and
`JSON_EXISTS`; only `JSON_ARRAY` is a constructor. Could these comments say
"routed SQL/JSON functions" so their scope matches all four members and future
changes do not overlook the path functions and predicate?
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -937,9 +937,9 @@ class SetPathSuite extends SharedSparkSession {
test("path-driven COUNT(*) rewrite gate: temp count shadowing builtin under
SET PATH " +
"(session-first) suppresses the * -> 1 rewrite") {
- // `Analyzer.matchesFunctionName` consults
- // `FunctionResolution.isSessionBeforeBuiltinInPath` to decide whether
COUNT(*) is the
- // builtin (eligible for the COUNT(*) -> COUNT(1) shortcut) or a
user-defined override.
+ // `Analyzer.matchesFunctionName` consults
`FunctionResolution.functionNameResolvesToBuiltin`
Review Comment:
**Non-blocking (P2):** This single-pass case starts from SQL `count(*)`, but
AstBuilder rewrites an unqualified `count(*)` to `count(1)` before
`handleStarInArguments` runs. Its builtin-first and session-first outcomes
therefore exercise ordinary resolution of `count(1)`, and the test still passes
if the single-pass owner gate always selects the builtin. Could you use a
retained-Star DataFrame `count("*")` or a direct unresolved-plan case, with a
non-1 input, so this independently implemented branch is actually covered? The
SQL case name/comments should also avoid claiming owner-probe coverage.
--
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]
