Repository navigation
fix: use SUM0 in AggregateCaseToFilter D1 rewrite to avoid NULL on empty windows - #20093
zhang-arvin wants to merge 5 commits into
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 2 of 2 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| RelDataType newType = typeFactory.createTypeWithNullability(call.getType(), false); | ||
| return AggregateCall.create( | ||
| call.getAggregation(), | ||
| SqlStdOperatorTable.SUM0, |
There was a problem hiding this comment.
[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.
| RelDataType newType = typeFactory.createTypeWithNullability(call.getType(), false); | ||
| return AggregateCall.create( | ||
| call.getAggregation(), | ||
| SqlStdOperatorTable.SUM0, |
There was a problem hiding this comment.
[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.
…pty windows The D1 rewrite in DruidAggregateCaseToFilterRule converts SUM(CASE WHEN COND THEN COL1 ELSE 0 END) to SUM(COL1) FILTER(WHERE COND), but SUM returns NULL when the filter never matches instead of the expected 0. This fix changes the D1 rewrite to use SUM0 instead of SUM, which returns 0 for empty/null input, matching the original CASE expression behavior. Fixes apache#18058, apache#17768
2b91fb6 to
cacd9fb
Compare
|
can you add some test cases |
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The incremental baseline recorded in the packet was unavailable, so this review explicitly covered the full current merge-base diff. The packet contained prior severity counts but no inline finding text; the relevant surrounding aggregation, rewrite, filtering, and window-lifecycle paths were rechecked directly.
Reviewed 3 of 3 changed files:
- sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.java — filtered and unfiltered SUM0 aggregation construction, post-aggregation behavior, and factory naming.
- sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java — D1 rewrite operator and result nullability.
- sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/filtered_sum.iq — enabled and disabled rewrite expectations and logical plans.
The recommendation is changes because the current implementation changes a nullable result for all-null matching inputs and introduces a planning regression for filtered SUM0 window aggregates.
Validation: git diff --check 5b1bcc1 cacd9fb passed. No builds, tests, dependency installs, or formatters were run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| ImmutableList.of(innerFactory), | ||
| new ExpressionPostAggregator( | ||
| name, | ||
| "nvl(\"" + innerName + "\", 0)", |
There was a problem hiding this 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.
| return null; | ||
| } | ||
|
|
||
| if (aggregateCall.filterArg >= 0) { |
There was a problem hiding this 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.
Checkstyle's NewlineAtEndOfFile rule rejects files not ending with a newline, which failed the static-checks, strict-compilation, packaging-check and openrewrite jobs.
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The incremental diff from the previous reviewed SHA cacd9fb92ab9e82e3c4dbdc2bee9ce49a60fbf44 to the current head 2796511e0e64fae3344bc8e68d6c445252d4e48c only adds the missing EOF newline; no prior behavioral finding is resolved. I reviewed the full current merge-base diff and rechecked the changed aggregation and rewrite code against the native filtered-aggregation contract, DruidOperatorTable registration, window translation contract, and the enabled/disabled Quidem expectations.
Reviewed 3 of 3 changed files:
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.javasql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.javasql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/filtered_sum.iq
Two current P2 findings remain: the new nvl changes the all-null matching result from NULL to 0, and the post-aggregator makes filtered SUM0 window aggregates unsupported by the existing window translator. No builds, tests, dependency installs, or formatters were run; validation was static only.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| ImmutableList.of(innerFactory), | ||
| new ExpressionPostAggregator( | ||
| name, | ||
| "nvl(\"" + innerName + "\", 0)", |
There was a problem hiding this 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.
| return null; | ||
| } | ||
|
|
||
| if (aggregateCall.filterArg >= 0) { |
There was a problem hiding this 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.
|
Thanks @FrankChen021. Findings addressed in 98922e1, one by one: 1. 2. 3. "can you add some test cases" — not done in this push. 🤖 Generated by an AI agent (zhang-arvin). |
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The current head still needs changes: the new guard does not identify D1 rewrites, and the guarded branch removes the aggregate factory required to compute and filter the sum. Fix both before merging; the ordinary top-level no-match case still follows the nullable native sum path, while affected finalized subqueries can fail planning.
Reviewed 8 of 8 changed files, starting from the incremental diff and then checking the full merge-base diff:
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MaxSqlAggregator.javasql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MinSqlAggregator.javasql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.javasql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SingleValueSqlAggregator.javasql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.javasql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.javasql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.javasql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/filtered_sum.iq
Also traced the finalization callers, GroupByRules filter application and reuse checks, Aggregation input validation, Windowing translation, and nullable native aggregation behavior. The earlier window post-aggregator rejection is no longer triggered on the normal window path, because Windowing passes false; this is not evidence that post-aggregator-only window aggregation is supported. The all-null test discussion also needs correction: the existing 7=7,null case explicitly has MIN(l2) IS NULL = true, and its rewrite-disabled expectation remains NULL. The new 0 expectation does not demonstrate preserved CASE semantics.
Static review only; no builds, tests, dependency installs, or formatters were run. No additional concurrency or lifecycle concern was identified in these planner changes.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 2 |
| P3 | 0 |
| Total | 2 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| // 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); |
There was a problem hiding this comment.
[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.
| // 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( |
There was a problem hiding this comment.
[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.
|
Added the two test cases requested in
These were not executed locally (offline Druid build is unavailable here), so the expected values are derived from the code and the existing assertions in the same file; CI is the verification. Generated by an AI agent. |
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The current head still needs changes. The incremental commit adds tests but does not resolve the prior planner defects: finalization mode is being used as the D1-rewrite marker, and the finalized path drops the aggregate factory before the rewritten filter is applied. The null-to-zero post-aggregator also changes the original CASE/SUM result for all-null matching rows, and the added unused imports will fail the repository's UnusedImports check.
Reviewed 8 of 8 changed files, starting with the incremental test diff and then the full current merge-base diff. The reviewed files were:
- sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MaxSqlAggregator.java
- sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/MinSqlAggregator.java
- sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.java
- sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SingleValueSqlAggregator.java
- sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.java
- sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.java
- sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java
- sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/filtered_sum.iq
I also inspected the surrounding GroupByRules and Aggregation.filter paths, DruidRel finalization callers, Windowing.fromCalciteStuff, nullable native sum factories, and the relevant Quidem expectations.
Validation: git diff --check for the full current PR diff passed. No builds, tests, dependency installs, or formatters were run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 3 |
| P3 | 0 |
| Total | 4 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| // 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); |
There was a problem hiding this comment.
[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.
| // 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( |
There was a problem hiding this comment.
[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.
| return Aggregation.create( | ||
| new ExpressionPostAggregator( | ||
| name, | ||
| "nvl(\"" + fieldName + "\", 0)", |
There was a problem hiding this comment.
[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.
|
|
||
| package org.apache.druid.sql.calcite.aggregation.builtin; | ||
|
|
||
| import com.google.common.collect.Iterables; |
There was a problem hiding this comment.
[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.
Description
Fixes #18058 and #17768.
The
DruidAggregateCaseToFilterRuleD1 rewrite convertsSUM(CASE WHEN COND THEN COL1 ELSE 0 END)toSUM(COL1) FILTER(WHERE COND). However,SUMreturns NULL when the filter never matches, while the original CASE expression would return 0. This causes incorrect NULL results for queries with aggregations on empty windows.Root Cause
The D1 rewrite in
DruidAggregateCaseToFilterRule.transform()usescall.getAggregation()(which isSUM) when creating the filtered aggregate call.SUMreturns NULL for empty groups, but the originalSUM(CASE WHEN ... ELSE 0 END)returns 0 when no rows match the condition.Fix
Changed the D1 rewrite to use
SUM0(viaSqlStdOperatorTable.SUM0) instead ofSUM.SUM0returns 0 for empty/null input, which matches the expected behavior of the original CASE expression.Changes
DruidAggregateCaseToFilterRule.java: Changed the D1 case intransform()to useSUM0instead ofSUMfor the filtered aggregate call, with non-nullable result type.filtered_sum.iq: Updated expected test results to reflect the corrected behavior (0 instead of NULL for empty window and no-match cases).Behavior Changes
Key Features
Testing
Updated the
filtered_sum.iqquidem test to verify the corrected behavior for all four scenarios (empty input, no match, some match, all null values).