Skip to content

Commit 287b17d

Browse files
ilaclaude
andcommitted
Fix LEFT JOIN pipeline secondary-delta bugs under combined insert+delete 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>
1 parent 7697928 commit 287b17d

11 files changed

Lines changed: 638 additions & 6 deletions

src/core/parser.cpp

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,6 +1156,30 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC
11561156
BuildUpdateViewJsonSQL("semi_anti_aux_meta_json", RefreshMetadata::SemiAntiAuxMetaToJson(meta), view_name));
11571157
}
11581158

1159+
// LEFT JOIN pipeline secondary-delta (Larson & Zhou): generate the secondary-delta INSERT once at CREATE
1160+
// time and store it; refresh appends it between the primary-delta INSERT and the MERGE.
1161+
if (view_model.type == RefreshType::AGGREGATE_GROUP && facts.analysis.found_left_join) {
1162+
add_profile_marker("create_mv_leftjoin_secondary");
1163+
// LPTS on the preserved subtree needs an active transaction for catalog access; the main
1164+
// create-time LPTS ran inside a transaction that was already rolled back above.
1165+
con.BeginTransaction();
1166+
string sec_sql;
1167+
vector<string> preserved_cols;
1168+
try {
1169+
sec_sql = BuildLeftJoinSecondaryDeltaSQL(*con.context, facts, output_names, view_name, preserved_cols);
1170+
} catch (...) {
1171+
sec_sql.clear();
1172+
}
1173+
con.Rollback();
1174+
if (!sec_sql.empty()) {
1175+
RefreshMetadata::LeftJoinSecondaryMeta sm;
1176+
sm.sql = sec_sql;
1177+
sm.preserved_cols_csv = StringUtil::Join(preserved_cols, ",");
1178+
aux_metadata_ddl.push_back(BuildUpdateViewJsonSQL(
1179+
"leftjoin_secondary_meta_json", RefreshMetadata::LeftJoinSecondaryMetaToJson(sm), view_name));
1180+
}
1181+
}
1182+
11591183
const auto &source_table_info = facts.source_table_info;
11601184
const auto &dl_table_info = facts.ducklake_table_info; // keyed by lowercased name
11611185

src/core/parser_create_mv_helpers.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ void AppendCreateMVSystemTablesDDL(vector<string> &ddl, const string &view_name,
6464
AddColumnIfNotExists(ddl, openivm::VIEWS_TABLE, "count_distinct_aux_meta_json varchar default null");
6565
AddColumnIfNotExists(ddl, openivm::VIEWS_TABLE, "semi_anti_aux_meta_json varchar default null");
6666
AddColumnIfNotExists(ddl, openivm::VIEWS_TABLE, "lineage_json varchar default null");
67+
AddColumnIfNotExists(ddl, openivm::VIEWS_TABLE, "leftjoin_secondary_meta_json varchar default null");
6768
AddColumnIfNotExists(ddl, openivm::VIEWS_TABLE, "has_join boolean default null");
6869
AddColumnIfNotExists(ddl, openivm::VIEWS_TABLE, "group_recompute_affected_mode varchar default null");
6970
AddColumnIfNotExists(ddl, openivm::VIEWS_TABLE, "group_recompute_source_occurrences_json varchar default null");

src/core/parser_plan_helpers.cpp

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1969,4 +1969,243 @@ void ForwardPacSettingsIfLoaded(ClientContext &context, Connection &con) {
19691969
}
19701970
}
19711971

