ganeshashree commented on code in PR #58450:
URL: https://github.com/apache/spark/pull/58450#discussion_r4011316381
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2105,54 +2105,55 @@ class Analyzer(
* This is used for special syntax transformations (e.g., COUNT(*) ->
COUNT(1)) that
* should only apply to builtin functions, not to user-defined functions.
*
- * When the effective SQL PATH puts `system.session` before
`system.builtin`, temp
- * functions shadow builtins, so an unqualified name that matches a temp
function
- * should NOT be treated as builtin.
+ * Mirrors function resolution precedence, including SQL PATH shadowing
for unqualified names
+ * and `spark.sql.legacy.persistentCatalogFirst` for two-part
`builtin.name` references.
*/
- private def matchesFunctionName(nameParts: Seq[String], expectedName:
String): Boolean = {
- if (!FunctionResolution.isUnqualifiedOrBuiltinFunctionName(nameParts,
expectedName)) {
- return false
- }
- if (nameParts.size == 1 &&
functionResolution.isSessionBeforeBuiltinInPath) {
- val v1Catalog = catalogManager.v1SessionCatalog
- !v1Catalog.isTemporaryFunction(FunctionIdentifier(nameParts.head))
- } else {
- true
- }
- }
+ private def matchesFunctionName(nameParts: Seq[String], expectedName:
String): Boolean =
+ functionResolution.functionNameResolvesToBuiltin(nameParts, expectedName)
/**
* Expands the matching attribute.*'s in `child`'s output.
*/
def expandStarExpression(expr: Expression, child: LogicalPlan): Expression
= {
expr.transformUp {
- case f0: UnresolvedFunction if !f0.isDistinct &&
- matchesFunctionName(f0.nameParts, "count") &&
- isCountStarExpansionAllowed(f0.arguments) =>
- // Transform COUNT(*) into COUNT(1).
- // We do not normalize the name to "count"; we keep the original
name parts
- // (e.g. builtin.count, system.builtin.count) so that resolution
still sees
- // the same qualification.
- f0.copy(arguments = Seq(Literal(1)))
- case f1: UnresolvedFunction if containsStar(f1.arguments) =>
- // SPECIAL CASE: We want to block count(tblName.*) because in spark,
count(tblName.*) will
- // be expanded while count(*) will be converted to count(1). They
will produce different
- // results and confuse users if there are any null values. For
count(t1.*, t2.*), it is
- // still allowed, since it's well-defined in spark.
- if (!conf.allowStarWithSingleTableIdentifierInCount &&
- matchesFunctionName(f1.nameParts, "count") &&
- f1.arguments.length == 1) {
- f1.arguments.foreach {
- case u: UnresolvedStar if u.isQualifiedByTable(child.output,
resolver) =>
- throw QueryCompilationErrors
-
.singleTableStarInCountNotAllowedError(u.target.get.mkString("."))
- case _ => // do nothing
+ case f: UnresolvedFunction if containsStar(f.arguments) =>
+ // A routed SQL/JSON function (json_array(*)) forbids a bare `*`;
reject it rather than
+ // expand below. A nested star (json_array(array(*))) is expanded
bottom-up before we get
+ // here, so only a bare `*` reaches this guard.
+ if
(functionResolution.resolvesToStarDisallowedJsonConstructor(f.nameParts)) {
Review Comment:
Fixed: the direct-star rejection now also requires
`SessionCatalog.isStockBuiltinFunction(name)`, which compares the session
registry's builder with `FunctionRegistry.builtin` by identity. The session
registry is a clone that shares the stock builder by reference, while
injectFunction installs a fresh builder under the same `system.builtin`
identifier. As a result, an injected replacement no longer triggers the
built-in-only rejection: the star is expanded and routed to it.
##########
sql/core/src/test/scala/org/apache/spark/sql/SetPathSuite.scala:
##########
@@ -965,6 +956,36 @@ class SetPathSuite extends SharedSparkSession {
}
}
+ test("path-driven COUNT rewrite gate: DataFrame count(\"*\") and count(t.*)
reach the owner " +
+ "probe and expand through a shadowing temp count") {
+ // SQL `count(*)` is normalized to `count(1)` in AstBuilder, so it never
reaches the analyzer
+ // owner probe. A DataFrame `count("*")` keeps its UnresolvedStar and
does, as does count(t.*).
+ // A non-1 input distinguishes an incorrect `count(1)` rewrite from
correct star expansion to
+ // `count(a)` through the temp.
+ withPathEnabled {
+ sql("CREATE TEMPORARY FUNCTION count(x INT) RETURNS INT RETURN x + 100")
+ try {
+ val df = sql("SELECT * FROM VALUES (7) AS t(a)")
+
+ // Builtin-first: count is the builtin, so `count("*")` collapses to
`count(1)` and returns
+ // the row count (1), while `count(t.*)` hits the single-table-star
guard.
+ checkAnswer(df.select(functions.count("*")), Row(1))
+ intercept[AnalysisException] {
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: [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]
