Conversation
…ete batches Three stacked bugs in the fast AGGREGATE_GROUP path for LEFT JOIN pipelines (customer LEFT JOIN orders LEFT JOIN lineitem style views) caused wrong preserved-side aggregate counts (e.g. COUNT(o.o_orderkey)) when multiple tables changed together in one refresh: - BuildLeftJoinSecondaryDeltaSQL fired its stale-dangling-row correction even for preserved-side rows that were brand new in the same batch, spuriously subtracting a row that was never materialized. Gated the correction on the preserved row having existed before this batch (old_pres_count > 0). - GuardKeptOuterJoinsForMask's transitioning-key exclusion treated upward and downward match-count transitions the same way. Only downward (delete) transitions are a phantom-row risk under classical inclusion-exclusion; upward (insert) transitions are the join's sole correct contribution and must not be excluded. Restricted the filter to old>0 AND new=0. - MERGE gating (refresh_compiler.cpp) incorrectly gated preserved-side COUNT columns by inner-side match_count; fixed via preserved_side_cols. Verified with minimal insert/delete-direction repros, a full SF1 bisection matrix over customer/orders/lineitem delta combinations, the existing pipeline regression test, the full test suite (zero regressions), and a 20-seed randomized SF1 differential against full recompute. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nerator The regression test for the LEFT JOIN pipeline secondary-delta bugs was written during debugging but never committed. Adds it to the repo and extends it with two batches covering the two bugs fixed in 287b17d, neither of which the original batches exercised: - A preserved-side row that is brand new in the SAME batch as its first inner row (no dangling row was ever materialized, so the correction must not fire), plus the same-batch churn case where the row is inserted and deleted again. - An order's last line and the order itself deleted together (the downward transition double-count), alongside an upward transition that must survive. Verified the new batches have teeth: reverting the direction fix in BuildTransitioningKeySetImpl makes the test fail; restoring it passes. Also hardens BuildLeftJoinSecondaryDeltaSQL: - Bail when the deepest join is self-referencing (preserved and inner sides are the same base table); the old-count arithmetic assumes two distinct tables with independent delta tracking. - Match hidden helper columns by exact name / prefix instead of substring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The secondary-delta INSERT corrects the delta-arithmetic MERGE emitted by CompileAggregateGroups. It was prepended unconditionally, including when that function falls back to recomputing affected groups from the base tables (openivm_left_join_merge=false, real MIN/MAX, a non-summable column, or derived-aggregate orphans). On the recompute path the corrected values are recomputed from the base tables anyway, so the extra delta rows only widen the affected-group set. CompileAggregateGroups now reports which path it took via a new out_used_group_recompute out-param, and refresh_sql.cpp prepends the secondary only when the MERGE was actually emitted. Reporting the path is more robust than testing the caller-visible conditions, since three independent conditions inside the function can select recompute. This is scoping, not a bug fix: no observed wrong-result case is attributable to emitting the secondary on the recompute path, and the added test section passes with or without the guard. It is committed for correct scoping only, and the comments say so. Also adds pipeline coverage for openivm_left_join_merge=false (the existing left_join.test coverage of that flag uses a single LEFT JOIN, which never generates secondary metadata) and for switching back to the MERGE path afterwards. Note recorded in the test: this does NOT cover the duplicate-key refresh failure seen on the group-recompute path at TPC-H scale. That failure is pre-existing -- it reproduces on 7697928, before any of the secondary-delta work -- and is tracked separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…atabases PRAGMA refresh() of an AGGREGATE_GROUP view failed outright with "Constraint Error: Duplicate key ... violates unique constraint" whenever the group-recompute branch ran against a file-backed database, leaving the MV stale. Reachable via openivm_left_join_merge=false, openivm_refresh_mode='full', real MIN/MAX, or a non-summable column. Root cause is upstream, in DuckDB's on-disk unique index: within one transaction it keeps deleted keys for constraint checking even though the delete is visible to queries. Reproducible with no openivm involved -- -- session 1, on-disk db CREATE TABLE data(k INT, v INT); INSERT INTO data SELECT i, i*10 FROM range(25) t(i); CREATE UNIQUE INDEX didx ON data(k); -- session 2, reopen the file BEGIN; DELETE FROM data; -- count(*) returns 0 INSERT INTO data SELECT i, i*99 FROM range(25) t(i); -- Duplicate key "k: 0" COMMIT; Group-recompute deletes and re-inserts every surviving group in one refresh transaction, so it trips this on exactly the views parser.cpp gives a unique index (AGGREGATE_GROUP / AGGREGATE_HAVING). BuildAffectedKeyRefreshSQL gains an opt-in upsert form: materialize the recomputed rows for the affected groups once into a temp table (the recompute is the expensive part), delete only the affected groups that no longer produce a row, then INSERT OR REPLACE the survivors. The deleted and inserted key sets are disjoint, so no key is deleted and re-inserted in the same transaction. Gated on index_delta_view_catalog_entry being non-null, since INSERT OR REPLACE requires a UNIQUE/PK constraint and the index-less paths (DuckLake, type-6 group-recompute, window) already work. The test uses `load` + `restart`: a file-backed database with a disk-loaded index is REQUIRED to reproduce. The rest of the suite runs in-memory, where the index is built in-session and the bug cannot fire -- which is why this went unnoticed. Verified the test fails without the gate and passes with it, and that a group which disappears entirely is still removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8547913 fixed the group-recompute branch but missed a second path with the same root cause: BuildFullRecomputeSQL emits `DELETE FROM data; INSERT INTO data <query>;`, which re-inserts every key deleted earlier in the same transaction. On a persistent database DuckDB's on-disk unique index still reports those keys as present, so refresh fails with "Duplicate key ... violates unique constraint". Reproduced via the interrupted-refresh recovery route: Warning: recovering 'mv' from interrupted refresh via full recompute. Executor Error: ... Constraint Error: Duplicate key "g: 0" violates unique constraint. BuildFullRecomputeSQL gains the same opt-in upsert form: materialize the new contents once, delete only the keys that are gone, then INSERT OR REPLACE the rest, so no key is deleted and re-inserted in one transaction. Applied in BuildRecomputeQuery for AGGREGATE_GROUP / AGGREGATE_HAVING, the view types parser.cpp gives a unique index; every other caller keeps the plain form, since INSERT OR REPLACE requires a UNIQUE/PK constraint. The test gains recovery-path coverage, including a group that disappears entirely. Note the added `restart` calls: once a session has modified the index the bug stops firing, so without a fresh reopen immediately before each recovery refresh the section passed vacuously (verified -- it passed with the fix disabled until the restarts were added, and fails with them). auto_refresh.test's execute_refresh_sql_stmt count goes 4 -> 6: the full-recompute path now emits four statements instead of two. That assertion counts generated statements, not view contents; every correctness assertion in that test is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 3-table LEFT JOIN chain over DuckLake tables could not be maintained correctly. Two distinct problems, both invisible to the existing tests because left_join_pipeline_secondary_delta.test only covers regular tables: 1. The generated secondary-delta INSERT did not qualify its target. A DuckLake-backed view's delta table is dl.main.openivm_delta_<view>, so the unqualified INSERT failed the whole refresh. Threads internal_catalog_prefix through to the generator. 2. More fundamentally, the secondary read openivm_delta_<source> tables, which DuckLake does not have -- it tracks changes via snapshot time travel. Before the secondary existed this shape was SILENTLY WRONG (an order that lost its last line dropped out of the preserved-side COUNT); once generated it turned into a hard refresh failure. The two delta row sources are now placeholders in the stored SQL, resolved at refresh time to whichever backend the source uses: openivm_delta_<table> filtered by timestamp for a regular table, or ducklake_table_insertions / ducklake_table_deletions between the last-refreshed and current snapshot for a DuckLake table. Snapshot IDs only exist at refresh, whereas the SQL is generated once at CREATE, which is why a placeholder is needed rather than a literal. Both substitutions yield (__k, __m) so the rest of the SQL is backend-agnostic. LeftJoinSecondaryMeta carries the two sides' table/key identities so refresh can pick the right source. Note openivm_delta_tables keys DuckLake sources by their BARE table name but regular sources by openivm_delta_<table>; the lookup probes the bare name first to tell them apart. Getting that wrong is what made the first attempt still emit a regular-table delta reference. If a row source cannot be resolved (missing snapshot metadata) the secondary is skipped rather than emitted with placeholders intact, which would be a syntax error. The correlated LATERAL form is kept deliberately: rewriting those subqueries as pre-aggregated CTEs restricted to the delta keys measured ~40% SLOWER at TPC-H SF50, since DuckDB already decorrelates them well. New test covers the DuckLake pipeline across three batches (inner-side transitions both directions, preserved side changing, and an order losing its last line while being deleted in the same batch). Verified it fails when the DuckLake branch is disabled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ains A chain of N tables has N-1 LEFT JOIN levels, and a match disappearing at ANY level can strand a preserved-side count. Only the outermost level was corrected, so 3-table pipelines were right but deeper ones were not: with cust ⟕ ord ⟕ line ⟕ supp, deleting an order's last line dropped that order from COUNT(o.oid) because the `⟕ line` level had no correction. Reproduced at 4 and 5 tables on both regular and DuckLake sources, and at TPC-H SF50 where the 4-table shape was wrong in all 18 benchmark scenarios on both engines. BuildLeftJoinSecondaryDeltaSQL now walks facts.comparison_joins and emits a fragment for every LEFT JOIN whose preserved side is itself a join (the innermost level still needs none -- the primary delta already emits its null-padded reappearance). Placeholders are indexed per level and the two source identities per level are stored as index-aligned CSV lists, so refresh resolves each level independently against whichever backend that source uses. The per-level aggregate contributions need a transitive NULL closure, not a comparison against a single inner table: in cust ⟕ ord ⟕ line ⟕ supp, when a line disappears the supplier joined to that line's key disappears too, and anything joined past it. ComputeNullTablesForLevel starts from the level's inner subtree and closes over any join whose condition touches an already-NULL table. If any level is unsupported the whole correction is skipped rather than emitted partially -- a partial correction would double-count the levels it does cover. New test covers 4- and 5-table chains in both transition directions; verified it fails when restricted to the outermost level only. An apparent residual undercount at SF50 turned out to be an artifact of a benchmark database carrying nine rounds of synthetic mutations: on freshly generated SF50 data the same shape verifies exactly, including a follow-up multi-table batch with fake-key inserts and order deletions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refresh failed outright with Binder Error: Values list "X" does not have a column named "__k" for any LEFT JOIN pipeline aggregate that groups by a column the deepest join also joins on -- a common star-schema shape (GROUP BY the dimension key you join on). Reproducible with the minimum pipeline: 3 tables, 2 LEFT JOINs, GROUP BY the join key. The generator aliases the preserved-side join key "__k" and each group column "__g<i>", but an output column can only carry one alias. When the key IS a group column the group alias overwrote "__k", so no "__k" column was emitted while the surrounding SQL still referenced X."__k". Now the alias the key actually received is tracked and used, instead of assuming "__k". Verified the test fails when the reference is put back to a literal "__k". With the fix, star-schema aggregates grouped by the join key refresh correctly at 2, 3 and 5 joins, including a dimension-side change at the deepest level (which is what drives the secondary delta). Not fixed here, and deliberately not asserted by the test: when a group's LAST preserved-side row is deleted the MV keeps a zeroed row instead of dropping the group. That is pre-existing and independent of this bug -- it reproduces on an unaffected shape (GROUP BY a non-join column, where no alias collision is possible), so it needs its own investigation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… deleted A LEFT JOIN aggregate MV never removed a group after its last preserved-side row was deleted: the group lingered as a zeroed row (e.g. `B | 0 | 0`) where a full recompute drops it. Reproduces with a SINGLE left join, so it is unrelated to the pipeline/secondary-delta work. Plain and INNER-join aggregates were unaffected -- only views carrying openivm_match_count. The empty-group cleanup in CompileAggregateGroups was gated `!has_match_count`, skipping every LEFT JOIN view. The stated reason was sound (a NULL-padded group legitimately has inner-side COUNT = 0 and would look empty) but the remedy was too blunt. The correct discriminator is the OUTPUT-row count: a NULL-padded group has count_star >= 1, an emptied one has 0. Because the delete predicate ANDs every count column, including count_star is enough to protect NULL-padded groups. Enabling that alone deletes VALID groups, because count_star was not actually maintained as an output-row count. Three parts were needed: 1. Emit the secondary delta at EVERY LEFT JOIN level, including a single left join whose preserved side is a bare table. The old comment claimed the primary delta emits the null-padded reappearance there; it does not -- the primary emits only a retraction of the matched row and the MERGE's gating synthesises the visible NULL/0 values, leaving count_star at 0 for a group that still has an output row. 2. Recognise the hidden openivm_count_star column by NAME in the MERGE. It has no col_agg_type entry, so the existing `agg_type == "count_star"` test never matched it and it fell through to the gated branch, which zeroes it on a >0 -> 0 match transition. 3. Enable the cleanup for LEFT JOIN views, but only when a count_star-type column anchors the predicate; without an anchor it still skips, so a view that cannot distinguish the two cases is never at risk. Each part is load-bearing: reverting any one of the three makes the new test fail. Covers a group losing all matches but keeping its row, a group disappearing entirely, an already-unmatched group being removed, a group coming back, and the same lifecycle in a 3-table pipeline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`make tidy-check` failed on seven pre-existing findings (introduced in 1429ff6 and 6b735e2, both ancestors of the current LEFT JOIN work). All are mechanical: - performance-move-const-arg, 3 sites in refresh_compiler.cpp: BuildNullableSum takes `const string &`, so std::move on its arguments never moves. - readability-container-size-empty, 4 sites in parser.cpp: INVALID_CATALOG and INVALID_SCHEMA are both defined as "", so comparing against them is an emptiness check — spelled with .empty() now. - google-runtime-int, refresh_sql.cpp: cast to idx_t rather than `unsigned long long` for the %llu argument, matching the convention already used elsewhere (e.g. src/delta/operators/join.cpp). No behaviour change. tidy-check and format-check both clean; suite unchanged at 18372 assertions / 164 test cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Consolidates the LEFT JOIN correctness and performance work into one document: the goal and its two tracks, the file map, all nine bug fixes with root causes and repros, the clean benchmark numbers for DuckLake and standard IVM, the five optimisation candidates that were tried and refuted (with the measurements that refuted them), the methodology rules that emerged, the production query shapes Raki described, the open items in priority order, and the traps that cost real time — contamination, per-session DuckLake metadata, the in-memory suite being unable to catch on-disk bugs, and dirty benchmark databases faking correctness regressions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors OpenIVM’s delta and refresh pipeline while fixing the correctness, transactionality, and scaling issues uncovered during the audit.
The main outcomes are:
ON CONFLICT,MERGE, defaults, generated columns, rollback, andRETURNING;Correctness and lifecycle changes
openivm_viewscannot be mistaken for metadata DDL.Delta and compiler changes
Tests
The regression coverage uses real materialized views and bidirectional
EXCEPT ALLchecks. New/expanded cases include:Final EC2 verification:
Compatibility and follow-ups
docs/codebase-audit-2026-07-22.md.left_join_emptied_groupresults.