1972+
static bool SubtreeContainsComparisonJoin(LogicalOperator *op) {
1973+
if (!op) {
1974+
return false;
1975+
}
1976+
if (op->type == LogicalOperatorType::LOGICAL_COMPARISON_JOIN || op->type == LogicalOperatorType::LOGICAL_ANY_JOIN ||
1977+
op->type == LogicalOperatorType::LOGICAL_DELIM_JOIN) {
1978+
return true;
1979+
}
1980+
for (auto &child : op->children) {
1981+
if (SubtreeContainsComparisonJoin(child.get())) {
1982+
return true;
1983+
}
1984+
}
1985+
return false;
1986+
}
1987+
1988+
// LEFT-JOIN pipeline secondary-delta INSERT (Larson & Zhou). Returns "" if unsupported (caller falls back
1989+
// to current behavior — no regression). Fixes the deepest LEFT JOIN's preserved-side aggregate undercount
1990+
// when an inner-table change flips a join key's match-count across zero. Self-contained SQL: it re-reads the
1991+
// per-source delta timestamp via subquery, so refresh runs it verbatim (no substitution, no plan walk).
1992+
string BuildLeftJoinSecondaryDeltaSQL(ClientContext &context, const CreateMVPlanFacts &facts,
1993+
const vector<string> &output_names, const string &view_name,
1994+
vector<string> &preserved_cols) {
1995+
preserved_cols.clear();
1996+
if (facts.aggregates.size() != 1) {
1997+
return "";
1998+
}
1999+
auto *oj = facts.first_comparison_join;
2000+
if (!oj || oj->join_type != JoinType::LEFT || oj->conditions.empty() || oj->children.size() != 2) {
2001+
return "";
2002+
}
2003+
// Only PIPELINES need the secondary: for a single LEFT JOIN the primary delta already emits the
2004+
// null-padded reappearance. Require the preserved (left) side to itself contain a join.
2005+
if (!SubtreeContainsComparisonJoin(oj->children[0].get())) {
2006+
return "";
2007+
}
2008+
auto &agg = *facts.aggregates[0];
2009+
size_t G = agg.groups.size();
2010+
if (G == 0 || output_names.size() < G + agg.expressions.size()) {
2011+
return "";
2012+
}
2013+
// Deepest-join keys: condition.left = preserved side, condition.right = inner (null) side.
2014+
auto *pres_ref = GetColumnRefThroughCasts(oj->conditions[0].left.get());
2015+
auto *inner_ref = GetColumnRefThroughCasts(oj->conditions[0].right.get());
2016+
if (!pres_ref || !inner_ref) {
2017+
return "";
2018+
}
2019+
ColumnBinding key_pres = pres_ref->binding;
2020+
LogicalGet *inner_get = nullptr;
2021+
string inner_col;
2022+
if (!ResolveBindingToGetColumn(inner_ref->binding, facts, inner_get, inner_col) || !inner_get ||
2023+
!inner_get->GetTable().get()) {
2024+
return "";
2025+
}
2026+
idx_t inner_tidx = inner_get->table_index;
2027+
string inner_table = inner_get->GetTable().get()->name;
2028+
auto sti = facts.source_table_info.find(inner_table);
2029+
if (sti == facts.source_table_info.end()) {
2030+
return "";
2031+
}
2032+
string cat = sti->second.catalog_name;
2033+
string sch = sti->second.schema_name;
2034+
string qual;
2035+
if (!cat.empty()) {
2036+
qual += KeywordHelper::WriteOptionallyQuoted(cat) + ".";
2037+
}
2038+
if (!sch.empty()) {
2039+
qual += KeywordHelper::WriteOptionallyQuoted(sch) + ".";
2040+
}
2041+
string inner_base = qual + KeywordHelper::WriteOptionallyQuoted(inner_table);
2042+
string delta_inner_bare = SqlUtils::DeltaName(inner_table);
2043+
string delta_inner = qual + KeywordHelper::WriteOptionallyQuoted(delta_inner_bare);
2044+
string qcol = KeywordHelper::WriteOptionallyQuoted(inner_col);
2045+
2046+
// The correction below assumes a dangling (null-padded) row for this key is ALREADY materialized
2047+
// in the view -- true only if the preserved-side row (e.g. the order) existed before this refresh
2048+
// batch. If the preserved-side row is itself brand new in this same batch (e.g. a freshly inserted
2049+
// order that immediately gets its first line), no dangling row was ever materialized, and applying
2050+
// this correction would spuriously subtract a row that never existed. Resolve the preserved side's
2051+
// own base table/column so we can gate on "did this key exist before this batch's delta."
2052+
LogicalGet *pres_get = nullptr;
2053+
string pres_col;
2054+
if (!ResolveBindingToGetColumn(key_pres, facts, pres_get, pres_col) || !pres_get || !pres_get->GetTable().get()) {
2055+
return "";
2056+
}
2057+
string pres_table = pres_get->GetTable().get()->name;
2058+
auto pres_sti = facts.source_table_info.find(pres_table);
2059+
if (pres_sti == facts.source_table_info.end()) {
2060+
return "";
2061+
}
2062+
string pres_qual;
2063+
if (!pres_sti->second.catalog_name.empty()) {
2064+
pres_qual += KeywordHelper::WriteOptionallyQuoted(pres_sti->second.catalog_name) + ".";
2065+
}
2066+
if (!pres_sti->second.schema_name.empty()) {
2067+
pres_qual += KeywordHelper::WriteOptionallyQuoted(pres_sti->second.schema_name) + ".";
2068+
}
2069+
string pres_base = pres_qual + KeywordHelper::WriteOptionallyQuoted(pres_table);
2070+
string delta_pres_bare = SqlUtils::DeltaName(pres_table);
2071+
string delta_pres = pres_qual + KeywordHelper::WriteOptionallyQuoted(delta_pres_bare);
2072+
string pres_qcol = KeywordHelper::WriteOptionallyQuoted(pres_col);
2073+
2074+
// Classify each aggregate output column's contribution to a null-padded row of the deepest join.
2075+
// output_names layout: [group cols (G)] [visible aggregates (agg.expressions, in order)] [hidden helpers].
2076+
// Visible: resolve the arg's base table — inner-side count/sum -> 0, preserved-side count -> 1
2077+
// (preserved-side sum needs the value: unsupported for now -> bail). count_star (no arg) -> 1.
2078+
// Hidden: openivm_count_star -> 1, openivm_match_count / openivm_nonnull_sum_count* -> 0 (all inner-derived).
2079+
size_t num_agg = output_names.size() - G;
2080+
vector<string> contribs;
2081+
for (size_t j = 0; j < num_agg; j++) {
2082+
if (j < agg.expressions.size()) {
2083+
auto &e = agg.expressions[j];
2084+
if (e->expression_class != ExpressionClass::BOUND_AGGREGATE) {
2085+
return "";
2086+
}
2087+
auto &ba = e->Cast<BoundAggregateExpression>();
2088+
string fn = StringUtil::Lower(ba.function.name);
2089+
if (ba.children.empty()) {
2090+
contribs.push_back("1"); // count_star: null-padded row still counts
2091+
continue;
2092+
}
2093+
auto *cref = GetColumnRefThroughCasts(ba.children[0].get());
2094+
if (!cref) {
2095+
return "";
2096+
}
2097+
LogicalGet *g = nullptr;
2098+
string c;
2099+
bool resolved = ResolveBindingToGetColumn(cref->binding, facts, g, c);
2100+
bool is_inner = resolved && g && g->table_index == inner_tidx;
2101+
if (fn == "count") {
2102+
contribs.push_back(is_inner ? "0" : "1");
2103+
if (!is_inner) {
2104+
// Preserved-side COUNT: the MERGE must NOT gate it by the (inner-side) match_count.
2105+
preserved_cols.push_back(output_names[G + j]);
2106+
}
2107+
} else if (fn == "sum") {
2108+
if (is_inner) {
2109+
contribs.push_back("0");
2110+
} else {
2111+
return ""; // preserved-side SUM needs the value; defer to a later generalization
2112+
}
2113+
} else {
2114+
return "";
2115+
}
2116+
} else {
2117+
const string &col = output_names[G + j];
2118+
if (col.find("count_star") != string::npos) {
2119+
contribs.push_back("1");
2120+
} else if (col.find("match_count") != string::npos || col.find("nonnull_sum_count") != string::npos) {
2121+
contribs.push_back("0");
2122+
} else {
2123+
return "";
2124+
}
2125+
}
2126+
}
2127+
2128+
// Build the preserved-side subquery X via LPTS, naming the join key "__k" and each group col "__g<i>".
2129+
auto x_plan = oj->children[0]->Copy(context);
2130+
x_plan->ResolveOperatorTypes();
2131+
auto x_bindings = x_plan->GetColumnBindings();
2132+
vector<ColumnBinding> group_bindings;
2133+
for (auto &ge : agg.groups) {
2134+
auto *gr = GetColumnRefThroughCasts(ge.get());
2135+
if (!gr) {
2136+
return "";
2137+
}
2138+
group_bindings.push_back(gr->binding);
2139+
}
2140+
vector<string> x_names(x_bindings.size());
2141+
int key_pos = -1;
2142+
vector<int> group_pos(G, -1);
2143+
for (size_t i = 0; i < x_bindings.size(); i++) {
2144+
x_names[i] = "__x" + std::to_string(i);
2145+
if (x_bindings[i] == key_pres) {
2146+
x_names[i] = "__k";
2147+
key_pos = static_cast<int>(i);
2148+
}
2149+
for (size_t gi = 0; gi < G; gi++) {
2150+
if (x_bindings[i] == group_bindings[gi]) {
2151+
x_names[i] = "__g" + std::to_string(gi);
2152+
group_pos[gi] = static_cast<int>(i);
2153+
}
2154+
}
2155+
}
2156+
if (key_pos < 0) {
2157+
return "";
2158+
}
2159+
for (size_t gi = 0; gi < G; gi++) {
2160+
if (group_pos[gi] < 0) {
2161+
return "";
2162+
}
2163+
}
2164+
string x_sql;
2165+
try {
2166+
SqlDialect dialect = SqlDialect::DUCKDB;
2167+
auto ast = LogicalPlanToAst(context, x_plan, dialect);
2168+
auto cte_list = AstToCteList(*ast, dialect);
2169+
x_sql = cte_list->ToQuery(true, x_names);
2170+
if (!x_sql.empty() && x_sql.back() == ';') {
2171+
x_sql.pop_back();
2172+
}
2173+
} catch (...) {
2174+
return "";
2175+
}
2176+
if (x_sql.empty()) {
2177+
return "";
2178+
}
2179+
2180+
// delta_mv columns: group cols + aggregate cols + openivm_multiplicity (matches the primary INSERT).
2181+
string col_list;
2182+
string sel;
2183+
for (size_t gi = 0; gi < G; gi++) {
2184+
col_list += KeywordHelper::WriteOptionallyQuoted(output_names[gi]) + ", ";
2185+
sel += "X.\"__g" + std::to_string(gi) + "\", ";
2186+
}
2187+
for (size_t j = 0; j < contribs.size(); j++) {
2188+
col_list += KeywordHelper::WriteOptionallyQuoted(output_names[G + j]) + ", ";
2189+
sel += contribs[j] + ", ";
2190+
}
2191+
col_list += "openivm_multiplicity";
2192+
sel += "CASE WHEN (__newc - __t.dsum) > 0 AND __newc = 0 THEN 1 ELSE -1 END";
2193+
2194+
string dmv = KeywordHelper::WriteOptionallyQuoted(SqlUtils::DeltaName(view_name));
2195+
string sql = "INSERT INTO " + dmv + " (" + col_list + ")\nSELECT " + sel + "\nFROM (SELECT " + qcol +
2196+
" AS __k, SUM(openivm_multiplicity) AS dsum FROM " + delta_inner +
2197+
" WHERE openivm_timestamp >= (SELECT last_update FROM " + string(openivm::DELTA_TABLES_TABLE) +
2198+
" WHERE view_name = '" + SqlUtils::EscapeValue(view_name) + "' AND table_name = '" +
2199+
SqlUtils::EscapeValue(delta_inner_bare) + "') GROUP BY " + qcol + ") __t\nJOIN (" + x_sql +
2200+
") X ON X.\"__k\" = __t.__k,\nLATERAL (SELECT (SELECT COUNT(*) FROM " + inner_base + " ib WHERE ib." +
2201+
qcol + " = __t.__k) AS __newc) __n,\nLATERAL (SELECT (SELECT COUNT(*) FROM " + pres_base +
2202+
" pb WHERE pb." + pres_qcol + " = __t.__k) - COALESCE((SELECT SUM(openivm_multiplicity) FROM " +
2203+
delta_pres + " WHERE " + pres_qcol + " = __t.__k AND openivm_timestamp >= (SELECT last_update FROM " +
2204+
string(openivm::DELTA_TABLES_TABLE) + " WHERE view_name = '" + SqlUtils::EscapeValue(view_name) +
2205+
"' AND table_name = '" + SqlUtils::EscapeValue(delta_pres_bare) +
2206+
"')), 0) AS __old_pres_count) __op\nWHERE (__newc > 0) <> ((__newc - __t.dsum) > 0) AND "
2207+
"__old_pres_count > 0;";
2208+
return sql;
2209+
}
2210+
19722211
} // namespace duckdb

