FrankChen021 commented on code in PR #20093:
URL: https://github.com/apache/druid/pull/20093#discussion_r4144561713
##########
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] 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.
##########
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(
+ new ExpressionPostAggregator(
+ name,
+ "nvl(\"" + fieldName + "\", 0)",
Review 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.
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumZeroSqlAggregator.java:
##########
@@ -19,8 +19,18 @@
package org.apache.druid.sql.calcite.aggregation.builtin;
+import com.google.common.collect.Iterables;
Review 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.
##########
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] 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.
--
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]
