FrankChen021 commented on code in PR #20093:
URL: https://github.com/apache/druid/pull/20093#discussion_r4026120338
##########
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] Do not coalesce all-null matching values
**Finding:** When every row passes the D1 filter but the input value is NULL
for every such row, the original SUM(CASE WHEN ... THEN value ELSE 0 END)
evaluates to NULL. The filtered native sum is also NULL, but this unconditional
nvl converts it to 0, so enabling extendedFilteredSumRewrite changes a
legitimate SQL result and cannot distinguish no matching rows from matching
rows whose values are all NULL.
**Suggestion:** Preserve the original nullable result for the
all-matching/all-null case while still returning zero for an actually empty
filter, and add a regression case covering both states.
##########
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 usable in window aggregates
**Finding:** This branch now gives every filtered SUM0 an
ExpressionPostAggregator. Windowing.fromCalciteStuff explicitly rejects any
translated aggregation with a post-aggregator, so SUM0(x) FILTER (WHERE ...)
OVER (...)—a SUM0 operator exposed by DruidOperatorTable and previously
translated to one filtered factory—now fails planning instead of executing.
**Suggestion:** Limit the post-aggregation workaround to query shapes that
support post-aggregators or add equivalent null-to-zero handling in the window
frame processor, with a filtered SUM0 window regression test.
--
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]