src/core/refresh_metadata.cpp

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1077,6 +1077,29 @@ string RefreshMetadata::FilteredGroupCountAuxMetaToJson(const FilteredGroupCount
10771077
",\"threshold\":" + SqlUtils::JsonQuote(meta.threshold_sql) + "}";
10781078
}
10791079

1080+
bool RefreshMetadata::GetLeftJoinSecondaryMeta(const string &view_name, LeftJoinSecondaryMeta &out) {
1081+
auto result = con.Query("SELECT leftjoin_secondary_meta_json FROM " + string(openivm::VIEWS_TABLE) +
1082+
" WHERE view_name = '" + SqlUtils::EscapeValue(view_name) + "'");
1083+
if (result->HasError() || result->RowCount() == 0 || result->GetValue(0, 0).IsNull()) {
1084+
return false;
1085+
}
1086+
string json = result->GetValue(0, 0).ToString();
1087+
if (json.empty()) {
1088+
return false;
1089+
}
1090+
string kind;
1091+
if (!ExtractJsonString(json, "kind", kind) || kind != "leftjoin_secondary") {
1092+
return false;
1093+
}
1094+
ExtractJsonString(json, "preserved_cols", out.preserved_cols_csv);
1095+
return ExtractJsonString(json, "sql", out.sql);
1096+
}
1097+
1098+
string RefreshMetadata::LeftJoinSecondaryMetaToJson(const LeftJoinSecondaryMeta &meta) {
1099+
return "{\"kind\":\"leftjoin_secondary\",\"sql\":" + SqlUtils::JsonQuote(meta.sql) +
1100+
",\"preserved_cols\":" + SqlUtils::JsonQuote(meta.preserved_cols_csv) + "}";
1101+
}
1102+
10801103
vector<string> RefreshMetadata::ExpectedDistinctAuxColumns(const DistinctAuxMeta &meta) {
10811104
auto expected = meta.cols;
10821105
expected.push_back("_count");

0 commit comments

Comments
 (0)