Skip to content

Commit 1429ff6

Browse files
committed
Preserve NULL semantics for incremental SUM
1 parent 16b0098 commit 1429ff6

11 files changed

Lines changed: 622 additions & 16 deletions
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
-- {"operators": "AGGREGATE,CASE", "complexity": "medium", "is_incremental": true, "has_nulls": true, "has_cast": false, "has_case": true, "tables": "ORDER_LINE", "ducklake": true}
2+
SELECT OL_W_ID, OL_D_ID, OL_O_ID, OL_NUMBER,
3+
SUM(CASE WHEN OL_DELIVERY_D IS NULL THEN OL_AMOUNT ELSE NULL END) AS pending_amount
4+
FROM dl.ORDER_LINE
5+
GROUP BY OL_W_ID, OL_D_ID, OL_O_ID, OL_NUMBER;

docs/codebase-audit-2026-07-22.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ maintenance path. A bug must not be hidden by weakening a test or silently chang
3232
`IS NOT DISTINCT FROM` predicates for standard, cascade, running-window, and DuckLake paths. Regression coverage includes
3333
batched conflicting DML on single and composite NULL partitions plus a DuckLake TPCC window whose delivery timestamp moves
3434
from the NULL partition to a non-NULL partition.
35-
- [ ] **Preserve `SUM` NULL semantics.** Weighted SUM alone cannot distinguish numeric zero from no non-NULL inputs. Persist
35+
- [x] **Preserve `SUM` NULL semantics.** Weighted SUM alone cannot distinguish numeric zero from no non-NULL inputs. Persist
3636
a hidden `COUNT(sum_argument)` and render NULL when that count reaches zero.
3737
- [ ] **Use an unconditional row count for group existence.** `COUNT(nullable_expression)=0` does not mean a group is empty.
3838
Persist a hidden `COUNT(*)` solely for deciding whether the group row must be deleted.
@@ -122,6 +122,9 @@ maintenance path. A bug must not be hidden by weakening a test or silently chang
122122
base-leaf appearances, compilation work, and generated SQL size.
123123
- [ ] **Remove the fake `ConstraintCache` or make it a cache.** It repopulates a map that no read path consumes, while FK cost
124124
estimation separately parses textual constraint descriptions.
125+
- [ ] **Audit edge-case tests and mirror them in the rewriter benchmark.** Review the SQL harness for missing boundary and
126+
transition cases, add deterministic bidirectional `EXCEPT ALL` coverage, and add representative cases to the rewriter
127+
benchmark so real refresh compilation and execution exercise the same semantics.
125128

126129
## Target refresh flow
127130

src/core/ivm_view_classifier.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -418,9 +418,10 @@ static void BuildGroupColumns(DeltaViewModel &model, const CreateMVPlanFacts &fa
418418
model.group_columns = DeriveGroupColumnNames(facts, group_index, group_count, output_names);
419419
}
420420

