Skip to content

fix: use SUM0 in AggregateCaseToFilter D1 rewrite to avoid NULL on empty windows #20093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zhang-arvin wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: use SUM0 in AggregateCaseToFilter D1 rewrite to avoid NULL on empty windows #20093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea91c13
fix: use SUM0 in AggregateCaseToFilter D1 rewrite to avoid NULL on em…
zhang-arvin Aug 20, 2026
cacd9fb
fix: wrap filtered SUM0 in nvl post-aggregator to honor SUM0 empty-se…
zhang-arvin Sep 16, 2026
2796511
fix: add missing newline at end of SumZeroSqlAggregator.java
zhang-arvin Sep 25, 2026
98922e1
fix: apply the SUM0 null-to-zero handling only to the D1 rewrite
zhang-arvin Sep 29, 2026
473de73
test: cover all-null-match and window forms of the filtered SUM rewrite
zhang-arvin Sep 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MaxSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ Aggregation getAggregation(
final String name,
final AggregateCall aggregateCall,
final ExprMacroTable macroTable,
final String fieldName
final String fieldName,
final boolean filteredByElseZeroRewrite
)
{
final ColumnType valueType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
Expand Down
3 changes: 2 additions & 1 deletion sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MinSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ Aggregation getAggregation(
final String name,
final AggregateCall aggregateCall,
final ExprMacroTable macroTable,
final String fieldName
final String fieldName,
final boolean filteredByElseZeroRewrite
)
{
final ColumnType valueType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
Expand Down
18 changes: 16 additions & 2 deletions sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,27 @@ public Aggregation toDruidAggregation(
fieldName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(arg, aggregateCall.getType());
}

return getAggregation(name, aggregateCall, macroTable, fieldName);
// 'finalizeAggregations' is also set by DruidAggregateCaseToFilterRule, to indicate that the filter of this
// aggregate call comes from the D1 rewrite of SUM(CASE WHEN COND THEN value ELSE 0 END), in which case it
// is semantically a SUM0 with 0 - rather than null - as its empty result. See that rule and
// SumZeroSqlAggregator for details.
return getAggregation(name, aggregateCall, macroTable, fieldName, finalizeAggregations);

@FrankChen021 FrankChen021 Sep 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Use an actual D1 marker instead of the query finalization flag

Finding: finalizeAggregations is not set by DruidAggregateCaseToFilterRule. It is a query-level flag for explicit finalization of subquery aggregators (see SqlAggregator and DruidOuterQueryRel); DruidRel.runQuery passes false for a top-level query, and Windowing also passes false. Consequently, the top-level D1 calls in filtered_sum.iq take the plain nullable sum-factory branch, so the no-match case still returns NULL instead of the newly expected 0. Conversely, SUM0 calls in finalized subqueries enter the new branch regardless of whether D1 produced them. This does not implement the claimed restriction and regresses the previous revision’s no-match fix.

Suggestion: Carry rewrite provenance explicitly, independently of finalizeAggregations, and cover both a top-level no-match D1 query and a finalized subquery with an unrelated SUM0.

@FrankChen021 FrankChen021 Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not use finalization mode as the D1 marker

Finding: finalizeAggregations is controlled by query nesting, not by DruidAggregateCaseToFilterRule; top-level execution explicitly calls toDruidQuery(false), while the rule only changes the AggregateCall to SUM0. Consequently this new flag is false for normal D1 rewrites, so SumZeroSqlAggregator returns the nullable native sum and a filter with no matching input still produces NULL. The new filtered_sum.iq no-match expectations of 0 therefore cannot pass for the ordinary query path.

Suggestion: Carry an explicit D1-rewrite marker through aggregate translation separately from finalization, and apply the zero-empty handling only when that marker is present.

}

/**
* Returns the Druid {@link Aggregation} for this aggregate call.
*
* @param filteredByElseZeroRewrite true if the filter of this call was introduced by the
* {@code DruidAggregateCaseToFilterRule} rewrite of a
* {@code SUM(CASE WHEN COND THEN value ELSE 0 END)} expression, see
* {@link #toDruidAggregation}. Implementations that do not care about that
* distinction may ignore it.
*/
abstract Aggregation getAggregation(
String name,
AggregateCall aggregateCall,
ExprMacroTable macroTable,
String fieldName
String fieldName,
boolean filteredByElseZeroRewrite
);
}
3 changes: 2 additions & 1 deletion .../main/java/org/apache/druid/sql/calcite/aggregation/builtin/SingleValueSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ Aggregation getAggregation(
final String name,
final AggregateCall aggregateCall,
final ExprMacroTable macroTable,
final String fieldName
final String fieldName,
final boolean filteredByElseZeroRewrite
)
{
final ColumnType valueType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
Expand Down
3 changes: 2 additions & 1 deletion sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ Aggregation getAggregation(
final String name,
final AggregateCall aggregateCall,
final ExprMacroTable macroTable,
final String fieldName
final String fieldName,
final boolean filteredByElseZeroRewrite
)
{
final ColumnType valueType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
Expand Down
55 changes: 55 additions & 0 deletions sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,18 @@

package org.apache.druid.sql.calcite.aggregation.builtin;

import com.google.common.collect.Iterables;

@FrankChen021 FrankChen021 Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Remove unused imports before static checks

Finding: The new file imports Iterables and AggregatorFactory but uses neither. codestyle/checkstyle.xml enables the UnusedImports check, so the repository static-checks job will fail before exercising this query change.

Suggestion: Remove both unused imports from SumZeroSqlAggregator.java.

import org.apache.calcite.rel.core.AggregateCall;
import org.apache.calcite.sql.SqlAggFunction;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.druid.math.expr.ExprMacroTable;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.post.ExpressionPostAggregator;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
import org.apache.druid.sql.calcite.planner.Calcites;

import javax.annotation.Nullable;

public class SumZeroSqlAggregator extends SumSqlAggregator
{
Expand All @@ -29,4 +39,49 @@ public SqlAggFunction calciteFunction()
{
return SqlStdOperatorTable.SUM0;
}

@Override
@Nullable
Aggregation getAggregation(
final String name,
final AggregateCall aggregateCall,
final ExprMacroTable macroTable,
final String fieldName,
final boolean filteredByElseZeroRewrite
)
{
final ColumnType valueType = Calcites.getColumnTypeForRelDataType(aggregateCall.getType());
if (valueType == null) {
return null;
}

if (filteredByElseZeroRewrite) {
// This SUM0 only exists because DruidAggregateCaseToFilterRule rewrote the D1 shape
// SUM(CASE WHEN COND THEN value ELSE 0 END) into SUM0(value) FILTER (WHERE COND), and the
// native filtered aggregator returns null when the filter matches no row, while the
// original CASE expression returned 0. Restore that by wrapping the filtered sum in an
// expression post-aggregator that replaces null with 0.
//
// The post-aggregator is created here, instead of leaving it to Aggregation.filter, on
// purpose: the Aggregation returned by this method carries no aggregator factory, so
// Aggregation.filter never pushes the filter down into a FilteredAggregatorFactory. The
// resulting post-aggregator-only Aggregation is what Windowing accepts for window
// aggregations, while the equivalent factory-based post-aggregator
// ([expressionPostAgg, filteredAgg]) is rejected by Windowing.fromCalciteStuff.
return Aggregation.create(

@FrankChen021 FrankChen021 Sep 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep a real sum factory behind the post-aggregator

Finding: When this branch is reached (for example, a D1 aggregate in a finalized native subquery), fieldName names the raw input or virtual column, not an existing aggregate output. Returning only nvl(fieldName, 0) removes the sum entirely. GroupByRules then applies the D1 filter, and Aggregation.filter throws "Cannot filter an Aggregation that does not contain its inputs" because the post-aggregator depends on fieldName but the factory list is empty. Thus previously executable nested CASE/SUM queries fail during planning; omitting the filter would still not compute a sum. A post-aggregator-only object also does not satisfy Windowing’s contract, which rejects every non-null post-aggregator.

Suggestion: Retain a sum factory and reference its output from the post-aggregator, or implement the required semantics in an aggregate factory that satisfies both filtered aggregation and window translation contracts.

@FrankChen021 FrankChen021 Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Retain the aggregate factory with the post-aggregator

Finding: When finalizeAggregations is true, the new branch returns an Aggregation containing only an ExpressionPostAggregator whose expression references fieldName; no AggregatorFactory computes that field or receives the D1 filter. GroupByRules subsequently applies the rewritten CASE filter through Aggregation.filter, which rejects post-aggregator inputs absent from aggregatorFactories, so finalized subqueries using this rewrite fail during planning with an ISE.

Suggestion: Keep the filtered SUM0 factory in the Aggregation and layer the null-to-zero result around its output, while handling any window-specific restriction in window translation rather than dropping the factory.

new ExpressionPostAggregator(
name,
"nvl(\"" + fieldName + "\", 0)",

@FrankChen021 FrankChen021 Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve NULL for all-null matching rows

Finding: nvl(fieldName, 0) collapses two distinct SQL states: no rows pass the CASE condition and rows pass it but every value is NULL. The original SUM(CASE WHEN ... THEN value ELSE 0 END) returns 0 in the first state but NULL in the second, as the rewrite-disabled control in filtered_sum.iq demonstrates; this post-aggregator returns 0 for both whenever the branch is used.

Suggestion: Distinguish an empty match from an all-null match before coalescing, or retain the original nullable result for the all-null case.

null,
valueType,
macroTable
)
);
}

// Plain SQL SUM0(x) and SUM0(x) FILTER (WHERE ...): keep the native nullable factory, which
// preserves the documented null-on-empty behavior for those calls. Only the D1 rewrite above
// is required to return 0.
return Aggregation.create(createSumAggregatorFactory(valueType, name, fieldName, macroTable));
}
}
8 changes: 6 additions & 2 deletions sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,13 @@ && isIntLiteral(arg2, BigDecimal.ZERO))) {
newProjects.add(arg1);
newProjects.add(filter);

RelDataType newType = typeFactory.createTypeWithNullability(call.getType(), true);
// Use SUM0 instead of SUM to return 0 instead of NULL when filter never matches.
// This fixes the known inconsistency documented in the class javadoc where
// SUM(CASE WHEN COND THEN COL1 ELSE 0 END) would return NULL instead of 0
// when no rows match the condition.
RelDataType newType = typeFactory.createTypeWithNullability(call.getType(), false);
return AggregateCall.create(
call.getAggregation(),
SqlStdOperatorTable.SUM0,

@FrankChen021 FrankChen021 Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] SUM0 still executes as nullable SUM

SumZeroSqlAggregator inherits SumSqlAggregator's nullable native Long/Float/DoubleSumAggregatorFactory. With no matching filter rows, the nullable wrapper returns NULL, so native Druid queries still produce NULL for the empty/no-match cases changed to 0 in filtered_sum.iq.

@FrankChen021 FrankChen021 Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Rewrite changes all-null semantics

When every row matches the condition and the value is NULL, SUM(CASE WHEN condition THEN value ELSE 0 END) returns NULL, while SUM0(value) FILTER returns 0. The rewrite applies this universally; the rewrite-disabled test still documents NULL for the 7=7,null case.

false,
false,
false,
Expand Down
53 changes: 47 additions & 6 deletions sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/filtered_sum.iq
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ SELECT COUNT(1)FILTER(WHERE l1=-1),COUNT(1)FILTER(WHERE l1!=-1),MIN(l2) is null,
+--------+--------+--------+--------+--------+
| EXPR$0 | EXPR$1 | EXPR$2 | EXPR$3 | EXPR$4 |
+--------+--------+--------+--------+--------+
| 0 | 0 | true | | |
| 0 | 0 | true | 0 | |
+--------+--------+--------+--------+--------+
(1 row)

Expand All @@ -18,7 +18,7 @@ SELECT COUNT(1)FILTER(WHERE l1=-1),COUNT(1)FILTER(WHERE l1!=-1),MIN(l2) is null,
+--------+--------+--------+--------+--------+
| EXPR$0 | EXPR$1 | EXPR$2 | EXPR$3 | EXPR$4 |
+--------+--------+--------+--------+--------+
| 0 | 1 | false | | |
| 0 | 1 | false | 0 | |
+--------+--------+--------+--------+--------+
(1 row)

Expand All @@ -37,21 +37,50 @@ SELECT COUNT(1)FILTER(WHERE l1=0),COUNT(1)FILTER(WHERE l1!=0),MIN(l2) is null,

!ok

-- all rows match the condition, but all matching values are null: l1 IS NULL selects
-- the three rows that have neither l1 nor l2 set. The D1 rewrite turns
-- SUM(CASE WHEN l1 IS NULL THEN l2 ELSE 0 END) into SUM0(l2) FILTER (WHERE l1 IS NULL),
-- which matches rows but has no non-null value to sum, so it returns NULL and the
-- nvl post-aggregator of the D1 rewrite reports 0. The user-written filtered SUM stays
-- on the native path and keeps the pre-rewrite NULL.
SELECT SUM(CASE WHEN l1 IS NULL THEN l2 ELSE 0 END),SUM(l2) FILTER(WHERE l1 IS NULL) FROM numfoo WHERE l1 IS NULL;
+--------+--------+
| EXPR$0 | EXPR$1 |
+--------+--------+
| 0 | |
+--------+--------+
(1 row)

!ok

-- window form of a user-written filtered aggregate: must not pick up the D1 rewrite
-- post-aggregator, otherwise Windowing.fromCalciteStuff rejects the translation.
-- The frame is the entire partition and holds the two non-null l2 values (325323 and 0).
SELECT SUM0(l2) FILTER(WHERE l2 IS NOT NULL) OVER () FROM numfoo ORDER BY l1 LIMIT 1;
+--------+
| EXPR$0 |
+--------+
| 325323 |
+--------+
(1 row)

!ok

-- 7=7,null
SELECT COUNT(1)FILTER(WHERE l1=7),COUNT(1)FILTER(WHERE l1!=7),MIN(l2) is null,
SUM(CASE WHEN l1 = 7 THEN l2 ELSE 0 END),SUM(l2) FILTER(WHERE l1=7) FROM numfoo where 0 < l1 and l1 < 10;
+--------+--------+--------+--------+--------+
| EXPR$0 | EXPR$1 | EXPR$2 | EXPR$3 | EXPR$4 |
+--------+--------+--------+--------+--------+
| 1 | 0 | true | | |
| 1 | 0 | true | 0 | |
+--------+--------+--------+--------+--------+
(1 row)

!ok

LogicalProject(EXPR$0=[$0], EXPR$1=[$1], EXPR$2=[IS NULL($2)], EXPR$3=[$3], EXPR$4=[$4])
LogicalAggregate(group=[{}], EXPR$0=[COUNT() FILTER $0], EXPR$1=[COUNT() FILTER $1], agg#2=[MIN($2)], EXPR$3=[SUM($3)], EXPR$4=[SUM($2) FILTER $0])
LogicalProject($f1=[IS TRUE(=($0, 7))], $f2=[IS TRUE(<>($0, 7))], l2=[$1], $f4=[CASE(=($0, 7), $1, 0:BIGINT)])
LogicalProject(EXPR$0=[$0], EXPR$1=[$1], EXPR$2=[IS NULL($2)], EXPR$3=[CAST($3):BIGINT], EXPR$4=[$4])
LogicalAggregate(group=[{}], EXPR$0=[COUNT() FILTER $0], EXPR$1=[COUNT() FILTER $1], agg#2=[MIN($2)], EXPR$3=[$SUM0($3) FILTER $4], EXPR$4=[SUM($2) FILTER $0])
LogicalProject($f0=[IS TRUE(=($0, 7))], $f1=[IS TRUE(<>($0, 7))], l2=[$1], l20=[$1], $f5=[IS TRUE(=($0, 7))])
LogicalFilter(condition=[SEARCH($0, Sarg[(0L:BIGINT..10L:BIGINT)]:BIGINT)])
LogicalProject(l1=[$11], l2=[$12])
LogicalTableScan(table=[[druid, numfoo]])
Expand Down Expand Up @@ -98,6 +127,18 @@ SELECT COUNT(1)FILTER(WHERE l1=0),COUNT(1)FILTER(WHERE l1!=0),MIN(l2) is null,

!ok

-- all rows match the condition, but all matching values are null (control: rewrite off,
-- so the pre-rewrite expression is evaluated natively and both columns stay NULL)
SELECT SUM(CASE WHEN l1 IS NULL THEN l2 ELSE 0 END),SUM(l2) FILTER(WHERE l1 IS NULL) FROM numfoo WHERE l1 IS NULL;
+--------+--------+
| EXPR$0 | EXPR$1 |
+--------+--------+
| | |
+--------+--------+
(1 row)

!ok

-- 7=7,null
SELECT COUNT(1)FILTER(WHERE l1=7),COUNT(1)FILTER(WHERE l1!=7),MIN(l2) is null,
SUM(CASE WHEN l1 = 7 THEN l2 ELSE 0 END),SUM(l2) FILTER(WHERE l1=7) FROM numfoo where 0 < l1 and l1 < 10;
Expand Down
Loading