Skip to content

Commit 9aab260

Browse files
alekjarmovAlek Jarmov
authored andcommitted
[SPARK-53868][SQL] Only use signature with Expression[] of visitAggregateFunction in V2ExpressionSQBuilder
### What changes were proposed in this pull request? #50143 This PR introduced `visitAggregateFunction` with `inputs: Array[Expression]`, but it's not the only usage of `visitAggregateFunction` for example here https://github.com/apache/spark/blob/6eb4d3c9d38f6849b0acfcffdbadce03c8f49ac6/sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java#L134 the old API is still used but it no longer checked if the function is supported. This means that if some dialect did not support one of `(MIN, MAX, COUNT, SUM, AVG)` it would not be blocked, but as of now all the dialects have them in the `supportedFunctions` so this is not a behavioral change. ### Why are the changes needed? To unify the API in case in the future if some dialect does not support an aggregate function of `(MIN, MAX, COUNT, SUM, AVG)` it should be blocked. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes #52573 from alekjarmov/block-aggregates. Lead-authored-by: alekjarmov <[email protected]> Co-authored-by: Alek Jarmov <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
1 parent 418cf56 commit 9aab260

File tree

1 file changed

+18
-12
lines changed

1 file changed

+18
-12
lines changed

sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -131,22 +131,17 @@ yield visitBinaryArithmetic(
131131
default -> visitUnexpectedExpr(expr);
132132
};
133133
} else if (expr instanceof Min min) {
134-
return visitAggregateFunction("MIN", false,
135-
expressionsToStringArray(min.children()));
134+
return visitAggregateFunction("MIN", false, min.children());
136135
} else if (expr instanceof Max max) {
137-
return visitAggregateFunction("MAX", false,
138-
expressionsToStringArray(max.children()));
136+
return visitAggregateFunction("MAX", false, max.children());
139137
} else if (expr instanceof Count count) {
140-
return visitAggregateFunction("COUNT", count.isDistinct(),
141-
expressionsToStringArray(count.children()));
138+
return visitAggregateFunction("COUNT", count.isDistinct(), count.children());
142139
} else if (expr instanceof Sum sum) {
143-
return visitAggregateFunction("SUM", sum.isDistinct(),
144-
expressionsToStringArray(sum.children()));
145-
} else if (expr instanceof CountStar) {
146-
return visitAggregateFunction("COUNT", false, new String[]{"*"});
140+
return visitAggregateFunction("SUM", sum.isDistinct(), sum.children());
141+
} else if (expr instanceof CountStar countStar) {
142+
return visitAggregateFunction("COUNT", false, countStar.children());
147143
} else if (expr instanceof Avg avg) {
148-
return visitAggregateFunction("AVG", avg.isDistinct(),
149-
expressionsToStringArray(avg.children()));
144+
return visitAggregateFunction("AVG", avg.isDistinct(), avg.children());
150145
} else if (expr instanceof GeneralAggregateFunc f) {
151146
if (f.orderingWithinGroups().length == 0) {
152147
return visitAggregateFunction(f.name(), f.isDistinct(), f.children());
@@ -282,8 +277,19 @@ protected String visitSQLFunction(String funcName, String[] inputs) {
282277
return joinArrayToString(inputs, ", ", funcName + "(", ")");
283278
}
284279

280+
/**
281+
* Builds SQL for an aggregate function.
282+
*
283+
* In V2ExpressionSQLBuilder, always use this override (with Expression[])
284+
* instead of the String[] version, as the String[] version does not validate
285+
* whether the function is supported in JDBC dialects.
286+
*/
285287
protected String visitAggregateFunction(
286288
String funcName, boolean isDistinct, Expression[] inputs) {
289+
// CountStar has no children but should return with a star
290+
if (funcName.equals("COUNT") && inputs == Expression.EMPTY_EXPRESSION) {
291+
return visitAggregateFunction(funcName, isDistinct, new String[]{"*"});
292+
}
287293
return visitAggregateFunction(funcName, isDistinct, expressionsToStringArray(inputs));
288294
}
289295

0 commit comments

Comments
 (0)