421+
idx_t actual_visible_outputs = GetVisibleOutputCount(output_names, visible_output_count);
421422
if (analysis.found_delim_join && analysis.found_aggregation && model.group_columns.empty() &&
422-
output_names.size() > analysis.aggregate_types.size()) {
423-
idx_t key_count = output_names.size() - analysis.aggregate_types.size();
423+
actual_visible_outputs > analysis.aggregate_types.size()) {
424+
idx_t key_count = actual_visible_outputs - analysis.aggregate_types.size();
424425
for (idx_t i = 0; i < key_count; i++) {
425426
if (!output_names[i].empty() && !IncrementalTableNames::IsInternalColumn(output_names[i])) {
426427
model.group_columns.push_back(output_names[i]);
@@ -507,11 +508,11 @@ static void BuildGroupColumns(DeltaViewModel &model, const CreateMVPlanFacts &fa
507508

508509
if (analysis.found_join && analysis.found_aggregation && !model.group_columns.empty()) {
509510
idx_t expected_linear_outputs = model.group_columns.size() + model.aggregate_types.size();
510-
if (output_names.size() > expected_linear_outputs) {
511+
if (actual_visible_outputs > expected_linear_outputs) {
511512
AddUnique(model.strategy_reasons, DeltaStrategyReason::JOIN_AGGREGATE_PROJECTION_FALLBACK);
512513
OPENIVM_DEBUG_PRINT("[CREATE MV] Join-over-aggregate exposes %zu columns but only %zu are "
513514
"group/aggregate outputs -- using GROUP_RECOMPUTE\n",
514-
output_names.size(), expected_linear_outputs);
515+
actual_visible_outputs, expected_linear_outputs);
515516
}
516517
}
517518

src/core/parser.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "core/parser_ddl.hpp"
77
#include "core/parser_plan_helpers.hpp"
88
#include "core/parser_sql_extractors.hpp"
9+
#include "core/plan_rewrite_internal.hpp"
910
#include "core/refresh_locks.hpp"
1011
#include "core/refresh_metadata.hpp"
1112
#include "core/ivm_delta_model.hpp"
@@ -400,6 +401,12 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC
400401
// Strip HAVING filter from plan — data table stores all groups.
401402
// The predicate is extracted as SQL (using output aliases) for the VIEW WHERE clause.
402403
having_predicate = StripHavingFilter(select_plan, output_names);
404+
// HAVING-only aggregates are exposed by StripHavingFilter. Inject SUM's
405+
// non-NULL state afterward so visible, wrapped, and HAVING-only SUMs all
406+
// use the same output-index mapping during incremental maintenance.
407+
InjectSumNonNullCounts(context, select_plan);
408+
PropagateHiddenAggregateColumns(select_plan);
409+
output_names = PrepareOutputNames(select_plan.get(), select_planner.names);
403410
auto post_rewrite_facts = BuildCreateMVPlanFacts(select_plan.get(), current_catalog);
404411
stored_query_has_aggregate_filter = post_rewrite_facts.has_filter_above_aggregate;
405412
has_hidden_minmax_having = post_rewrite_facts.has_hidden_minmax_having_column;

src/core/plan_rewrite.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,8 @@ static void InjectGroupCountStar(unique_ptr<LogicalOperator> &plan) {
939939
/// projection's expression up through a pass-through parent projection.
940940
static bool IsHiddenAggregateAlias(const string &alias) {
941941
return alias.find(openivm::SUM_COL_PREFIX) == 0 || alias.find(openivm::COUNT_COL_PREFIX) == 0 ||
942-
alias.find(openivm::SUM_SQ_COL_PREFIX) == 0 || alias.find(openivm::SUM_SQP_COL_PREFIX) == 0;
942+
alias.find(openivm::SUM_SQ_COL_PREFIX) == 0 || alias.find(openivm::SUM_SQP_COL_PREFIX) == 0 ||
943+
alias.find(openivm::SUM_COUNT_COL_PREFIX) == 0;
943944
}
944945

945946
/// Propagate hidden aggregate columns (openivm_sum_*, openivm_count_*, …) added by
@@ -955,7 +956,7 @@ static bool IsHiddenAggregateAlias(const string &alias) {
955956
/// data table stores only the final AVG and the MERGE computes `v.avg + d.avg`
956957
/// — wrong for non-summable aggregates. Propagation lets CompileAggregateGroups
957958
/// see the hidden SUM/COUNT columns and maintain them separately.
958-
static void PropagateHiddenAggregateColumns(unique_ptr<LogicalOperator> &plan) {
959+
void PropagateHiddenAggregateColumns(unique_ptr<LogicalOperator> &plan) {
959960
for (auto &child : plan->children) {
960961
PropagateHiddenAggregateColumns(child);
961962
}

src/core/plan_rewrite_aggregates.cpp

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "duckdb/optimizer/optimizer.hpp"
1010
#include "duckdb/planner/expression/bound_aggregate_expression.hpp"
1111
#include "duckdb/planner/expression/bound_case_expression.hpp"
12+
#include "duckdb/planner/expression/bound_cast_expression.hpp"
1213
#include "duckdb/planner/expression/bound_columnref_expression.hpp"
1314
#include "duckdb/planner/expression/bound_comparison_expression.hpp"
1415
#include "duckdb/planner/expression/bound_constant_expression.hpp"
@@ -134,6 +135,137 @@ LogicalOperator *FindProjectionAggregateInput(unique_ptr<LogicalOperator> &plan,
134135
}
135136
return nullptr;
136137
}
138+
139+
void InjectSumNonNullCounts(ClientContext &context, unique_ptr<LogicalOperator> &plan) {
140+
if (!plan) {
141+
return;
142+
}
143+
144+
// Find the linear output path down to the aggregate. ORDER BY/LIMIT/TOP_N
145+
// and MATERIALIZED_CTE are pass-through wrappers; projections may rename or
146+
// cast aggregate outputs and therefore need to be followed explicitly.
147+
vector<LogicalProjection *> projections;
148+
LogicalOperator *node = plan.get();
149+
LogicalOperator *agg_search = nullptr;
150+
while (node) {
151+
switch (node->type) {
152+
case LogicalOperatorType::LOGICAL_ORDER_BY:
153+
case LogicalOperatorType::LOGICAL_LIMIT:
154+
case LogicalOperatorType::LOGICAL_TOP_N:
155+
case LogicalOperatorType::LOGICAL_DISTINCT:
156+
node = node->children.empty() ? nullptr : node->children[0].get();
157+
continue;
158+
case LogicalOperatorType::LOGICAL_MATERIALIZED_CTE:
159+
node = node->children.size() < 2 ? nullptr : node->children[1].get();
160+
continue;
161+
case LogicalOperatorType::LOGICAL_PROJECTION:
162+
projections.push_back(&node->Cast<LogicalProjection>());
163+
node = node->children.empty() ? nullptr : node->children[0].get();
164+
continue;
165+
case LogicalOperatorType::LOGICAL_FILTER:
166+
if (!node->children.empty() &&
167+
node->children[0]->type == LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY) {
168+
agg_search = node->children[0].get();
169+
}
170+
node = nullptr;
171+
continue;
172+
case LogicalOperatorType::LOGICAL_AGGREGATE_AND_GROUP_BY:
173+
agg_search = node;
174+
node = nullptr;
175+
continue;
176+
default:
177+
node = nullptr;
178+
continue;
179+
}
180+
}
181+
if (!agg_search || projections.empty()) {
182+
return;
183+
}
184+
185+
auto &agg = agg_search->Cast<LogicalAggregate>();
186+
auto &aggregate_projection = *projections.back();
187+
auto &output_projection = *projections.front();
188+
const idx_t original_aggregate_count = agg.expressions.size();
189+
190+
struct SumCountOutput {
191+
idx_t projection_index;
192+
idx_t aggregate_index;
193+
};
194+
vector<SumCountOutput> outputs;
195+
for (idx_t projection_index = 0; projection_index < output_projection.expressions.size(); projection_index++) {
196+
Expression *resolved = output_projection.expressions[projection_index].get();
197+
while (resolved) {
198+
while (resolved->expression_class == ExpressionClass::BOUND_CAST) {
199+
resolved = resolved->Cast<BoundCastExpression>().child.get();
200+
}
201+
if (resolved->type != ExpressionType::BOUND_COLUMN_REF) {
202+
break;
203+
}
204+
auto &ref = resolved->Cast<BoundColumnRefExpression>();
205+
if (ref.binding.table_index == agg.aggregate_index) {
206+
break;
207+
}
208+
LogicalProjection *referenced_projection = nullptr;
209+
for (idx_t projection_depth = 1; projection_depth < projections.size(); projection_depth++) {
210+
if (projections[projection_depth]->table_index == ref.binding.table_index) {
211+
referenced_projection = projections[projection_depth];
212+
break;
213+
}
214+
}
215+
if (!referenced_projection || ref.binding.column_index >= referenced_projection->expressions.size()) {
216+
resolved = nullptr;
217+
break;
218+
}
219+
resolved = referenced_projection->expressions[ref.binding.column_index].get();
220+
}
221+
if (!resolved || resolved->type != ExpressionType::BOUND_COLUMN_REF) {
222+
continue;
223+
}
224+
auto &ref = resolved->Cast<BoundColumnRefExpression>();
225+
if (ref.binding.table_index != agg.aggregate_index || ref.binding.column_index >= original_aggregate_count) {
226+
continue;
227+
}
228+
auto &aggregate_expr = agg.expressions[ref.binding.column_index];
229+
if (aggregate_expr->expression_class != ExpressionClass::BOUND_AGGREGATE) {
230+
continue;
231+
}
232+
auto &sum = aggregate_expr->Cast<BoundAggregateExpression>();
233+
if (sum.function.name != "sum" || sum.IsDistinct() || sum.children.size() != 1) {
234+
continue;
235+
}
236+
if (sum.alias.find(string(openivm::SUM_COL_PREFIX)) == 0 ||
237+
sum.alias.find(string(openivm::VAR_SQ_COL_PREFIX)) == 0 ||
238+
sum.alias.find(string(openivm::VAR_SQP_COL_PREFIX)) == 0) {
239+
continue;
240+
}
241+
242+
auto count_func = BindAggregateByName(context, "count", {sum.children[0]->return_type});
243+
vector<unique_ptr<Expression>> count_args;
244+
count_args.push_back(sum.children[0]->Copy());
245+
auto count_expr = make_uniq<BoundAggregateExpression>(std::move(count_func), std::move(count_args), nullptr,
246+
nullptr, AggregateType::NON_DISTINCT);
247+
count_expr->alias = string(openivm::SUM_COUNT_COL_PREFIX) + to_string(projection_index);
248+
outputs.push_back({projection_index, agg.expressions.size()});
249+
agg.expressions.push_back(std::move(count_expr));
250+
}
251+
if (outputs.empty()) {
252+
return;
253+
}
254+
255+
agg.ResolveOperatorTypes();
256+
auto aggregate_bindings = agg_search->GetColumnBindings();
257+
auto aggregate_types = agg_search->types;
258+
for (auto &output : outputs) {
259+
idx_t output_index = agg.groups.size() + output.aggregate_index;
260+
auto count_ref =
261+
make_uniq<BoundColumnRefExpression>(aggregate_types[output_index], aggregate_bindings[output_index]);
262+
count_ref->alias = string(openivm::SUM_COUNT_COL_PREFIX) + to_string(output.projection_index);
263+
aggregate_projection.expressions.push_back(std::move(count_ref));
264+
}
265+
aggregate_projection.ResolveOperatorTypes();
266+
OPENIVM_DEBUG_PRINT("[PlanRewrite] Injected %zu SUM non-NULL counts\n", outputs.size());
267+
}
268+
137269
void RewriteDerivedAggregates(ClientContext &context, unique_ptr<LogicalOperator> &plan, Optimizer &opt, bool is_top) {
138270
for (auto &child : plan->children) {
139271
RewriteDerivedAggregates(context, child, opt, false);

src/include/core/openivm_constants.hpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ constexpr const char *VAR_SQ_COL_PREFIX = "openivm_var_sq_"; // VARIANCE: no s
3737
constexpr const char *SUM_SQP_COL_PREFIX = "openivm_sum_sqp_"; // STDDEV_POP: sqrt + population denominator
3838
constexpr const char *VAR_SQP_COL_PREFIX = "openivm_var_sqp_"; // VAR_POP: no sqrt + population denominator
3939
constexpr const char *COUNT_COL_PREFIX = "openivm_count_";
40+
// COUNT(sum_argument) companions for user-visible SUM outputs. The suffix is
41+
// the visible projection index, which associates the state with the exact
42+
// bound output without inspecting SQL text or aliases.
43+
constexpr const char *SUM_COUNT_COL_PREFIX = "openivm_nonnull_sum_count_";
4044

4145
// Hidden COUNT(*) injected into AGGREGATE_GROUP MVs that don't already have a
4246
// count aggregate. Tracks per-group cardinality so the cleanup can delete rows

src/include/core/plan_rewrite_internal.hpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ AggregateFunction BindAggregateByName(ClientContext &context, const string &name
1111
LogicalOperator *FindProjectionAggregateInput(unique_ptr<LogicalOperator> &plan, bool allow_having_filter);
1212
void RewriteDerivedAggregates(ClientContext &context, unique_ptr<LogicalOperator> &plan, Optimizer &opt,
1313
bool is_top = true);
14+
void InjectSumNonNullCounts(ClientContext &context, unique_ptr<LogicalOperator> &plan);
15+
void PropagateHiddenAggregateColumns(unique_ptr<LogicalOperator> &plan);
1416
bool RewriteSafeSemiAntiDelimGets(ClientContext &context, unique_ptr<LogicalOperator> &plan);
1517

1618
} // namespace duckdb

0 commit comments

Comments
 (0)