FrankChen021 commented on code in PR #20093:
URL: https://github.com/apache/druid/pull/20093#discussion_r4133458934
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SimpleSqlAggregator.java:
##########
@@ -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);
Review 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.
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.java:
##########
@@ -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(
Review 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.
--
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]
