FrankChen021 commented on code in PR #20093:
URL: https://github.com/apache/druid/pull/20093#discussion_r4111370300
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.java:
##########
@@ -29,4 +37,42 @@ public SqlAggFunction calciteFunction()
{
return SqlStdOperatorTable.SUM0;
}
+
+ @Override
+ Aggregation getAggregation(
+ final String name,
+ final AggregateCall aggregateCall,
+ final ExprMacroTable macroTable,
+ final String fieldName
+ )
+ {
+ final ColumnType valueType =
Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
+ if (valueType == null) {
+ return null;
+ }
+
+ if (aggregateCall.filterArg >= 0) {
Review Comment:
[P2] Keep filtered SUM0 supported in window aggregates
**Finding:** The filtered branch now always returns an Aggregation with an
ExpressionPostAggregator. Windowing.fromCalciteStuff rejects any translated
window aggregation with a post-aggregator, so a filtered SUM0 window
call—SUM0(value) FILTER (WHERE cond) OVER (...), which is registered in
DruidOperatorTable and previously translated to one
FilteredAggregatorFactory—now fails planning instead of executing.
**Suggestion:** Use a window-compatible aggregation for filtered SUM0 or
move the null-to-zero handling into window processing; cover a filtered SUM0
window query so this path remains translatable.
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.java:
##########
@@ -29,4 +37,42 @@ public SqlAggFunction calciteFunction()
{
return SqlStdOperatorTable.SUM0;
}
+
+ @Override
+ Aggregation getAggregation(
+ final String name,
+ final AggregateCall aggregateCall,
+ final ExprMacroTable macroTable,
+ final String fieldName
+ )
+ {
+ final ColumnType valueType =
Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
+ if (valueType == null) {
+ return null;
+ }
+
+ if (aggregateCall.filterArg >= 0) {
+ // SQL SUM0 semantics require 0 when the filter matches nothing, but the
native
+ // FilteredAggregatorFactory chain outputs null for empty input. Wrap
the sum in
+ // an expression post-aggregator that replaces null with 0. This keeps
the
+ // D1 rewrite of DruidAggregateCaseToFilterRule semantically consistent
with the
+ // original SUM(CASE WHEN ... THEN ... ELSE 0 END).
+ // Unfiltered SUM0 keeps the plain factory path to avoid widening this
fix.
+ final String innerName = name + ":sum";
+ final AggregatorFactory innerFactory =
+ createSumAggregatorFactory(valueType, innerName, fieldName,
macroTable);
+ return Aggregation.create(
+ ImmutableList.of(innerFactory),
+ new ExpressionPostAggregator(
+ name,
+ "nvl(\"" + innerName + "\", 0)",
Review Comment:
[P2] Preserve NULL for all-null matching rows
**Finding:** The unconditional nvl converts every null result from the
filtered sum to 0. For SUM(CASE WHEN cond THEN value ELSE 0 END), rows that
satisfy cond with a null value still contribute NULL, so when all input rows
match and every value is null the original expression returns NULL; the
rewritten SUM0/filter now returns 0. This changes SQL results for the existing
7=7,null case whenever extendedFilteredSumRewrite is enabled.
**Suggestion:** Keep enough information to distinguish no matching rows from
matching rows with only null values (for example, a filtered count alongside
the sum) and coalesce only the former; add a regression case for the all-null
state.
--
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]
