From 6c626fd3ee0d9901d8b78a86ce0acfc3ec8f1156 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sat, 15 Aug 2026 21:49:25 +0000 Subject: [PATCH 01/18] feat: native Spark add_months scalar function DuckDB lacks add_months, which appears in Spark SQL fed to the openivm compiler and caused COMPILE_FAILED -> silent FULL_REFRESH demotion. Register add_months(DATE, INTEGER) -> DATE implementing full Spark semantics: day-of-month preserved, clamped to target month length, with the end-of-month rule (last day of source month maps to last day of target month). Reuses DuckDB Date API for leap-year-correct month lengths. Adds test/sql/spark_add_months.test covering scalar correctness and incremental MV maintenance (SIMPLE_PROJECTION delta + EXCEPT ALL parity). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CMakeLists.txt | 1 + src/functions/spark_scalar_functions.cpp | 52 +++++++ .../functions/spark_scalar_functions.hpp | 11 ++ src/openivm_extension.cpp | 3 + test/sql/spark_add_months.test | 135 ++++++++++++++++++ 5 files changed, 202 insertions(+) create mode 100644 src/functions/spark_scalar_functions.cpp create mode 100644 src/include/functions/spark_scalar_functions.hpp create mode 100644 test/sql/spark_add_months.test diff --git a/CMakeLists.txt b/CMakeLists.txt index 9889f8bb..869b7c0a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,6 +13,7 @@ include_directories(src/include ${LPTS_DIR}/src/include ${LPTS_DIR}/third_party/ set(EXTENSION_SOURCES src/openivm_extension.cpp src/compile_facts.cpp + src/functions/spark_scalar_functions.cpp src/core/ivm_delta_model.cpp src/core/ivm_view_classifier.cpp src/core/parser.cpp diff --git a/src/functions/spark_scalar_functions.cpp b/src/functions/spark_scalar_functions.cpp new file mode 100644 index 00000000..e982f963 --- /dev/null +++ b/src/functions/spark_scalar_functions.cpp @@ -0,0 +1,52 @@ +#include "functions/spark_scalar_functions.hpp" + +#include "duckdb/common/operator/numeric_cast.hpp" +#include "duckdb/common/types/date.hpp" +#include "duckdb/common/vector_operations/binary_executor.hpp" +#include "duckdb/function/scalar_function.hpp" + +namespace duckdb { + +// Spark add_months(start_date, num_months): +// Shift start_date by num_months. Day-of-month is preserved, except: +// - if start_date is the last day of its month, the result is the last day +// of the target month (end-of-month rule); +// - otherwise, if the original day exceeds the target month's length, it is +// clamped to the target month's last day. +// DuckDB's DATE + INTERVAL MONTH clamps but does not apply the end-of-month rule, +// so we implement the full Spark semantics here. +static date_t AddMonthsSparkImpl(date_t input, int32_t num_months) { + if (!Date::IsFinite(input)) { + return input; + } + int32_t year, month, day; + Date::Convert(input, year, month, day); + bool input_is_month_end = (day == Date::MonthDays(year, month)); + + int64_t zero_based_months = static_cast(year) * 12 + (month - 1) + num_months; + int32_t new_year = NumericCast(zero_based_months / 12); + int32_t new_month = static_cast(zero_based_months % 12); + if (new_month < 0) { + new_month += 12; + new_year -= 1; + } + new_month += 1; + + int32_t target_month_days = Date::MonthDays(new_year, new_month); + int32_t new_day = input_is_month_end ? target_month_days : MinValue(day, target_month_days); + return Date::FromDate(new_year, new_month, new_day); +} + +static void AddMonthsFunction(DataChunk &args, ExpressionState &state, Vector &result) { + BinaryExecutor::Execute( + args.data[0], args.data[1], result, args.size(), + [](date_t input, int32_t num_months) { return AddMonthsSparkImpl(input, num_months); }); +} + +void RegisterSparkScalarFunctions(ExtensionLoader &loader) { + ScalarFunction add_months("add_months", {LogicalType::DATE, LogicalType::INTEGER}, LogicalType::DATE, + AddMonthsFunction); + loader.RegisterFunction(add_months); +} + +} // namespace duckdb diff --git a/src/include/functions/spark_scalar_functions.hpp b/src/include/functions/spark_scalar_functions.hpp new file mode 100644 index 00000000..7cad619b --- /dev/null +++ b/src/include/functions/spark_scalar_functions.hpp @@ -0,0 +1,11 @@ +#pragma once + +#include "duckdb/main/extension/extension_loader.hpp" + +namespace duckdb { + +// Registers Spark-compatible scalar functions that DuckDB lacks natively but that +// appear in Spark SQL fed to the openivm compiler (compile/binding coverage). +void RegisterSparkScalarFunctions(ExtensionLoader &loader); + +} // namespace duckdb diff --git a/src/openivm_extension.cpp b/src/openivm_extension.cpp index ba49b33d..fb68a6a6 100644 --- a/src/openivm_extension.cpp +++ b/src/openivm_extension.cpp @@ -2,6 +2,7 @@ #include "core/openivm_extension.hpp" #include "compile_facts.hpp" +#include "functions/spark_scalar_functions.hpp" #include "core/openivm_constants.hpp" #include "core/refresh_metadata.hpp" #include "core/refresh_daemon.hpp" @@ -166,6 +167,8 @@ static void LoadInternal(ExtensionLoader &loader) { // statement OpenIVM does not recognize with DuckDB's native parser. db_config.SetOption(AllowParserOverrideExtensionSetting::SettingIndex, Value("fallback")); + RegisterSparkScalarFunctions(loader); + db_config.AddExtensionOption("openivm_files_path", "path for compiled SQL reference files", LogicalType::VARCHAR); db_config.AddExtensionOption("openivm_refresh_mode", "refresh strategy: incremental, full, or auto", LogicalType::VARCHAR, Value("incremental")); diff --git a/test/sql/spark_add_months.test b/test/sql/spark_add_months.test new file mode 100644 index 00000000..27170dfa --- /dev/null +++ b/test/sql/spark_add_months.test @@ -0,0 +1,135 @@ +# name: test/sql/spark_add_months.test +# description: Spark-compatible add_months scalar function (correctness + incremental MV maintenance) +# group: [sql] + +require openivm + +statement ok +SET openivm_files_path='__TEST_DIR__'; + +# --- Direct scalar correctness (Spark add_months semantics) --- + +# Month-end input -> target month-end (with clamp) +query I +SELECT add_months(DATE '2015-01-31', 1) = DATE '2015-02-28'; +---- +true + +# Non month-end input, day clamps to target month length +query I +SELECT add_months(DATE '2015-01-30', 1) = DATE '2015-02-28'; +---- +true + +# Non month-end input, day fits +query I +SELECT add_months(DATE '2015-01-28', 1) = DATE '2015-02-28'; +---- +true + +# Leap-year target month-end +query I +SELECT add_months(DATE '2016-01-31', 1) = DATE '2016-02-29'; +---- +true + +# Feb month-end (non-leap) -> March month-end +query I +SELECT add_months(DATE '2015-02-28', 1) = DATE '2015-03-31'; +---- +true + +# Leap Feb month-end -> non-leap Feb month-end across a year +query I +SELECT add_months(DATE '2016-02-29', 12) = DATE '2017-02-28'; +---- +true + +# Negative months, month-end preserved +query I +SELECT add_months(DATE '2015-03-31', -1) = DATE '2015-02-28'; +---- +true + +# Year rollover, day preserved +query I +SELECT add_months(DATE '2015-01-15', 13) = DATE '2016-02-15'; +---- +true + +# December month-end -> January month-end (year rollover) +query I +SELECT add_months(DATE '2015-12-31', 1) = DATE '2016-01-31'; +---- +true + +# NULL propagation +query I +SELECT add_months(NULL::DATE, 1) IS NULL; +---- +true + +# --- Incremental materialized-view maintenance using add_months --- + +statement ok +CREATE TABLE am_item (id INT, d DATE, n INT); + +statement ok +INSERT INTO am_item VALUES + (1, DATE '2015-01-31', 1), + (2, DATE '2016-02-29', 12), + (3, DATE '2015-01-15', 13); + +statement ok +CREATE MATERIALIZED VIEW am_mv AS + SELECT id, d, n, add_months(d, n) AS shifted + FROM am_item; + +statement ok +SELECT COUNT(*) FROM openivm_compile_with_facts( + 'am_mv', + '{"target_dialect":"duckdb","compile_only":true}' +); + +# Real SIMPLE_PROJECTION delta emitted (no full-refresh demotion) +query I +SELECT CASE + WHEN contains(content, 'INSERT INTO openivm_delta_am_mv') AND + contains(content, 'openivm_delta_am_item') AND + NOT contains(content, 'SELECT NULL::') + THEN 1 ELSE 0 + END +FROM read_text('__TEST_DIR__/openivm_upsert_queries_am_mv.sql'); +---- +1 + +statement ok +UPDATE am_item SET d = DATE '2015-12-31', n = 1 WHERE id = 1; + +statement ok +INSERT INTO am_item VALUES (4, DATE '2015-06-30', 3); + +statement ok +DELETE FROM am_item WHERE id = 3; + +statement ok +PRAGMA refresh('am_mv'); + +# Incrementally maintained MV matches full recomputation, both directions +query I +SELECT COUNT(*) FROM ( + SELECT * FROM am_mv + EXCEPT ALL + SELECT id, d, n, add_months(d, n) AS shifted FROM am_item +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + SELECT id, d, n, add_months(d, n) AS shifted FROM am_item + EXCEPT ALL + SELECT * FROM am_mv +); +---- +0 From c723a240761f438c90a549da2729939b348dbc15 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sun, 16 Aug 2026 18:34:18 +0000 Subject: [PATCH 02/18] refactor: source add_months from lpts instead of a native openivm copy Per review, the add_months scalar function belongs in lpts (the Spark-compat layer openivm already builds), not duplicated in openivm. Move the implementation to lpts (cwida/lpts#18) and consume it here via the pin: - delete src/functions/spark_scalar_functions.{cpp,hpp} (now in lpts) - compile ${LPTS_DIR}/src/spark_scalar_functions.cpp from the lpts submodule and include its header from ${LPTS_DIR}/src/include (already on the path) - keep the thin RegisterSparkScalarFunctions(loader) call in openivm's LoadInternal (openivm does not invoke lpts's LoadInternal, so it registers the lpts-provided function itself) - bump third_party/lpts 13786cb..642c762 (cwida/lpts main + add_months) - trim test/sql/spark_add_months.test to openivm's concern (add_months resolves + drives a real SIMPLE_PROJECTION delta / EXCEPT ALL parity); exhaustive scalar-correctness now lives in lpts Local openivm CI green (build + full sqllogictest: 10020 assertions / 83 cases). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CMakeLists.txt | 4 +- src/functions/spark_scalar_functions.cpp | 52 -------------- .../functions/spark_scalar_functions.hpp | 11 --- src/openivm_extension.cpp | 2 +- test/sql/spark_add_months.test | 67 +++---------------- third_party/lpts | 2 +- 6 files changed, 13 insertions(+), 125 deletions(-) delete mode 100644 src/functions/spark_scalar_functions.cpp delete mode 100644 src/include/functions/spark_scalar_functions.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 869b7c0a..10e20533 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -13,7 +13,6 @@ include_directories(src/include ${LPTS_DIR}/src/include ${LPTS_DIR}/third_party/ set(EXTENSION_SOURCES src/openivm_extension.cpp src/compile_facts.cpp - src/functions/spark_scalar_functions.cpp src/core/ivm_delta_model.cpp src/core/ivm_view_classifier.cpp src/core/parser.cpp @@ -79,7 +78,8 @@ set(EXTENSION_SOURCES ${LPTS_DIR}/src/lpts_ast_renderer.cpp ${LPTS_DIR}/src/lpts_ast_builder.cpp ${LPTS_DIR}/src/lpts_ast_flattener.cpp - ${LPTS_DIR}/src/dialect_function_map.cpp) + ${LPTS_DIR}/src/dialect_function_map.cpp + ${LPTS_DIR}/src/spark_scalar_functions.cpp) build_static_extension(${TARGET_NAME} ${EXTENSION_SOURCES}) build_loadable_extension(${TARGET_NAME} " " ${EXTENSION_SOURCES}) diff --git a/src/functions/spark_scalar_functions.cpp b/src/functions/spark_scalar_functions.cpp deleted file mode 100644 index e982f963..00000000 --- a/src/functions/spark_scalar_functions.cpp +++ /dev/null @@ -1,52 +0,0 @@ -#include "functions/spark_scalar_functions.hpp" - -#include "duckdb/common/operator/numeric_cast.hpp" -#include "duckdb/common/types/date.hpp" -#include "duckdb/common/vector_operations/binary_executor.hpp" -#include "duckdb/function/scalar_function.hpp" - -namespace duckdb { - -// Spark add_months(start_date, num_months): -// Shift start_date by num_months. Day-of-month is preserved, except: -// - if start_date is the last day of its month, the result is the last day -// of the target month (end-of-month rule); -// - otherwise, if the original day exceeds the target month's length, it is -// clamped to the target month's last day. -// DuckDB's DATE + INTERVAL MONTH clamps but does not apply the end-of-month rule, -// so we implement the full Spark semantics here. -static date_t AddMonthsSparkImpl(date_t input, int32_t num_months) { - if (!Date::IsFinite(input)) { - return input; - } - int32_t year, month, day; - Date::Convert(input, year, month, day); - bool input_is_month_end = (day == Date::MonthDays(year, month)); - - int64_t zero_based_months = static_cast(year) * 12 + (month - 1) + num_months; - int32_t new_year = NumericCast(zero_based_months / 12); - int32_t new_month = static_cast(zero_based_months % 12); - if (new_month < 0) { - new_month += 12; - new_year -= 1; - } - new_month += 1; - - int32_t target_month_days = Date::MonthDays(new_year, new_month); - int32_t new_day = input_is_month_end ? target_month_days : MinValue(day, target_month_days); - return Date::FromDate(new_year, new_month, new_day); -} - -static void AddMonthsFunction(DataChunk &args, ExpressionState &state, Vector &result) { - BinaryExecutor::Execute( - args.data[0], args.data[1], result, args.size(), - [](date_t input, int32_t num_months) { return AddMonthsSparkImpl(input, num_months); }); -} - -void RegisterSparkScalarFunctions(ExtensionLoader &loader) { - ScalarFunction add_months("add_months", {LogicalType::DATE, LogicalType::INTEGER}, LogicalType::DATE, - AddMonthsFunction); - loader.RegisterFunction(add_months); -} - -} // namespace duckdb diff --git a/src/include/functions/spark_scalar_functions.hpp b/src/include/functions/spark_scalar_functions.hpp deleted file mode 100644 index 7cad619b..00000000 --- a/src/include/functions/spark_scalar_functions.hpp +++ /dev/null @@ -1,11 +0,0 @@ -#pragma once - -#include "duckdb/main/extension/extension_loader.hpp" - -namespace duckdb { - -// Registers Spark-compatible scalar functions that DuckDB lacks natively but that -// appear in Spark SQL fed to the openivm compiler (compile/binding coverage). -void RegisterSparkScalarFunctions(ExtensionLoader &loader); - -} // namespace duckdb diff --git a/src/openivm_extension.cpp b/src/openivm_extension.cpp index fb68a6a6..e9dd9211 100644 --- a/src/openivm_extension.cpp +++ b/src/openivm_extension.cpp @@ -2,7 +2,7 @@ #include "core/openivm_extension.hpp" #include "compile_facts.hpp" -#include "functions/spark_scalar_functions.hpp" +#include "spark_scalar_functions.hpp" #include "core/openivm_constants.hpp" #include "core/refresh_metadata.hpp" #include "core/refresh_daemon.hpp" diff --git a/test/sql/spark_add_months.test b/test/sql/spark_add_months.test index 27170dfa..236503a2 100644 --- a/test/sql/spark_add_months.test +++ b/test/sql/spark_add_months.test @@ -1,74 +1,25 @@ # name: test/sql/spark_add_months.test -# description: Spark-compatible add_months scalar function (correctness + incremental MV maintenance) +# description: add_months incremental MV maintenance through the openivm compiler # group: [sql] +# +# Exhaustive scalar-correctness coverage for add_months lives in lpts +# (third_party/lpts test/sql/spark_add_months.test). openivm consumes the +# function from lpts; this test covers openivm's concern: that add_months +# resolves in the compiler and drives a real SIMPLE_PROJECTION delta rather +# than a silent FULL_REFRESH demotion. + require openivm statement ok SET openivm_files_path='__TEST_DIR__'; -# --- Direct scalar correctness (Spark add_months semantics) --- - -# Month-end input -> target month-end (with clamp) +# add_months resolves in an openivm session (registered via lpts source) query I SELECT add_months(DATE '2015-01-31', 1) = DATE '2015-02-28'; ---- true -# Non month-end input, day clamps to target month length -query I -SELECT add_months(DATE '2015-01-30', 1) = DATE '2015-02-28'; ----- -true - -# Non month-end input, day fits -query I -SELECT add_months(DATE '2015-01-28', 1) = DATE '2015-02-28'; ----- -true - -# Leap-year target month-end -query I -SELECT add_months(DATE '2016-01-31', 1) = DATE '2016-02-29'; ----- -true - -# Feb month-end (non-leap) -> March month-end -query I -SELECT add_months(DATE '2015-02-28', 1) = DATE '2015-03-31'; ----- -true - -# Leap Feb month-end -> non-leap Feb month-end across a year -query I -SELECT add_months(DATE '2016-02-29', 12) = DATE '2017-02-28'; ----- -true - -# Negative months, month-end preserved -query I -SELECT add_months(DATE '2015-03-31', -1) = DATE '2015-02-28'; ----- -true - -# Year rollover, day preserved -query I -SELECT add_months(DATE '2015-01-15', 13) = DATE '2016-02-15'; ----- -true - -# December month-end -> January month-end (year rollover) -query I -SELECT add_months(DATE '2015-12-31', 1) = DATE '2016-01-31'; ----- -true - -# NULL propagation -query I -SELECT add_months(NULL::DATE, 1) IS NULL; ----- -true - # --- Incremental materialized-view maintenance using add_months --- statement ok diff --git a/third_party/lpts b/third_party/lpts index 13786cbd..642c762a 160000 --- a/third_party/lpts +++ b/third_party/lpts @@ -1 +1 @@ -Subproject commit 13786cbd8f32216aa5d92c99f8bf53c7cc52c9fb +Subproject commit 642c762aa4443d0e2249515cc367ac8e06f5e83f From 1220ba41fb5dcf3dc4bd4f316995f08899f10c09 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sun, 16 Aug 2026 23:18:32 +0000 Subject: [PATCH 03/18] build: bump third_party/lpts to include the SPARK dialect icu test guard Points the lpts submodule at 592d469, which adds `require icu` to dialect_spark.test so offline linux_amd64 CI skips the TIMESTAMPTZ-binding assertions instead of failing. Keeps this PR's add_months source (already relocated into lpts) building against green lpts CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- third_party/lpts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/lpts b/third_party/lpts index 642c762a..592d469f 160000 --- a/third_party/lpts +++ b/third_party/lpts @@ -1 +1 @@ -Subproject commit 642c762aa4443d0e2249515cc367ac8e06f5e83f +Subproject commit 592d469f305fd56cff625e05b1860c86f18ffeae From 35615ede1d62c960febf5336bf605ee1f8ed41f8 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Mon, 17 Aug 2026 11:11:20 +0000 Subject: [PATCH 04/18] Linearize LEFT-JOIN IVM delta via N-term telescoping The compile-only regular N-term telescoping delta (openivm_regular_nterm) was gated to INNER-only joins; LEFT-join SIMPLE_PROJECTION views fell back to the inclusion-exclusion path, which enumerates 2^N-1 subset terms and copies the full plan per term. A 15-LEFT-JOIN star model therefore compiled in ~3.5h. Extend the linear telescoping path to LEFT joins, mirroring the shipping DuckLake N-term path (BuildDuckLakeJoinTerms/DemoteLeftJoinsForMask): - Add HasOnlyInnerOrLeftJoins() and gate the LEFT branch behind a new openivm_regular_nterm_left setting (default true). - In BuildRegularJoinTerms, per term demote only the outer join whose NULL-supplying subtree contains that term's single delta leaf; other LEFT joins stay LEFT, preserving their NULL-padded rows. - NULL<->match transition correctness is completed by the upsert layer's key-based partial recompute (BuildLeftJoinProjectionRefresh). - Propagate the new setting through PropagateRefreshPlanningSettings. Result: the 15-LEFT-JOIN model compiles in ~0.5s (statement/term count linear in join count) with identical results. FULL OUTER / RIGHT shapes and the inclusion-exclusion FK-pruning path are unchanged. Adds test/sql/left_join_regular_nterm.test: a 5-LEFT-JOIN star with a mixed DML batch (NULL->match, match->NULL, dim/fact updates, fact insert/delete) that compiles via the telescoping path and is bag-equal to the recomputed base query in both directions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/delta/operators/join.cpp | 51 ++++++-- src/openivm_extension.cpp | 3 + src/upsert/refresh_sql.cpp | 6 +- test/sql/left_join_regular_nterm.test | 160 ++++++++++++++++++++++++++ 4 files changed, 210 insertions(+), 10 deletions(-) create mode 100644 test/sql/left_join_regular_nterm.test diff --git a/src/delta/operators/join.cpp b/src/delta/operators/join.cpp index 428a9cc9..ca60db8a 100644 --- a/src/delta/operators/join.cpp +++ b/src/delta/operators/join.cpp @@ -1616,6 +1616,22 @@ static bool HasOnlyInnerJoins(LogicalOperator *node) { return true; } +static bool HasOnlyInnerOrLeftJoins(LogicalOperator *node) { + if (node->type == LogicalOperatorType::LOGICAL_COMPARISON_JOIN || + node->type == LogicalOperatorType::LOGICAL_ANY_JOIN) { + auto *join = dynamic_cast(node); + if (!join || (join->join_type != JoinType::INNER && join->join_type != JoinType::LEFT)) { + return false; + } + } + for (auto &child : node->children) { + if (!HasOnlyInnerOrLeftJoins(child.get())) { + return false; + } + } + return true; +} + static bool SupportsRegularNtermLeaf(const JoinLeafInfo &leaf) { if (leaf.get) { return leaf.get->GetTable().get() != nullptr; @@ -1705,7 +1721,7 @@ static DeltaPlanFragment CompileRegularLeafDelta(const DeltaOperatorInput &input static vector> BuildRegularJoinTerms(DeltaOperatorInput input, ClientContext &context, Binder &binder, const vector &leaves, - uint64_t unchanged_mask) { + uint64_t unchanged_mask, bool has_left_join) { vector> terms; // Base scans see post-DML state. Term i uses current state before i, delta i, and reconstructs old state after i as // current - delta. These disjoint telescoping terms cover every non-empty delta combination exactly once. @@ -1735,6 +1751,16 @@ static vector> BuildRegularJoinTerms(DeltaOperatorIn LogicalOperator *term_root = term.get(); CollectJoinLeaves(term.get(), {}, term_leaves); D_ASSERT(term_leaves.size() == leaves.size()); + + // LEFT-JOIN telescoping: demote only the outer join(s) whose NULL-supplying + // subtree contains this term's single delta leaf, mirroring the DuckLake + // N-term path (DemoteLeftJoinsForMask). Preserved outer joins elsewhere keep + // their NULL-padded rows; the upsert layer's key-based partial recompute + // (BuildLeftJoinProjectionRefresh) fixes NULL<->match transition rows. + if (has_left_join) { + DemoteLeftJoinsForMask(term.get(), term_leaves, (1ULL << delta_leaf)); + } + vector mul_bindings; for (size_t leaf = 0; leaf < term_leaves.size(); leaf++) { @@ -1880,11 +1906,22 @@ DeltaPlanFragment CompileJoinDelta(DeltaOperatorInput input) { } auto compile_facts = openivm::CompileFactsContextSlot::Get(context); auto unchanged_mask = ComputeFactsUnchangedMask(compile_facts, leaves); - bool regular_nterm = !all_ducklake && compile_facts.compile_only && !has_left_join && - input.context.model.type == RefreshType::SIMPLE_PROJECTION && - HasOnlyInnerJoins(input.plan.get()) && - RegularNtermPreservesFKPruning(context, compile_facts, leaves, input.plan.get()) && - SqlUtils::GetBoolSetting(context, "openivm_regular_nterm", true); + bool regular_nterm_base = !all_ducklake && compile_facts.compile_only && + input.context.model.type == RefreshType::SIMPLE_PROJECTION && + SqlUtils::GetBoolSetting(context, "openivm_regular_nterm", true); + bool regular_nterm; + if (has_left_join) { + // LEFT-JOIN telescoping: the regular N-term delta extends to LEFT joins via + // per-term demotion of only the outer join whose NULL-supplying side carries + // that term's delta (see BuildRegularJoinTerms). FULL OUTER / RIGHT shapes and + // the inclusion-exclusion FK-pruning path are out of scope; NULL-padded row + // correctness is completed by BuildLeftJoinProjectionRefresh in the upsert layer. + regular_nterm = regular_nterm_base && HasOnlyInnerOrLeftJoins(input.plan.get()) && + SqlUtils::GetBoolSetting(context, "openivm_regular_nterm_left", true); + } else { + regular_nterm = regular_nterm_base && HasOnlyInnerJoins(input.plan.get()) && + RegularNtermPreservesFKPruning(context, compile_facts, leaves, input.plan.get()); + } if (regular_nterm) { for (auto &leaf : leaves) { if (!SupportsRegularNtermLeaf(leaf)) { @@ -1902,7 +1939,7 @@ DeltaPlanFragment CompileJoinDelta(DeltaOperatorInput input) { if (all_ducklake) { terms = BuildDuckLakeJoinTerms(input, context, binder, leaves, has_left_join, flattened_ducklake); } else if (regular_nterm) { - terms = BuildRegularJoinTerms(input, context, binder, leaves, unchanged_mask); + terms = BuildRegularJoinTerms(input, context, binder, leaves, unchanged_mask, has_left_join); } else { terms = BuildInclusionExclusionTerms(input, context, binder, leaves, has_left_join, transition_ctes); } diff --git a/src/openivm_extension.cpp b/src/openivm_extension.cpp index e9dd9211..3c347ed6 100644 --- a/src/openivm_extension.cpp +++ b/src/openivm_extension.cpp @@ -194,6 +194,9 @@ static void LoadInternal(ExtensionLoader &loader) { LogicalType::BOOLEAN, Value::BOOLEAN(true)); db_config.AddExtensionOption("openivm_regular_nterm", "use N-term telescoping for compile-only regular inner joins", LogicalType::BOOLEAN, Value::BOOLEAN(true)); + db_config.AddExtensionOption("openivm_regular_nterm_left", + "extend compile-only N-term telescoping to LEFT-join projection views", + LogicalType::BOOLEAN, Value::BOOLEAN(true)); db_config.AddExtensionOption("openivm_fk_pruning", "prune inclusion-exclusion join terms using FK constraints", LogicalType::BOOLEAN, Value::BOOLEAN(true)); db_config.AddExtensionOption("openivm_scd2_range_join_accel", diff --git a/src/upsert/refresh_sql.cpp b/src/upsert/refresh_sql.cpp index 26026b3d..43cbeb5d 100644 --- a/src/upsert/refresh_sql.cpp +++ b/src/upsert/refresh_sql.cpp @@ -532,9 +532,9 @@ static void PropagateRefreshPlanningSettings(ClientContext &from, ClientContext // session-scoped planning settings still need to be mirrored onto the fresh // planning connection. static const char *PLANNING_SETTINGS[] = { - "openivm_adaptive_refresh", "openivm_cost_decay", "openivm_skip_empty_deltas", - "openivm_fk_pruning", "openivm_ducklake_nterm", "openivm_scd2_range_join_accel", - "openivm_regular_nterm", + "openivm_adaptive_refresh", "openivm_cost_decay", "openivm_skip_empty_deltas", + "openivm_fk_pruning", "openivm_ducklake_nterm", "openivm_scd2_range_join_accel", + "openivm_regular_nterm", "openivm_regular_nterm_left", }; for (auto setting_name : PLANNING_SETTINGS) { CopyOpenIvmSetting(from, to, setting_name); diff --git a/test/sql/left_join_regular_nterm.test b/test/sql/left_join_regular_nterm.test new file mode 100644 index 00000000..7e96e14d --- /dev/null +++ b/test/sql/left_join_regular_nterm.test @@ -0,0 +1,160 @@ +# name: test/sql/left_join_regular_nterm.test +# description: Compile-only N-term telescoping delta for LEFT-JOIN SIMPLE_PROJECTION views +# (openivm_regular_nterm_left). A deep LEFT-join star that would blow up under +# 2^N inclusion-exclusion must compile to a linear delta and stay bag-correct. +# group: [sql] + +require openivm + +statement ok +SET openivm_files_path='__TEST_DIR__'; + +statement ok +SET openivm_regular_nterm_left=true; + +# ========================================== +# Star schema: 1 fact + 5 LEFT-joined dimensions, projection (no aggregation). +# ========================================== + +statement ok +CREATE TABLE fact(id INTEGER, amount INTEGER, k1 INTEGER, k2 INTEGER, k3 INTEGER, k4 INTEGER, k5 INTEGER); + +statement ok +CREATE TABLE d1(k INTEGER, v VARCHAR); + +statement ok +CREATE TABLE d2(k INTEGER, v VARCHAR); + +statement ok +CREATE TABLE d3(k INTEGER, v VARCHAR); + +statement ok +CREATE TABLE d4(k INTEGER, v VARCHAR); + +statement ok +CREATE TABLE d5(k INTEGER, v VARCHAR); + +statement ok +INSERT INTO d1 VALUES (11, 'd1a'), (21, 'd1b'); + +statement ok +INSERT INTO d2 VALUES (12, 'd2a'), (22, 'd2b'); + +statement ok +INSERT INTO d3 VALUES (13, 'd3a'), (23, 'd3b'); + +statement ok +INSERT INTO d4 VALUES (14, 'd4a'), (24, 'd4b'); + +statement ok +INSERT INTO d5 VALUES (15, 'd5a'), (25, 'd5b'); + +# Row 2 has k1=91 which has NO matching d1 yet (NULL-padded until d1 gets 91). +statement ok +INSERT INTO fact VALUES + (1, 100, 11, 12, 13, 14, 15), + (2, 200, 91, 12, 13, 14, 15), + (3, 300, 21, 22, 23, 24, 25), + (4, 400, 11, 22, 13, 24, 15); + +statement ok +CREATE MATERIALIZED VIEW mv AS + SELECT f.id, f.amount, + d1.v AS v1, d2.v AS v2, d3.v AS v3, d4.v AS v4, d5.v AS v5 + FROM fact f + LEFT JOIN d1 ON f.k1 = d1.k + LEFT JOIN d2 ON f.k2 = d2.k + LEFT JOIN d3 ON f.k3 = d3.k + LEFT JOIN d4 ON f.k4 = d4.k + LEFT JOIN d5 ON f.k5 = d5.k; + +# Snapshot the MV so we can prove openivm_compile_with_facts does not mutate it. +statement ok +CREATE TABLE mv_before AS SELECT * FROM mv; + +# ========================================== +# Mixed DML batch: NULL->match (d1 gains key 91), match->NULL (d2 loses key 22), +# a dimension value update, and fact insert/update/delete. +# ========================================== + +statement ok +INSERT INTO d1 VALUES (91, 'd1_late'); + +statement ok +DELETE FROM d2 WHERE k = 22; + +statement ok +UPDATE d3 SET v = 'd3_upd' WHERE k = 13; + +statement ok +UPDATE fact SET amount = amount + 5 WHERE id = 1; + +statement ok +INSERT INTO fact VALUES (5, 500, 91, 22, 13, 14, 15); + +statement ok +DELETE FROM fact WHERE id = 3; + +# ========================================== +# Test 1: compile-only path emits a SIMPLE_PROJECTION join delta into +# openivm_delta_mv without exploding (a LEFT star that would be 2^6-1 under +# inclusion-exclusion compiles to a linear N-term telescoping delta). +# ========================================== + +query I +SELECT COUNT(*) > 0 +FROM openivm_compile_with_facts( + 'mv', + '{"target_dialect":"duckdb","compile_only":true, + "delta_shape":{"fact":"MIXED","d1":"INSERT_ONLY","d2":"MIXED","d3":"MIXED","d4":"UNCHANGED","d5":"UNCHANGED"}}' +) +WHERE stmt_kind = 'data' AND refresh_type_name = 'SIMPLE_PROJECTION' + AND sql LIKE '%openivm_delta_mv%'; +---- +true + +# Test 2: the compile call left the materialized view untouched. +query I +SELECT (SELECT count(*) FROM ((SELECT * FROM mv) EXCEPT ALL (SELECT * FROM mv_before))) = 0 AND + (SELECT count(*) FROM ((SELECT * FROM mv_before) EXCEPT ALL (SELECT * FROM mv))) = 0 AS unchanged; +---- +true + +# ========================================== +# Test 3: a real incremental refresh applies the batched deltas and the MV is +# bag-equal to the fully recomputed base query in BOTH directions (full IVM +# correctness cross-check, including the NULL<->match transition rows). +# ========================================== + +statement ok +PRAGMA refresh('mv'); + +query I +SELECT count(*) FROM ( + SELECT f.id, f.amount, d1.v AS v1, d2.v AS v2, d3.v AS v3, d4.v AS v4, d5.v AS v5 + FROM fact f + LEFT JOIN d1 ON f.k1 = d1.k + LEFT JOIN d2 ON f.k2 = d2.k + LEFT JOIN d3 ON f.k3 = d3.k + LEFT JOIN d4 ON f.k4 = d4.k + LEFT JOIN d5 ON f.k5 = d5.k + EXCEPT ALL + SELECT id, amount, v1, v2, v3, v4, v5 FROM mv +); +---- +0 + +query I +SELECT count(*) FROM ( + SELECT id, amount, v1, v2, v3, v4, v5 FROM mv + EXCEPT ALL + SELECT f.id, f.amount, d1.v AS v1, d2.v AS v2, d3.v AS v3, d4.v AS v4, d5.v AS v5 + FROM fact f + LEFT JOIN d1 ON f.k1 = d1.k + LEFT JOIN d2 ON f.k2 = d2.k + LEFT JOIN d3 ON f.k3 = d3.k + LEFT JOIN d4 ON f.k4 = d4.k + LEFT JOIN d5 ON f.k5 = d5.k +); +---- +0 From b74785f1d238ea3bed449352cae5f7d8dd9163a8 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Mon, 17 Aug 2026 11:34:53 +0000 Subject: [PATCH 05/18] test: fix sqllogictest header ordering for format-check duckdb/scripts/format.py requires the `# group:` tag immediately after a single-line `# description:`; the multi-line description tripped the Code Quality Check / Format Check CI leg. Collapse the description to one line and move the elaboration below the group tag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/sql/left_join_regular_nterm.test | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/sql/left_join_regular_nterm.test b/test/sql/left_join_regular_nterm.test index 7e96e14d..5b4177d3 100644 --- a/test/sql/left_join_regular_nterm.test +++ b/test/sql/left_join_regular_nterm.test @@ -1,9 +1,10 @@ # name: test/sql/left_join_regular_nterm.test -# description: Compile-only N-term telescoping delta for LEFT-JOIN SIMPLE_PROJECTION views -# (openivm_regular_nterm_left). A deep LEFT-join star that would blow up under -# 2^N inclusion-exclusion must compile to a linear delta and stay bag-correct. +# description: Compile-only N-term telescoping delta for LEFT-JOIN SIMPLE_PROJECTION views (openivm_regular_nterm_left) # group: [sql] +# A deep LEFT-join star that would blow up under 2^N inclusion-exclusion must +# compile to a linear delta and stay bag-correct. + require openivm statement ok From a9fd5d0886f5a4e695f1672d768ce5e7ff3fa30b Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sat, 22 Aug 2026 22:55:45 +0000 Subject: [PATCH 06/18] Advance lpts pin to 77ed5e5 and add arc-machine/int-instance regressions Bump third_party/lpts to 77ed5e585f26e2702a7b2af3cc9e8623769256c8 (LPTS PR #18, "Fix set-op column binding remaps"). The fork URL/remote in .gitmodules is untouched -- only the pinned commit moves forward. This pin alone fixes a stale UNION ALL child-alias remap surfaced by the reduced "int_instance_status_transaction" shape (duplicated UNION ALL key projections/output alias remapping that could leave a stale reference to a child alias no longer present in the rewritten plan). Extend test/sql/compile_refresh.test, compile_spark_dialect_hardening.test, and cascade_simple_projection_join.test with reduced regressions covering both previously-failing benchmark shapes: - arc_machine_status_transaction: a CTE join (INNER JOIN + LEFT JOIN) feeding an outer query with two further chained LEFT JOINs -- downstream projection/join over an N-ary join whose lhs binding could be lost or clobbered by sibling traversal. - int_instance_status_transaction: a UNION ALL of two LEFT JOIN branches partitioned by an IS NOT NULL / IS NULL predicate on the joined side, exercising duplicated UNION ALL key projections/output alias remapping. Compile-only assertions (openivm_compile_with_facts with compile_only=true, force_view_delta_cascade=true) confirm both shapes compile to a real incremental SIMPLE_PROJECTION delta program -- never FULL_REFRESH -- including a batched multi-leaf delta variant of the arc-machine shape. The cascade_simple_projection_join.test additions add a real CREATE + batched INSERT/DELETE/UPDATE + one PRAGMA refresh integration case per shape, with bidirectional EXCEPT ALL bag-equality checks against a from-scratch recompute. The arc-machine integration case (batched deltas spread across all 5 base tables) requires the native fix in the following commit to pass; it is included here to document the exact regression that fix guards against. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/sql/cascade_simple_projection_join.test | 217 ++++++++++++++++++ test/sql/compile_refresh.test | 206 +++++++++++++++++ test/sql/compile_spark_dialect_hardening.test | 137 +++++++++++ third_party/lpts | 2 +- 4 files changed, 561 insertions(+), 1 deletion(-) diff --git a/test/sql/cascade_simple_projection_join.test b/test/sql/cascade_simple_projection_join.test index 8de65d6c..9aa421c7 100644 --- a/test/sql/cascade_simple_projection_join.test +++ b/test/sql/cascade_simple_projection_join.test @@ -565,3 +565,220 @@ SELECT COUNT(*) FROM ( ); ---- 0 + +# Reduced "arc_machine_status_transaction" shape: a CTE (INNER JOIN + LEFT +# JOIN) feeding an outer query with two further chained LEFT JOINs. Before +# src/delta/operators/join.cpp's AppendMultiplicityToAncestorProjectionMaps +# fix, a deeper join's own left_projection_map growing (to append a new +# per-leaf multiplicity column) silently shifted where its right-side +# contribution begins in its own combined column numbering. A grandparent +# join's pre-existing projection-map entry, fixed before that growth +# happened, could then alias onto the just-added multiplicity column +# instead of the real business column (`cust_key`) it used to select, +# permanently dropping it and surfacing as an internal arity mismatch. +statement ok +CREATE TABLE cspj_arcm_msf(id INT, coll_key INT); + +statement ok +CREATE TABLE cspj_arcm_acd(coll_key INT, sub_id INT, arm_id VARCHAR); + +statement ok +CREATE TABLE cspj_arcm_hw(arm_id VARCHAR, provider VARCHAR); + +statement ok +CREATE TABLE cspj_arcm_tpid(sub_id INT, tp_id INT); + +statement ok +CREATE TABLE cspj_arcm_cust(tp_id INT, cust_key INT); + +statement ok +INSERT INTO cspj_arcm_msf VALUES (1, 100), (2, 100); + +statement ok +INSERT INTO cspj_arcm_acd VALUES (100, 10, 'arm-1'); + +statement ok +INSERT INTO cspj_arcm_hw VALUES ('arm-1', 'azure'); + +statement ok +INSERT INTO cspj_arcm_tpid VALUES (10, 200); + +statement ok +INSERT INTO cspj_arcm_cust VALUES (200, 21); + +statement ok +CREATE MATERIALIZED VIEW cspj_arcm_mv AS + WITH bff AS ( + SELECT msf.id, acd.sub_id, + coalesce(hw.provider, 'N/A') AS provider + FROM cspj_arcm_msf msf + INNER JOIN cspj_arcm_acd acd ON msf.coll_key = acd.coll_key + LEFT JOIN cspj_arcm_hw hw ON acd.arm_id = hw.arm_id + ) + SELECT bff.id, + coalesce(cust.cust_key, 1) AS cust_key + FROM bff + LEFT JOIN cspj_arcm_tpid tpid ON bff.sub_id = tpid.sub_id + LEFT JOIN cspj_arcm_cust cust ON coalesce(tpid.tp_id, -1) = cust.tp_id; + +statement ok +SELECT COUNT(*) FROM openivm_compile_with_facts( + 'cspj_arcm_mv', + '{"target_dialect":"duckdb","compile_only":true}' +); + +query I +SELECT CASE + WHEN contains(content, 'INSERT INTO openivm_delta_cspj_arcm_mv') AND + contains(content, 'openivm_delta_cspj_arcm_msf') AND + contains(content, 'openivm_delta_cspj_arcm_acd') AND + contains(content, 'openivm_delta_cspj_arcm_hw') AND + contains(content, 'openivm_delta_cspj_arcm_tpid') AND + contains(content, 'openivm_delta_cspj_arcm_cust') AND + NOT contains(content, 'SELECT NULL::') + THEN 1 ELSE 0 + END +FROM read_text('__TEST_DIR__/openivm_upsert_queries_cspj_arcm_mv.sql'); +---- +1 + +statement ok +INSERT INTO cspj_arcm_msf VALUES (3, 300); + +statement ok +INSERT INTO cspj_arcm_acd VALUES (300, 30, 'arm-2'); + +statement ok +INSERT INTO cspj_arcm_hw VALUES ('arm-2', 'gcp'); + +statement ok +INSERT INTO cspj_arcm_tpid VALUES (30, 400); + +statement ok +DELETE FROM cspj_arcm_msf WHERE id = 2; + +statement ok +UPDATE cspj_arcm_cust SET cust_key = 99 WHERE tp_id = 200; + +statement ok +PRAGMA refresh('cspj_arcm_mv'); + +query I +SELECT COUNT(*) FROM ( + SELECT * FROM cspj_arcm_mv + EXCEPT ALL + SELECT bff.id, coalesce(cust.cust_key, 1) AS cust_key + FROM ( + SELECT msf.id, acd.sub_id, coalesce(hw.provider, 'N/A') AS provider + FROM cspj_arcm_msf msf + INNER JOIN cspj_arcm_acd acd ON msf.coll_key = acd.coll_key + LEFT JOIN cspj_arcm_hw hw ON acd.arm_id = hw.arm_id + ) bff + LEFT JOIN cspj_arcm_tpid tpid ON bff.sub_id = tpid.sub_id + LEFT JOIN cspj_arcm_cust cust ON coalesce(tpid.tp_id, -1) = cust.tp_id +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + SELECT bff.id, coalesce(cust.cust_key, 1) AS cust_key + FROM ( + SELECT msf.id, acd.sub_id, coalesce(hw.provider, 'N/A') AS provider + FROM cspj_arcm_msf msf + INNER JOIN cspj_arcm_acd acd ON msf.coll_key = acd.coll_key + LEFT JOIN cspj_arcm_hw hw ON acd.arm_id = hw.arm_id + ) bff + LEFT JOIN cspj_arcm_tpid tpid ON bff.sub_id = tpid.sub_id + LEFT JOIN cspj_arcm_cust cust ON coalesce(tpid.tp_id, -1) = cust.tp_id + EXCEPT ALL + SELECT * FROM cspj_arcm_mv +); +---- +0 + +# Reduced "int_instance_status_transaction" shape: a UNION ALL of two LEFT +# JOIN branches partitioned by an IS NOT NULL / IS NULL predicate on the +# joined side. Before the `third_party/lpts` pin was advanced to commit +# 77ed5e5 (fork PR #18, "Fix set-op column binding remaps"), remapping +# duplicated UNION ALL key-projection output aliases could leave a stale +# reference to a child alias that no longer existed in the rewritten plan, +# surfacing as a runtime Binder Error ("Referenced column ... not found in +# FROM clause"). +statement ok +CREATE TABLE cspj_inti_isf(id INT, k INT); + +statement ok +CREATE TABLE cspj_inti_ml(k INT, res VARCHAR); + +statement ok +INSERT INTO cspj_inti_isf VALUES (1, 100), (2, 200); + +statement ok +INSERT INTO cspj_inti_ml VALUES (100, 'machine-1'); + +statement ok +CREATE MATERIALIZED VIEW cspj_inti_mv AS + SELECT isf.id, isf.k FROM cspj_inti_isf isf LEFT JOIN cspj_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NOT NULL + UNION ALL + SELECT isf.id, isf.k FROM cspj_inti_isf isf LEFT JOIN cspj_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NULL; + +statement ok +SELECT COUNT(*) FROM openivm_compile_with_facts( + 'cspj_inti_mv', + '{"target_dialect":"duckdb","compile_only":true}' +); + +query I +SELECT CASE + WHEN contains(content, 'INSERT INTO openivm_delta_cspj_inti_mv') AND + contains(content, 'openivm_delta_cspj_inti_isf') AND + contains(content, 'openivm_delta_cspj_inti_ml') AND + contains(content, 'UNION ALL') AND + NOT contains(content, 'SELECT NULL::') + THEN 1 ELSE 0 + END +FROM read_text('__TEST_DIR__/openivm_upsert_queries_cspj_inti_mv.sql'); +---- +1 + +statement ok +INSERT INTO cspj_inti_isf VALUES (3, 300), (4, 400); + +statement ok +INSERT INTO cspj_inti_ml VALUES (300, 'machine-2'), (400, NULL); + +statement ok +DELETE FROM cspj_inti_isf WHERE id = 2; + +statement ok +UPDATE cspj_inti_ml SET res = NULL WHERE k = 100; + +statement ok +PRAGMA refresh('cspj_inti_mv'); + +query I +SELECT COUNT(*) FROM ( + SELECT * FROM cspj_inti_mv + EXCEPT ALL + ( + SELECT isf.id, isf.k FROM cspj_inti_isf isf LEFT JOIN cspj_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NOT NULL + UNION ALL + SELECT isf.id, isf.k FROM cspj_inti_isf isf LEFT JOIN cspj_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NULL + ) +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + ( + SELECT isf.id, isf.k FROM cspj_inti_isf isf LEFT JOIN cspj_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NOT NULL + UNION ALL + SELECT isf.id, isf.k FROM cspj_inti_isf isf LEFT JOIN cspj_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NULL + ) + EXCEPT ALL + SELECT * FROM cspj_inti_mv +); +---- +0 diff --git a/test/sql/compile_refresh.test b/test/sql/compile_refresh.test index d65cd275..70b8ae5b 100644 --- a/test/sql/compile_refresh.test +++ b/test/sql/compile_refresh.test @@ -389,6 +389,212 @@ WHERE stmt_kind = 'data'; ---- 1 +# ========================================== +# Test 10: reduced "arc_machine_status_transaction" shape — a CTE joining an +# INNER JOIN with a LEFT JOIN, whose result feeds an outer query with two +# further chained LEFT JOINs. Before src/delta/operators/join.cpp's +# AppendMultiplicityToAncestorProjectionMaps fix, a deeper join's own +# left_projection_map growing (to carry a new per-leaf multiplicity column up +# to the root) silently shifted where its right-side contribution begins in +# its own combined column numbering. A grandparent join's pre-existing +# projection-map entry — fixed before that growth happened — could then +# alias onto the just-added multiplicity column instead of the real column +# it used to select, permanently dropping a business column and surfacing +# as an internal arity mismatch ("union lhs column ref not in column_map"). +# ========================================== + +statement ok +CREATE TABLE arcm_msf(id INT, coll_key INT); + +statement ok +CREATE TABLE arcm_acd(coll_key INT, sub_id INT, arm_id VARCHAR); + +statement ok +CREATE TABLE arcm_hw(arm_id VARCHAR, provider VARCHAR); + +statement ok +CREATE TABLE arcm_tpid(sub_id INT, tp_id INT); + +statement ok +CREATE TABLE arcm_cust(tp_id INT, cust_key INT); + +statement ok +INSERT INTO arcm_msf VALUES (1, 100); + +statement ok +INSERT INTO arcm_acd VALUES (100, 10, 'arm-1'); + +statement ok +INSERT INTO arcm_hw VALUES ('arm-1', 'azure'); + +statement ok +INSERT INTO arcm_tpid VALUES (10, 200); + +statement ok +INSERT INTO arcm_cust VALUES (200, 21); + +statement ok +CREATE MATERIALIZED VIEW mv_arcm AS + WITH bff AS ( + SELECT msf.id, acd.sub_id, + coalesce(hw.provider, 'N/A') AS provider + FROM arcm_msf msf + INNER JOIN arcm_acd acd ON msf.coll_key = acd.coll_key + LEFT JOIN arcm_hw hw ON acd.arm_id = hw.arm_id + ) + SELECT bff.id, + coalesce(cust.cust_key, 1) AS cust_key + FROM bff + LEFT JOIN arcm_tpid tpid ON bff.sub_id = tpid.sub_id + LEFT JOIN arcm_cust cust ON coalesce(tpid.tp_id, -1) = cust.tp_id; + +statement ok +INSERT INTO arcm_msf VALUES (2, 100); + +# Must compile to a real incremental cascade delta (SIMPLE_PROJECTION), never +# FULL_REFRESH, and must not lose the `cust_key` column from the final insert. +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'mv_arcm', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) + LIKE '%INSERT INTO openivm_delta_mv_arcm (id, cust_key, openivm_multiplicity)%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_msf%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_acd%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_hw%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_tpid%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_cust%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'mv_arcm', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# Batched multi-leaf delta variant of the same shape: simultaneous +# inserts/delete/update spread across ALL FIVE base tables (not just a +# single-row insert into one leaf). This is the exact reduction that exposed +# a second, deeper defect in src/delta/operators/join.cpp's +# BuildInclusionExclusionTerms: leaf substitution reused a stale pointer into +# the ORIGINAL (pre-mask-renumbering) plan instead of the mask's own +# freshly-renumbered leaf, so a leaf feeding two joins (msf join acd AND acd +# join hw) got its delta substituted at the wrong table_index, leaving the +# second join's condition dangling and surfacing as +# LPTS_UNSUPPORTED_COLUMN_REF once more than one leaf changed at once. +statement ok +INSERT INTO arcm_msf VALUES (3, 300); + +statement ok +INSERT INTO arcm_acd VALUES (300, 30, 'arm-2'); + +statement ok +INSERT INTO arcm_hw VALUES ('arm-2', 'gcp'); + +statement ok +INSERT INTO arcm_tpid VALUES (30, 400); + +statement ok +DELETE FROM arcm_msf WHERE id = 2; + +statement ok +UPDATE arcm_cust SET cust_key = 99 WHERE tp_id = 200; + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'mv_arcm', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) + LIKE '%INSERT INTO openivm_delta_mv_arcm (id, cust_key, openivm_multiplicity)%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_msf%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_acd%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_hw%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_tpid%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_arcm_cust%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'mv_arcm', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# ========================================== +# Test 11: reduced "int_instance_status_transaction" shape — a UNION ALL of +# two LEFT JOIN branches partitioned by an IS NOT NULL / IS NULL predicate on +# the joined side. Before the `third_party/lpts` pin was advanced to commit +# 77ed5e5 (fork PR #18, "Fix set-op column binding remaps"), remapping +# duplicated UNION ALL key-projection output aliases could leave a stale +# reference to a child alias that no longer existed in the rewritten plan, +# surfacing as a runtime Binder Error ("Referenced column ... not found in +# FROM clause"). +# ========================================== + +statement ok +CREATE TABLE inti_isf(id INT, k INT); + +statement ok +CREATE TABLE inti_ml(k INT, res VARCHAR); + +statement ok +INSERT INTO inti_isf VALUES (1, 100), (2, 200); + +statement ok +INSERT INTO inti_ml VALUES (100, 'machine-1'); + +statement ok +CREATE MATERIALIZED VIEW mv_inti AS + SELECT isf.id, isf.k FROM inti_isf isf LEFT JOIN inti_ml ml ON isf.k = ml.k WHERE ml.res IS NOT NULL + UNION ALL + SELECT isf.id, isf.k FROM inti_isf isf LEFT JOIN inti_ml ml ON isf.k = ml.k WHERE ml.res IS NULL; + +statement ok +INSERT INTO inti_isf VALUES (3, 300); + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'mv_inti', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) + LIKE '%INSERT INTO openivm_delta_mv_inti (id, k, openivm_left_key, openivm_multiplicity)%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_inti_isf%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%openivm_delta_inti_ml%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%UNION ALL%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'mv_inti', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + # ========================================== # Test 9: openivm_compile_with_facts on a non-existent view fails cleanly # ========================================== diff --git a/test/sql/compile_spark_dialect_hardening.test b/test/sql/compile_spark_dialect_hardening.test index 246eefd3..0a496bf4 100644 --- a/test/sql/compile_spark_dialect_hardening.test +++ b/test/sql/compile_spark_dialect_hardening.test @@ -195,3 +195,140 @@ WHERE stmt_kind = 'data'; statement ok SET openivm_refresh_mode = 'incremental'; + +# ========================================== +# Spark hardening for the reduced "arc_machine_status_transaction" shape: a +# CTE (INNER JOIN + LEFT JOIN) feeding an outer query with two further +# chained LEFT JOINs must still compile to a real incremental cascade delta +# under target_dialect=spark, with a spark-portable (no `::`) cast and +# backtick-quoted identifiers, and without dropping the `cust_key` column +# from the final signed insert — the regression this shape covers is in +# src/delta/operators/join.cpp's AppendMultiplicityToAncestorProjectionMaps, +# not dialect-specific, but the original failure was first observed against +# a spark-targeted compile. +# ========================================== +statement ok +CREATE TABLE sph_arcm_msf(id INT, coll_key INT); + +statement ok +CREATE TABLE sph_arcm_acd(coll_key INT, sub_id INT, arm_id VARCHAR); + +statement ok +CREATE TABLE sph_arcm_hw(arm_id VARCHAR, provider VARCHAR); + +statement ok +CREATE TABLE sph_arcm_tpid(sub_id INT, tp_id INT); + +statement ok +CREATE TABLE sph_arcm_cust(tp_id INT, cust_key INT); + +statement ok +INSERT INTO sph_arcm_msf VALUES (1, 100); + +statement ok +INSERT INTO sph_arcm_acd VALUES (100, 10, 'arm-1'); + +statement ok +INSERT INTO sph_arcm_hw VALUES ('arm-1', 'azure'); + +statement ok +INSERT INTO sph_arcm_tpid VALUES (10, 200); + +statement ok +INSERT INTO sph_arcm_cust VALUES (200, 21); + +statement ok +CREATE MATERIALIZED VIEW sph_arcm_mv AS + WITH bff AS ( + SELECT msf.id, acd.sub_id, + coalesce(hw.provider, 'N/A') AS provider + FROM sph_arcm_msf msf + INNER JOIN sph_arcm_acd acd ON msf.coll_key = acd.coll_key + LEFT JOIN sph_arcm_hw hw ON acd.arm_id = hw.arm_id + ) + SELECT bff.id, + coalesce(cust.cust_key, 1) AS cust_key + FROM bff + LEFT JOIN sph_arcm_tpid tpid ON bff.sub_id = tpid.sub_id + LEFT JOIN sph_arcm_cust cust ON coalesce(tpid.tp_id, -1) = cust.tp_id; + +statement ok +INSERT INTO sph_arcm_msf VALUES (2, 100); + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'sph_arcm_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%::%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%`%' + AND string_agg(sql, ' ' ORDER BY stmt_order) + LIKE '%INSERT INTO openivm_delta_sph_arcm_mv (id, cust_key, openivm_multiplicity)%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'sph_arcm_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# ========================================== +# Spark hardening for the reduced "int_instance_status_transaction" shape: a +# UNION ALL of two LEFT JOIN branches partitioned by an IS NOT NULL / IS NULL +# predicate on the joined side must still compile cleanly under +# target_dialect=spark without a stale child-alias reference leaking into the +# emitted SQL (the failure this shape covers, fixed upstream in +# `third_party/lpts` at commit 77ed5e5, surfaced as a Binder Error referencing +# a column that no longer existed in the rewritten plan). +# ========================================== +statement ok +CREATE TABLE sph_inti_isf(id INT, k INT); + +statement ok +CREATE TABLE sph_inti_ml(k INT, res VARCHAR); + +statement ok +INSERT INTO sph_inti_isf VALUES (1, 100), (2, 200); + +statement ok +INSERT INTO sph_inti_ml VALUES (100, 'machine-1'); + +statement ok +CREATE MATERIALIZED VIEW sph_inti_mv AS + SELECT isf.id, isf.k FROM sph_inti_isf isf LEFT JOIN sph_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NOT NULL + UNION ALL + SELECT isf.id, isf.k FROM sph_inti_isf isf LEFT JOIN sph_inti_ml ml ON isf.k = ml.k WHERE ml.res IS NULL; + +statement ok +INSERT INTO sph_inti_isf VALUES (3, 300); + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'sph_inti_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%::%' + AND string_agg(sql, ' ' ORDER BY stmt_order) + LIKE '%INSERT INTO openivm_delta_sph_inti_mv (id, k, openivm_left_key, openivm_multiplicity)%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%UNION ALL%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'sph_inti_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 diff --git a/third_party/lpts b/third_party/lpts index 592d469f..77ed5e58 160000 --- a/third_party/lpts +++ b/third_party/lpts @@ -1 +1 @@ -Subproject commit 592d469f305fd56cff625e05b1860c86f18ffeae +Subproject commit 77ed5e585f26e2702a7b2af3cc9e8623769256c8 From 724e0d27d70eab2524f6fe447ac97b78ddd73a29 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sat, 22 Aug 2026 22:56:08 +0000 Subject: [PATCH 07/18] Fix stale leaf table_index reuse in inclusion-exclusion delta substitution BuildInclusionExclusionTerms (src/delta/operators/join.cpp) substituted each per-mask delta leaf using leaves[i].get, a LogicalGet* captured once from the ORIGINAL, un-renumbered input.plan before that mask's own renumber_and_rebind_subtree pass. CreateDeltaGetNode/CompactDeltaNode reuse old_get->table_index verbatim for the replacement delta subtree, so the substituted node ended up at the STALE, pre-renumbering table_index while every other reference to that leaf within `term` (join conditions, transitioning-key guards, projection maps) had already been rebound to the FRESH, mask-specific index renumber_and_rebind_subtree assigned it. This mismatch was invisible for single-leaf-delta cases because the existing AppendMultiplicityToAncestorProjectionMaps "shift_stale_parent_indexes" patch fixes up the immediate parent join's own projection map to tolerate it. It was NOT masked once the substituted leaf feeds more than one join (e.g. msf join acd AND acd join hw) or a transitioning-key guard, since those other references are never patched and stay dangling -- surfacing as LPTS_UNSUPPORTED_COLUMN_REF once a batched, multi-table delta hit that shape. A single-row compile-only check cannot reach this: it takes a real CREATE + batched INSERT/DELETE/UPDATE + PRAGMA refresh across every base table to trigger more than one leaf changing at once. Fix: substitute using term's own, already-renumbered LogicalGet at leaves[i].path (via GetNodeAtPath), instead of the stale leaves[i].get, so the replacement delta node's table_index is always consistent with the rest of `term`. BuildRegularJoinTerms/CompileRegularLeafDelta and the transitioning-key-guard delta path were checked and already re-derive their leaf/get pointers from term's own renumbered tree (the former via its own post-renumbering CollectJoinLeaves call, the latter via FindGetInSubtree/GetNodeAtPath walks of term's live join tree), so they did not need the same fix. Verified against test/sql/cascade_simple_projection_join.test's cspj_arcm_mv integration case (real CREATE + batched multi-table INSERT/DELETE/UPDATE + PRAGMA refresh + bidirectional EXCEPT ALL), which failed with LPTS_UNSUPPORTED_COLUMN_REF before this fix and passes after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/delta/operators/join.cpp | 64 ++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 6 deletions(-) diff --git a/src/delta/operators/join.cpp b/src/delta/operators/join.cpp index ca60db8a..ec33edda 100644 --- a/src/delta/operators/join.cpp +++ b/src/delta/operators/join.cpp @@ -834,20 +834,61 @@ void AppendMultiplicityToAncestorProjectionMaps(unique_ptr &ter if (mul_idx == DConstants::INVALID_INDEX) { continue; } + // Appending to this join's own left_projection_map grows its LEFT + // contribution width, which shifts the absolute position where its RIGHT + // contribution starts within its own combined GetColumnBindings(). A + // grandparent ancestor may already have a projection map entry referencing + // (by that now-stale absolute position) a column from this join's right + // side; left as-is, that entry would silently start pointing at the + // newly-inserted column instead, dropping the real column it used to + // select. Appending to right_projection_map never has this effect: right + // contributions are always placed last, so a new entry there only ever + // extends the combined output with a brand-new highest index. + idx_t old_width = proj_map.size(); + auto shift_stale_parent_indexes = [&](idx_t added) { + if (child_side != 0 || added == 0 || depth == 0) { + return; + } + size_t parent_side = leaf_path[depth - 1]; + auto *parent_join = dynamic_cast(ancestors[depth - 1]); + if (!parent_join || parent_side >= parent_join->children.size()) { + return; + } + auto &parent_map = + (parent_side == 0) ? parent_join->left_projection_map : parent_join->right_projection_map; + for (auto &parent_idx : parent_map) { + if (parent_idx >= old_width) { + parent_idx += added; + } + } + }; if (preserve_full_child) { idx_t projectable_count = MinValue(mul_idx + 1, child_bindings.size()); + idx_t added = 0; for (idx_t binding_idx = 0; binding_idx < projectable_count; binding_idx++) { if (std::find(proj_map.begin(), proj_map.end(), binding_idx) != proj_map.end()) { continue; } proj_map.push_back(binding_idx); + added++; OPENIVM_DEBUG_PRINT("[%s] Preserved child col %lu in immediate %s proj_map\n", context_label, (unsigned long)binding_idx, child_side == 0 ? "left" : "right"); } - } else if (std::find(proj_map.begin(), proj_map.end(), mul_idx) == proj_map.end()) { - proj_map.push_back(mul_idx); - OPENIVM_DEBUG_PRINT("[%s] Added mul col %lu to ancestor %s proj_map\n", context_label, - (unsigned long)mul_idx, child_side == 0 ? "left" : "right"); + shift_stale_parent_indexes(added); + } else { + // proj_map entries are positions into the child's *current* combined + // GetColumnBindings(). Testing raw index membership of mul_idx against + // proj_map can alias onto an unrelated pre-existing entry that now shares + // the same numeric position after a deeper level's own map grew. Compare + // by column identity against what this ancestor currently exposes instead + // of trusting the raw index. + auto exposed = join->GetColumnBindings(); + if (std::find(exposed.begin(), exposed.end(), mul_binding) == exposed.end()) { + proj_map.push_back(mul_idx); + shift_stale_parent_indexes(1); + OPENIVM_DEBUG_PRINT("[%s] Added mul col %lu to ancestor %s proj_map\n", context_label, + (unsigned long)mul_idx, child_side == 0 ? "left" : "right"); + } } join->ResolveOperatorTypes(); } @@ -1497,9 +1538,20 @@ BuildInclusionExclusionTerms(DeltaOperatorInput input, ClientContext &context, B for (size_t i = 0; i < N; i++) { if (mask & (1ULL << i)) { if (leaves[i].get) { - DeltaGetResult delta_i = CreateDeltaGetNode(context, binder, leaves[i].get, input.context.view); + // leaves[] was collected once on the ORIGINAL input.plan, before this + // mask's own renumber_and_rebind_subtree pass, so leaves[i].get is a + // stale pointer carrying the pre-renumbering table_index. The rest of + // `term` (join conditions, transitioning-key guards, etc.) was rebound + // to the FRESH per-term index, so the replacement delta node -- which + // reuses old_get->table_index verbatim -- must be built from term's own, + // already-renumbered GET at this leaf's (renumbering-invariant) path, + // not from leaves[i].get, or every reference elsewhere in `term` to this + // leaf's fresh index is left dangling. + auto &leaf_node_ref = GetNodeAtPath(term, leaves[i].path); + auto &term_local_get = leaf_node_ref->Cast(); + DeltaGetResult delta_i = CreateDeltaGetNode(context, binder, &term_local_get, input.context.view); mul_bindings.push_back(delta_i.mul_binding); - GetNodeAtPath(term, leaves[i].path) = std::move(delta_i.node); + leaf_node_ref = std::move(delta_i.node); UpdateParentProjectionMap(term, leaves[i], delta_i.mul_binding); } else { auto &subtree_ref = GetNodeAtPath(term, leaves[i].path); From f568ea3ebd598e50b33ae395e49dfd05d21ff6e8 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sun, 23 Aug 2026 07:28:26 +0000 Subject: [PATCH 08/18] Advance lpts pin to 754c797 and add HUGEINT/left-deep UNION canaries Advance third_party/lpts from 77ed5e5 to 754c797781c9b56429e05c1781cea5ca99c628e8 ("Fix bounded Spark HUGEINT and UNION aliases", mdrakiburrahman/lpts branch dev/mdrrahman/spark-add-months), preserving the existing fork URL contract in .gitmodules. Extend test/sql/compile_spark_dialect_hardening.test with two new canary reductions exercising the OpenIVM/LPTS integration boundary via openivm_compile_with_facts(..., compile_only=true, force_view_delta_cascade=true): - sph_hugeint_*: a plain (non-aggregate) projection widening BIGINT to HUGEINT via COALESCE(CAST(amount AS HUGEINT), 0). Before this pin, ANY cast to HUGEINT under target_dialect=spark unconditionally raised LPTS_UNSUPPORTED_TYPE; the fix maps a provably-bounded HUGEINT cast to Spark DECIMAL(38,0). A literal COALESCE(SUM(bigint), 0) is classified GROUP_RECOMPUTE by OpenIVM and bypasses LPTS entirely, so this canary uses the equivalent non-aggregate shape that is classified SIMPLE_PROJECTION and does reach LPTS. Verified via A/B pin flip: the exact same view throws LPTS_UNSUPPORTED_TYPE on 77ed5e5 and compiles to refresh_type=2 SIMPLE_PROJECTION with SQL containing DECIMAL(38,0) on 754c797. - sph_ms_*: a literal three-way (left-deep) UNION ALL of LEFT JOIN branches partitioned by a mutually exclusive predicate on the joined side, matching the shape of upstream lpts commit 754c797's own test/sql/union.test regression (a left-deep "machine status" UNION ALL carrying duplicated key/multiplicity output columns). Asserts SIMPLE_PROJECTION classification and correct binding of the trailing openivm_left_key/openivm_multiplicity columns across the left-deep chain. Add the same sph_ms_* shape (as cspj_ms_*) to test/sql/cascade_simple_projection_join.test with a real CREATE + batched multi-table INSERT/DELETE/UPDATE + PRAGMA refresh, asserting bidirectional EXCEPT ALL bag-equality against the view definition, per the existing cspj_arcm_mv/cspj_inti_mv convention in that file. Investigation note: extensive probing (15+ constructions covering N-ary join-delta cascades at 2-5 simultaneous leaves, literal N-ary UNION ALL, union-of-unions nesting, column-pruning bait, and real batched-DML + PRAGMA refresh cycles, cross-checked with temporary debug instrumentation in third_party/lpts/src/lpts_ast_builder.cpp, since reverted) did not reproduce LPTS's internal "trailing binding" condition (GetColumnBindings().size() > types.size() on a LOGICAL_UNION node) via OpenIVM's current native construction code; AssembleJoinUnionAll and CompileUnionDelta were confirmed to keep types and bindings self-consistent at construction time for every topology tried. The added sph_ms_*/cspj_ms_* tests therefore verify the observable, task-required integration-boundary contract (SIMPLE_PROJECTION classification, never COMPILE_FAILED/ FULL_REFRESH, correct left-deep UNION column binding, bidirectional bag equality) for this shape family under the new pin, rather than proving the exact internal LPTS code path fires; no native OpenIVM defect was found or hidden. Verified: format-check clean; targeted suite (compile_refresh.test, compile_spark_dialect_hardening.test, cascade_group_recompute_delta.test, cascade_window_partition_delta.test, cascade_simple_projection_join.test) 352/352 assertions passing (up from the prior 320/320 baseline); full make test 10172/10172 assertions passing across 84 test cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/sql/cascade_simple_projection_join.test | 125 ++++++++++++++++++ test/sql/compile_spark_dialect_hardening.test | 120 +++++++++++++++++ third_party/lpts | 2 +- 3 files changed, 246 insertions(+), 1 deletion(-) diff --git a/test/sql/cascade_simple_projection_join.test b/test/sql/cascade_simple_projection_join.test index 9aa421c7..147e136b 100644 --- a/test/sql/cascade_simple_projection_join.test +++ b/test/sql/cascade_simple_projection_join.test @@ -782,3 +782,128 @@ SELECT COUNT(*) FROM ( ); ---- 0 + +# Reduced "machine-status left-deep UNION" shape: a literal three-way +# (left-deep) UNION ALL of LEFT JOIN branches, each partitioned by a mutually +# exclusive predicate on the joined side (active / inactive / unmatched). +# Internally this compiles to a nested union whose own union is itself one +# arm of a further union (`(A UNION ALL B) UNION ALL C`), carrying duplicated +# `openivm_left_key`/`openivm_multiplicity` output columns at every level of +# the chain. This matches the shape of upstream `third_party/lpts` commit +# 754c797's own `test/sql/union.test` regression ("OpenIVM join-delta UNION +# terms carry ... a duplicated hidden left key and multiplicity. A left-deep +# UNION must preserve and bind all ... positions"); the fix retains a +# trailing rewritten UNION binding as an alias of its physical multiplicity +# output rather than losing it. Compiled with force_view_delta_cascade under +# target_dialect=spark, this must classify SIMPLE_PROJECTION -- never +# COMPILE_FAILED/FULL_REFRESH -- and a real batched multi-table +# CREATE + INSERT/DELETE/UPDATE + PRAGMA refresh must stay in bidirectional +# bag-equality with the view definition. +statement ok +CREATE TABLE cspj_ms_terms(event_time INT, machine_arm_id VARCHAR, customer_key INT); + +statement ok +CREATE TABLE cspj_ms_status(machine_arm_id VARCHAR, status_label VARCHAR); + +statement ok +INSERT INTO cspj_ms_terms VALUES (10, 'arm-1', 1), (20, 'arm-2', 6); + +statement ok +INSERT INTO cspj_ms_status VALUES ('arm-1', 'active'); + +statement ok +CREATE MATERIALIZED VIEW cspj_ms_mv AS + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'active' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'inactive' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label IS NULL; + +statement ok +INSERT INTO cspj_ms_terms VALUES (30, 'arm-3', 11); + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'cspj_ms_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE + WHEN contains(content, 'INSERT INTO openivm_delta_cspj_ms_mv') AND + contains(content, 'openivm_delta_cspj_ms_terms') AND + contains(content, 'openivm_delta_cspj_ms_status') AND + contains(content, 'UNION ALL') AND + NOT contains(content, 'SELECT NULL::') + THEN 1 ELSE 0 + END +FROM read_text('__TEST_DIR__/openivm_upsert_queries_cspj_ms_mv.sql'); +---- +1 + +statement ok +INSERT INTO cspj_ms_terms VALUES (40, 'arm-4', 16); + +statement ok +INSERT INTO cspj_ms_status VALUES ('arm-3', 'active'), ('arm-4', NULL); + +statement ok +DELETE FROM cspj_ms_terms WHERE event_time = 20; + +statement ok +UPDATE cspj_ms_status SET status_label = 'inactive' WHERE machine_arm_id = 'arm-1'; + +statement ok +PRAGMA refresh('cspj_ms_mv'); + +query I +SELECT COUNT(*) FROM ( + SELECT * FROM cspj_ms_mv + EXCEPT ALL + ( + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'active' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'inactive' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label IS NULL + ) +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + ( + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'active' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'inactive' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM cspj_ms_terms t LEFT JOIN cspj_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label IS NULL + ) + EXCEPT ALL + SELECT * FROM cspj_ms_mv +); +---- +0 diff --git a/test/sql/compile_spark_dialect_hardening.test b/test/sql/compile_spark_dialect_hardening.test index 0a496bf4..8d4b180b 100644 --- a/test/sql/compile_spark_dialect_hardening.test +++ b/test/sql/compile_spark_dialect_hardening.test @@ -332,3 +332,123 @@ FROM openivm_compile_with_facts( WHERE stmt_kind = 'data'; ---- 1 + +# ========================================== +# Spark hardening for the "bounded HUGEINT" canary: a plain (non-aggregate) +# projection that widens a BIGINT to HUGEINT via COALESCE(CAST(...), 0) must +# still compile to a real incremental cascade delta under +# target_dialect=spark. Before `third_party/lpts` was advanced to commit +# 754c797 ("Fix bounded Spark HUGEINT and UNION aliases"), rendering ANY cast +# to HUGEINT unconditionally raised LPTS_UNSUPPORTED_TYPE for Spark, even +# when the source type (BIGINT here) provably fits within Spark's +# DECIMAL(38,0); the fix maps a provably-bounded HUGEINT cast/literal to +# DECIMAL(38,0) instead of failing the compile. Note a literal +# COALESCE(SUM(...), 0) is classified GROUP_RECOMPUTE by OpenIVM (bypassing +# LPTS entirely, echoing native SQL), so this canary uses an equivalent +# non-aggregate COALESCE(CAST(... AS HUGEINT), 0) shape that is classified +# SIMPLE_PROJECTION and does go through LPTS. +# ========================================== +statement ok +CREATE TABLE sph_hugeint_src(id INT, amount BIGINT); + +statement ok +INSERT INTO sph_hugeint_src VALUES (1, 5000000000), (2, NULL); + +statement ok +CREATE MATERIALIZED VIEW sph_hugeint_mv AS + SELECT id, COALESCE(CAST(amount AS HUGEINT), 0) AS wide_amount + FROM sph_hugeint_src; + +statement ok +INSERT INTO sph_hugeint_src VALUES (3, 2000000000); + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'sph_hugeint_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%DECIMAL(38,0)%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '% HUGEINT%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'sph_hugeint_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# ========================================== +# Spark hardening for the "machine-status left-deep UNION" canary: a literal +# three-way (left-deep) UNION ALL of LEFT JOIN branches, each partitioned by a +# mutually exclusive predicate on the joined side, must still compile to a +# real incremental cascade delta under target_dialect=spark, correctly +# binding every column -- including the trailing duplicated +# `openivm_left_key`/`openivm_multiplicity` output columns -- at every level +# of the left-deep chain (internally: a nested union node whose own union is +# itself one arm of a further union, e.g. `(A UNION ALL B) UNION ALL C`). +# This is upstream `third_party/lpts` commit 754c797's second fix ("retain +# trailing rewritten UNION bindings as aliases of their physical multiplicity +# output"); unlike the two-way `sph_inti_mv` shape above, this exercises an +# actual left-deep union chain, matching the shape of LPTS's own +# `test/sql/union.test` regression (a left-deep "machine status" UNION ALL +# carrying duplicated key/multiplicity output columns). +# ========================================== +statement ok +CREATE TABLE sph_ms_terms(event_time INT, machine_arm_id VARCHAR, customer_key INT); + +statement ok +CREATE TABLE sph_ms_status(machine_arm_id VARCHAR, status_label VARCHAR); + +statement ok +INSERT INTO sph_ms_terms VALUES (10, 'arm-1', 1), (20, 'arm-2', 6); + +statement ok +INSERT INTO sph_ms_status VALUES ('arm-1', 'active'); + +statement ok +CREATE MATERIALIZED VIEW sph_ms_mv AS + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM sph_ms_terms t LEFT JOIN sph_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'active' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM sph_ms_terms t LEFT JOIN sph_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label = 'inactive' + UNION ALL + SELECT t.event_time, t.machine_arm_id, t.customer_key + FROM sph_ms_terms t LEFT JOIN sph_ms_status s ON t.machine_arm_id = s.machine_arm_id + WHERE s.status_label IS NULL; + +statement ok +INSERT INTO sph_ms_terms VALUES (30, 'arm-3', 11); + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'sph_ms_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +2 SIMPLE_PROJECTION + +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%::%' + AND string_agg(sql, ' ' ORDER BY stmt_order) + LIKE '%INSERT INTO openivm_delta_sph_ms_mv (event_time, machine_arm_id, customer_key, openivm_left_key, openivm_multiplicity)%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%UNION ALL%UNION ALL%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'sph_ms_mv', + '{"target_dialect":"spark","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 diff --git a/third_party/lpts b/third_party/lpts index 77ed5e58..754c7977 160000 --- a/third_party/lpts +++ b/third_party/lpts @@ -1 +1 @@ -Subproject commit 77ed5e585f26e2702a7b2af3cc9e8623769256c8 +Subproject commit 754c797781c9b56429e05c1781cea5ca99c628e8 From 1e2fed1fc41e23cd8cf4409dc5975f2fd7e0d8a0 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sun, 23 Aug 2026 11:59:15 +0000 Subject: [PATCH 09/18] Emit cascade view delta from unscopable WINDOW_PARTITION/GROUP_RECOMPUTE recomputes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 32-way local OpenIVM Spark canary demoted 36 of 58 incremental materialized views to FULL_REFRESH with reason `non_cascade_upstream:non_cascade:`; 54 nodes reported a non-cascade upstream. The dominant roots were nine views that the classifier kept as WINDOW_PARTITION (`window_partition_kept`) yet which reported `emits_cascade_view_delta='false'`: operating_system_dim, arc_sql_server_instance_{version,edition,type,mode}_dim, subscription_{offer,workload}_type_dim, infrastructure_dim and arc_sql_server_target_instances_mat_view — together ~39 of the non-cascade upstream references. Root cause: the partial-recompute compilers degrade to a plain full recompute whenever the affected partition/group key set cannot be scoped from the source deltas, and that fallback silently discarded a requested `CompileFacts::force_view_delta_cascade`: * refresh_compiler_aux.cpp CompileWindowRecompute — no affected keys and (no partition columns or no partition delta spec). This is the unpartitioned surrogate-key shape `CAST(ROW_NUMBER() OVER (ORDER BY ...) AS INT)` used by seven of the nine dims, and the computed-partition-key shape (`PARTITION BY lower(a) || '_' || lower(b)`) used by operating_system_dim. * refresh_window.cpp BuildWindowPartitionRefresh — DuckLake fallback, lineage UNSAFE fallback, and lineage-incomplete multi-source fallback (arc_sql_server_target_instances_mat_view). * refresh_compiler.cpp CompileGroupRecompute — degenerate no-group / no-delta-spec fallback. refresh_sql.cpp sets `recompute_handles_own_cascade_delta` for WINDOW_PARTITION/GROUP_RECOMPUTE when a cascade delta is requested, which suppresses the generic snapshot companion on the assumption that the recompute emits its own delta. For these fallbacks that assumption was false, so the compiled program contained no `openivm_delta_` write at all and every downstream view had to fall back to a full refresh. Fix: add CompileFullRecomputeWithCascadeDelta, used only when the caller explicitly requested a cascade delta. It brackets the existing recompute with `openivm_old_` / `openivm_new_` temp snapshots and publishes the exact signed multiset delta (whole old content at multiplicity -1, whole new content at +1) via the existing BuildSignedMultisetDeltaInsertSQL, then drops the temps. That is exactly `new_bag - old_bag`: unchanged rows contribute cancelling -1/+1 pairs, so bag semantics and inclusion-exclusion behaviour are preserved. The statement shapes match the non-degenerate WINDOW_PARTITION/GROUP_RECOMPUTE cascade branches already emitted today. Nothing is relabeled and nothing is force-refreshed: the refresh type stays WINDOW_PARTITION / GROUP_RECOMPUTE, data-table maintenance is unchanged, unsupported plans still fail or classify as FULL_REFRESH explicitly, and `PRAGMA refresh` (default CompileFacts, cascade off) is bit-for-bit unaffected. Adds test/sql/cascade_window_unscopable_delta.test covering the three canary query shapes (global surrogate-key window, computed partition key, multi-source join with a computed partition key), the no-cascade program shape, an end-to-end cascade in which the emitted program is executed after batched insert/update/delete and a downstream view is refreshed incrementally off the delta, bag-multiplicity preservation on a duplicate-heavy view, and the preserved unsupported-plan behaviour. The test fails on the pre-fix build at the first cascade-shape assertion. Native gate: 170/170 test cases, 20494 assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/include/upsert/refresh_compiler.hpp | 22 + src/upsert/refresh_compiler.cpp | 27 +- src/upsert/refresh_compiler_aux.cpp | 6 +- src/upsert/refresh_window.cpp | 11 +- test/sql/cascade_window_unscopable_delta.test | 529 ++++++++++++++++++ 5 files changed, 591 insertions(+), 4 deletions(-) create mode 100644 test/sql/cascade_window_unscopable_delta.test diff --git a/src/include/upsert/refresh_compiler.hpp b/src/include/upsert/refresh_compiler.hpp index ee06f309..76aeaa0c 100644 --- a/src/include/upsert/refresh_compiler.hpp +++ b/src/include/upsert/refresh_compiler.hpp @@ -58,6 +58,28 @@ string CompileWindowRecompute(const string &view_name, const string &view_query_ const vector &column_names = {}, bool running_window_incremental = false); string CompileFullRecompute(const string &view_name, const string &view_query_sql, const string &catalog_prefix = ""); +/// Full recompute of `openivm_data_` that ALSO emits the exact signed +/// multiset view-delta into `openivm_delta_`. +/// +/// The partial-recompute paths (`WINDOW_PARTITION`, `GROUP_RECOMPUTE`) degrade to +/// a full recompute whenever the affected partition/group key set cannot be +/// scoped from the source deltas — an unpartitioned surrogate-key +/// `ROW_NUMBER() OVER (ORDER BY ...)`, a partition key that is a computed +/// expression absent from every source delta table, or incomplete multi-source +/// lineage. The view keeps its `WINDOW_PARTITION` / `GROUP_RECOMPUTE` +/// classification, so a caller that asked for a cascade delta +/// (`CompileFacts::force_view_delta_cascade`) would otherwise receive a program +/// that writes no `openivm_delta_` rows at all, and every downstream MV +/// would have to be demoted to a full refresh. +/// +/// Retracting the whole pre-refresh content at multiplicity -1 and adding the +/// whole post-refresh content at +1 is the exact Z-set delta of the view +/// (`new_bag - old_bag`): unchanged rows contribute cancelling -1/+1 pairs, so +/// bag semantics are preserved exactly. Statement shapes mirror the +/// `CompileWindowRecompute` / `CompileGroupRecompute` cascade branches. +string CompileFullRecomputeWithCascadeDelta(const string &view_name, const string &view_query_sql, + const string &catalog_prefix = ""); + /// Group-level partial recompute, used by `RefreshType::GROUP_RECOMPUTE` /// (inner-DISTINCT under aggregate). For each base table T_i with a non-empty /// delta, builds a "view query with T_i restricted to its delta" variant by diff --git a/src/upsert/refresh_compiler.cpp b/src/upsert/refresh_compiler.cpp index 66ecb0db..3b2b1f37 100644 --- a/src/upsert/refresh_compiler.cpp +++ b/src/upsert/refresh_compiler.cpp @@ -1455,6 +1455,28 @@ string CompileFullRecompute(const string &view_name, const string &view_query_sq return SqlUtils::BuildFullRecomputeSQL(data_table, view_query_sql); } +string CompileFullRecomputeWithCascadeDelta(const string &view_name, const string &view_query_sql, + const string &catalog_prefix) { + string data_table = catalog_prefix + SqlUtils::QuoteIdentifier(IncrementalTableNames::DataTableName(view_name)); + string delta_table = catalog_prefix + SqlUtils::QuoteIdentifier(SqlUtils::DeltaName(view_name)); + string old_temp_table = SqlUtils::QuoteIdentifier(string(openivm::TEMP_TABLE_PREFIX) + view_name); + string new_temp_table = SqlUtils::QuoteIdentifier(string("openivm_new_") + view_name); + + string sql; + sql += "CREATE OR REPLACE TEMP TABLE " + old_temp_table + " AS\nSELECT * FROM " + data_table + " openivm_old;\n\n"; + sql += "CREATE OR REPLACE TEMP TABLE " + new_temp_table + " AS\nSELECT * FROM (" + view_query_sql + + ") openivm_recompute;\n\n"; + sql += "DELETE FROM " + data_table + ";\n"; + sql += "INSERT INTO " + data_table + "\nSELECT * FROM " + new_temp_table + ";\n"; + sql += "\n" + BuildSignedMultisetDeltaInsertSQL(delta_table, old_temp_table, new_temp_table); + sql += "DROP TABLE IF EXISTS " + old_temp_table + ";\n"; + sql += "DROP TABLE IF EXISTS " + new_temp_table + ";\n"; + OPENIVM_DEBUG_PRINT("[CompileFullRecomputeWithCascadeDelta] unscopable recompute for '%s' — emitting signed " + "whole-view cascade delta\n", + view_name.c_str()); + return sql; +} + string CompileGroupRecompute(const string &view_name, const string &view_query_sql, const vector &group_columns, const vector &delta_table_specs, const string &catalog_prefix, const string &lpts_table_prefix, bool emit_cascade_delta, @@ -1462,8 +1484,11 @@ string CompileGroupRecompute(const string &view_name, const string &view_query_s string data_table = catalog_prefix + SqlUtils::QuoteIdentifier(IncrementalTableNames::DataTableName(view_name)); // No GROUP BY columns or no source deltas registered → can't scope; fall back to full. + // A cascade delta was still requested, so emit the whole-view signed delta rather than + // silently producing a program with no `openivm_delta_` rows. if (group_columns.empty() || delta_table_specs.empty()) { - return CompileFullRecompute(view_name, view_query_sql, catalog_prefix); + return emit_cascade_delta ? CompileFullRecomputeWithCascadeDelta(view_name, view_query_sql, catalog_prefix) + : CompileFullRecompute(view_name, view_query_sql, catalog_prefix); } string group_csv = SqlUtils::JoinQuotedColumns(group_columns); diff --git a/src/upsert/refresh_compiler_aux.cpp b/src/upsert/refresh_compiler_aux.cpp index a1fe3c0d..fb79b334 100644 --- a/src/upsert/refresh_compiler_aux.cpp +++ b/src/upsert/refresh_compiler_aux.cpp @@ -1600,7 +1600,11 @@ string CompileWindowRecompute(const string &view_name, const string &view_query_ bool running_window_incremental) { bool have_affected_keys = !affected_keys_sql.empty(); if (!have_affected_keys && (partition_columns.empty() || partition_delta_specs.empty())) { - return CompileFullRecompute(view_name, view_query_sql, catalog_prefix); + // No PARTITION BY (global surrogate-key window) or no partition key resolvable in any + // source delta table → nothing to scope the recompute to. Keep the cascade delta the + // caller asked for so downstream MVs stay incremental. + return emit_cascade_delta ? CompileFullRecomputeWithCascadeDelta(view_name, view_query_sql, catalog_prefix) + : CompileFullRecompute(view_name, view_query_sql, catalog_prefix); } if (running_window_incremental) { auto suffix_sql = BuildRunningWindowSuffixRefreshSQL(view_name, view_query_sql, delta_ts_filter, catalog_prefix, diff --git a/src/upsert/refresh_window.cpp b/src/upsert/refresh_window.cpp index e77b4807..82f0f2cf 100644 --- a/src/upsert/refresh_window.cpp +++ b/src/upsert/refresh_window.cpp @@ -577,6 +577,9 @@ string BuildWindowPartitionRefresh(RefreshMetadata &metadata, Connection &con, c if (any_ducklake) { OPENIVM_DEBUG_PRINT( "[UPSERT] Compiling upsert for type: WINDOW_PARTITION (DuckLake, full recompute fallback)\n"); + if (emit_cascade_delta) { + return CompileFullRecomputeWithCascadeDelta(view_name, view_query_sql, internal_catalog_prefix); + } return "DELETE FROM " + data_table + ";\n" + "INSERT INTO " + data_table + " " + view_query_sql + ";\n"; } auto lineage_result = BuildLineageStandardAffectedKeysSQL( @@ -585,7 +588,9 @@ string BuildWindowPartitionRefresh(RefreshMetadata &metadata, Connection &con, c if (lineage_result == LineageAffectedKeysResult::UNSAFE) { OPENIVM_DEBUG_PRINT("[UPSERT] WINDOW_PARTITION lineage is unsafe for '%s' — full recompute fallback\n", view_name.c_str()); - return CompileFullRecompute(view_name, view_query_sql, internal_catalog_prefix); + return emit_cascade_delta + ? CompileFullRecomputeWithCascadeDelta(view_name, view_query_sql, internal_catalog_prefix) + : CompileFullRecompute(view_name, view_query_sql, internal_catalog_prefix); } have_lineage_affected_keys = lineage_result == LineageAffectedKeysResult::AVAILABLE; if (!have_lineage_affected_keys && delta_table_names.size() > 1 && @@ -593,7 +598,9 @@ string BuildWindowPartitionRefresh(RefreshMetadata &metadata, Connection &con, c OPENIVM_DEBUG_PRINT("[UPSERT] WINDOW_PARTITION lineage incomplete for '%s' (%zu sources) — full recompute " "fallback\n", view_name.c_str(), delta_table_names.size()); - return CompileFullRecompute(view_name, view_query_sql, internal_catalog_prefix); + return emit_cascade_delta + ? CompileFullRecomputeWithCascadeDelta(view_name, view_query_sql, internal_catalog_prefix) + : CompileFullRecompute(view_name, view_query_sql, internal_catalog_prefix); } OPENIVM_DEBUG_PRINT("[UPSERT] Compiling upsert for type: WINDOW_PARTITION (%zu partition cols, lineage keys: %s)\n", partition_cols.size(), have_lineage_affected_keys ? "yes" : "no"); diff --git a/test/sql/cascade_window_unscopable_delta.test b/test/sql/cascade_window_unscopable_delta.test new file mode 100644 index 00000000..38f93446 --- /dev/null +++ b/test/sql/cascade_window_unscopable_delta.test @@ -0,0 +1,529 @@ +# name: test/sql/cascade_window_unscopable_delta.test +# description: WINDOW_PARTITION plans whose partition keys cannot be scoped to a source delta still emit signed cascade deltas when facts.force_view_delta_cascade=true +# group: [sql] + +require openivm + +statement ok +SET openivm_files_path='__TEST_DIR__'; + +# ========================================================================== +# Case 1 - global surrogate key: ROW_NUMBER() OVER (ORDER BY ...) with no +# PARTITION BY. The classifier still selects WINDOW_PARTITION, but there are +# no partition columns to scope an affected-key set with, so the compiler +# falls back to a full recompute of the view body. That fallback must still +# honour an explicitly requested cascade view delta - otherwise every +# downstream materialized view observes "no delta" and gets demoted to a +# full refresh. +# ========================================================================== + +statement ok +CREATE TABLE uwd_src (id INT, grp VARCHAR, val INT); + +statement ok +INSERT INTO uwd_src VALUES + (1, 'a', 10), + (2, 'a', 20), + (3, 'b', 5), + (4, 'b', 15); + +statement ok +CREATE MATERIALIZED VIEW uwd_dim AS + SELECT CAST(ROW_NUMBER() OVER (ORDER BY grp, id) AS INT) AS dim_key, id, grp, val + FROM uwd_src; + +# The refresh type is unchanged by the cascade request: no relabeling, no +# demotion to FULL_REFRESH. +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'uwd_dim', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":false}' +) +LIMIT 1; +---- +5 WINDOW_PARTITION + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'uwd_dim', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +5 WINDOW_PARTITION + +# Without a cascade request the program is the plain in-place recompute and +# must not write into the view's own delta table. +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%DELETE FROM %openivm_data_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%INSERT INTO %openivm_data_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%INSERT INTO openivm_delta_uwd_dim%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'uwd_dim', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":false}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# With a cascade request the same recompute must be bracketed by old/new +# snapshots and publish the signed whole-view delta. +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CREATE OR REPLACE TEMP TABLE openivm_old_uwd_dim AS%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CREATE OR REPLACE TEMP TABLE openivm_new_uwd_dim AS%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%DELETE FROM %openivm_data_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%INSERT INTO %openivm_data_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%INSERT INTO openivm_delta_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%-1%INTEGER%openivm_old_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%UNION ALL%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%1%INTEGER%openivm_new_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%DROP TABLE IF EXISTS openivm_old_uwd_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%DROP TABLE IF EXISTS openivm_new_uwd_dim%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'uwd_dim', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# ========================================================================== +# Case 2 - computed partition key: PARTITION BY lower(a) || '_' || lower(b) +# is not a column of any source delta table, so no partition delta spec can +# be built. Same requirement. +# ========================================================================== + +statement ok +CREATE TABLE uwd_os_src (os_name VARCHAR, os_sku VARCHAR); + +statement ok +INSERT INTO uwd_os_src VALUES ('linux', 'a'), ('windows', 'b'), ('linux', 'c'); + +statement ok +CREATE MATERIALIZED VIEW uwd_os_dim AS + SELECT CAST(ROW_NUMBER() OVER (ORDER BY os_full) AS INT) AS os_key, os_full, os_name, os_sku + FROM ( + SELECT lower(os_name) || '_' || lower(os_sku) AS os_full, os_name, os_sku, + ROW_NUMBER() OVER (PARTITION BY lower(os_name) || '_' || lower(os_sku) ORDER BY os_name) AS rn + FROM uwd_os_src + ) t + WHERE rn = 1; + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'uwd_os_dim', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +5 WINDOW_PARTITION + +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%INSERT INTO openivm_delta_uwd_os_dim%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'uwd_os_dim', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":false}' +) +WHERE stmt_kind = 'data'; +---- +1 + +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CREATE OR REPLACE TEMP TABLE openivm_old_uwd_os_dim AS%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CREATE OR REPLACE TEMP TABLE openivm_new_uwd_os_dim AS%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%INSERT INTO openivm_delta_uwd_os_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%-1%INTEGER%openivm_old_uwd_os_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%1%INTEGER%openivm_new_uwd_os_dim%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%DROP TABLE IF EXISTS openivm_new_uwd_os_dim%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'uwd_os_dim', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# ========================================================================== +# Case 3 - multi-source join whose partition key is a computed expression: +# the window partition lineage cannot cover every source delta table. +# ========================================================================== + +statement ok +CREATE TABLE uwd_inst (instance_id INT, sub_id INT, region VARCHAR); + +statement ok +CREATE TABLE uwd_sub (sub_id INT, sub_name VARCHAR); + +statement ok +INSERT INTO uwd_inst VALUES (1, 10, 'eus'), (2, 10, 'wus'), (3, 11, 'eus'); + +statement ok +INSERT INTO uwd_sub VALUES (10, 's10'), (11, 's11'); + +statement ok +CREATE MATERIALIZED VIEW uwd_target AS + SELECT CAST(ROW_NUMBER() OVER (PARTITION BY upper(i.region) ORDER BY i.instance_id) AS INT) AS rn, + i.instance_id, i.region, s.sub_name + FROM uwd_inst i JOIN uwd_sub s ON i.sub_id = s.sub_id; + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'uwd_target', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +5 WINDOW_PARTITION + +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CREATE OR REPLACE TEMP TABLE openivm_old_uwd_target AS%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CREATE OR REPLACE TEMP TABLE openivm_new_uwd_target AS%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%INSERT INTO openivm_delta_uwd_target%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%-1%INTEGER%openivm_old_uwd_target%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%1%INTEGER%openivm_new_uwd_target%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'uwd_target', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +# ========================================================================== +# End-to-end: run the emitted cascade program for uwd_dim after a batch of +# conflicting DML (insert + update + delete applied before a single refresh) +# and prove (a) the signed delta reconstructs the new view content from the +# old content under bag semantics, and (b) a downstream materialized view +# refreshes incrementally off that delta and stays bidirectionally equal to +# its base query. PRAGMA refresh() uses the default CompileFacts (cascade +# off), so the cascade program is executed explicitly here exactly as the +# engine driver executes it. +# ========================================================================== + +statement ok +CREATE MATERIALIZED VIEW uwd_down AS + SELECT dim_key, grp, val FROM uwd_dim WHERE val > 6; + +statement ok +INSERT INTO uwd_src VALUES (5, 'a', 15), (6, 'c', 1); + +statement ok +DELETE FROM uwd_src WHERE id = 1; + +statement ok +UPDATE uwd_src SET val = 12 WHERE id = 3; + +statement ok +CREATE TABLE uwd_old_snapshot AS SELECT * FROM openivm_data_uwd_dim; + +statement ok +CREATE OR REPLACE TEMP TABLE openivm_old_uwd_dim AS +SELECT * FROM openivm_data_uwd_dim openivm_old; + +statement ok +CREATE OR REPLACE TEMP TABLE openivm_new_uwd_dim AS +SELECT * FROM ( + SELECT CAST(ROW_NUMBER() OVER (ORDER BY grp, id) AS INT) AS dim_key, id, grp, val + FROM uwd_src +) openivm_recompute; + +statement ok +DELETE FROM openivm_data_uwd_dim; + +statement ok +INSERT INTO openivm_data_uwd_dim +SELECT * FROM openivm_new_uwd_dim; + +statement ok +INSERT INTO openivm_delta_uwd_dim +SELECT *, CAST(-1 AS INTEGER), CURRENT_TIMESTAMP FROM openivm_old_uwd_dim +UNION ALL +SELECT *, CAST(1 AS INTEGER), CURRENT_TIMESTAMP FROM openivm_new_uwd_dim; + +statement ok +DROP TABLE IF EXISTS openivm_old_uwd_dim; + +statement ok +DROP TABLE IF EXISTS openivm_new_uwd_dim; + +# The retraction leg is exactly the pre-refresh content (bag equality). +query I +SELECT COUNT(*) FROM ( + SELECT dim_key, id, grp, val FROM openivm_delta_uwd_dim WHERE openivm_multiplicity = -1 + EXCEPT ALL + SELECT dim_key, id, grp, val FROM uwd_old_snapshot +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + SELECT dim_key, id, grp, val FROM uwd_old_snapshot + EXCEPT ALL + SELECT dim_key, id, grp, val FROM openivm_delta_uwd_dim WHERE openivm_multiplicity = -1 +); +---- +0 + +# Applying the signed delta to the old content reproduces the new content +# exactly, in both directions. +query I +WITH applied AS ( + ( + SELECT dim_key, id, grp, val FROM uwd_old_snapshot + UNION ALL + SELECT dim_key, id, grp, val FROM openivm_delta_uwd_dim WHERE openivm_multiplicity = 1 + ) + EXCEPT ALL + SELECT dim_key, id, grp, val FROM openivm_delta_uwd_dim WHERE openivm_multiplicity = -1 +) +SELECT COUNT(*) FROM ( + SELECT dim_key, id, grp, val FROM applied + EXCEPT ALL + SELECT dim_key, id, grp, val FROM openivm_data_uwd_dim +); +---- +0 + +query I +WITH applied AS ( + ( + SELECT dim_key, id, grp, val FROM uwd_old_snapshot + UNION ALL + SELECT dim_key, id, grp, val FROM openivm_delta_uwd_dim WHERE openivm_multiplicity = 1 + ) + EXCEPT ALL + SELECT dim_key, id, grp, val FROM openivm_delta_uwd_dim WHERE openivm_multiplicity = -1 +) +SELECT COUNT(*) FROM ( + SELECT dim_key, id, grp, val FROM openivm_data_uwd_dim + EXCEPT ALL + SELECT dim_key, id, grp, val FROM applied +); +---- +0 + +# The refreshed view itself matches its base query. +query I +SELECT COUNT(*) FROM ( + SELECT dim_key, id, grp, val FROM openivm_data_uwd_dim + EXCEPT ALL + SELECT CAST(ROW_NUMBER() OVER (ORDER BY grp, id) AS INT) AS dim_key, id, grp, val FROM uwd_src +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + SELECT CAST(ROW_NUMBER() OVER (ORDER BY grp, id) AS INT) AS dim_key, id, grp, val FROM uwd_src + EXCEPT ALL + SELECT dim_key, id, grp, val FROM openivm_data_uwd_dim +); +---- +0 + +# The downstream view consumes the cascade delta incrementally and is +# bidirectionally equal to its base query. +statement ok +PRAGMA refresh('uwd_down'); + +query I +SELECT COUNT(*) FROM ( + SELECT dim_key, grp, val FROM openivm_data_uwd_down + EXCEPT ALL + SELECT dim_key, grp, val FROM ( + SELECT CAST(ROW_NUMBER() OVER (ORDER BY grp, id) AS INT) AS dim_key, id, grp, val FROM uwd_src + ) d + WHERE val > 6 +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + SELECT dim_key, grp, val FROM ( + SELECT CAST(ROW_NUMBER() OVER (ORDER BY grp, id) AS INT) AS dim_key, id, grp, val FROM uwd_src + ) d + WHERE val > 6 + EXCEPT ALL + SELECT dim_key, grp, val FROM openivm_data_uwd_down +); +---- +0 + +# ========================================================================== +# Duplicate-heavy view: the signed delta must carry exact multiplicities, +# not a de-duplicated set. +# ========================================================================== + +statement ok +CREATE TABLE uwd_bag_src (grp VARCHAR, val INT); + +statement ok +INSERT INTO uwd_bag_src VALUES ('a', 1), ('a', 1), ('a', 1), ('b', 2), ('b', 2); + +statement ok +CREATE MATERIALIZED VIEW uwd_bag_mv AS + SELECT grp, val, CAST(SUM(val) OVER () AS INT) AS total FROM uwd_bag_src; + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'uwd_bag_mv', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +5 WINDOW_PARTITION + +query I +SELECT CASE WHEN + string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%INSERT INTO openivm_delta_uwd_bag_mv%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%-1%INTEGER%openivm_old_uwd_bag_mv%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%CAST%1%INTEGER%openivm_new_uwd_bag_mv%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts( + 'uwd_bag_mv', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +WHERE stmt_kind = 'data'; +---- +1 + +statement ok +INSERT INTO uwd_bag_src VALUES ('a', 1), ('c', 3); + +statement ok +CREATE TABLE uwd_bag_old AS SELECT * FROM openivm_data_uwd_bag_mv; + +statement ok +CREATE OR REPLACE TEMP TABLE openivm_old_uwd_bag_mv AS +SELECT * FROM openivm_data_uwd_bag_mv openivm_old; + +statement ok +CREATE OR REPLACE TEMP TABLE openivm_new_uwd_bag_mv AS +SELECT * FROM ( + SELECT grp, val, CAST(SUM(val) OVER () AS INT) AS total FROM uwd_bag_src +) openivm_recompute; + +statement ok +DELETE FROM openivm_data_uwd_bag_mv; + +statement ok +INSERT INTO openivm_data_uwd_bag_mv +SELECT * FROM openivm_new_uwd_bag_mv; + +statement ok +INSERT INTO openivm_delta_uwd_bag_mv +SELECT *, CAST(-1 AS INTEGER), CURRENT_TIMESTAMP FROM openivm_old_uwd_bag_mv +UNION ALL +SELECT *, CAST(1 AS INTEGER), CURRENT_TIMESTAMP FROM openivm_new_uwd_bag_mv; + +statement ok +DROP TABLE IF EXISTS openivm_old_uwd_bag_mv; + +statement ok +DROP TABLE IF EXISTS openivm_new_uwd_bag_mv; + +# Three identical ('a', 1, 5) rows must be retracted three times, and four +# identical ('a', 1, 9) rows must be inserted four times. +query III +SELECT grp, val, COUNT(*) FROM openivm_delta_uwd_bag_mv +WHERE openivm_multiplicity = -1 AND grp = 'a' +GROUP BY grp, val; +---- +a 1 3 + +query III +SELECT grp, val, COUNT(*) FROM openivm_delta_uwd_bag_mv +WHERE openivm_multiplicity = 1 AND grp = 'a' +GROUP BY grp, val; +---- +a 1 4 + +query I +WITH applied AS ( + ( + SELECT grp, val, total FROM uwd_bag_old + UNION ALL + SELECT grp, val, total FROM openivm_delta_uwd_bag_mv WHERE openivm_multiplicity = 1 + ) + EXCEPT ALL + SELECT grp, val, total FROM openivm_delta_uwd_bag_mv WHERE openivm_multiplicity = -1 +) +SELECT COUNT(*) FROM ( + SELECT grp, val, total FROM applied + EXCEPT ALL + SELECT grp, val, total FROM openivm_data_uwd_bag_mv +); +---- +0 + +query I +WITH applied AS ( + ( + SELECT grp, val, total FROM uwd_bag_old + UNION ALL + SELECT grp, val, total FROM openivm_delta_uwd_bag_mv WHERE openivm_multiplicity = 1 + ) + EXCEPT ALL + SELECT grp, val, total FROM openivm_delta_uwd_bag_mv WHERE openivm_multiplicity = -1 +) +SELECT COUNT(*) FROM ( + SELECT grp, val, total FROM openivm_data_uwd_bag_mv + EXCEPT ALL + SELECT grp, val, total FROM applied +); +---- +0 + +# ========================================================================== +# Requesting a cascade delta must not relabel or rescue an unsupported plan. +# A window view whose predicate is non-deterministic (the same guard that +# demotes production predicates built on current_date()) stays FULL_REFRESH, +# and an unknown view still fails explicitly. +# ========================================================================== + +statement ok +CREATE TABLE uwd_vol_src (id INT, event_date DATE, val INT); + +statement ok +INSERT INTO uwd_vol_src VALUES (1, DATE '2024-01-01', 10); + +statement ok +CREATE MATERIALIZED VIEW uwd_vol_mv AS + SELECT CAST(ROW_NUMBER() OVER (ORDER BY id) AS INT) AS k, id, event_date, val + FROM uwd_vol_src + WHERE val > random(); + +query IT +SELECT refresh_type, refresh_type_name +FROM openivm_compile_with_facts( + 'uwd_vol_mv', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +) +LIMIT 1; +---- +3 FULL_REFRESH + +statement error +SELECT * FROM openivm_compile_with_facts( + 'uwd_does_not_exist', + '{"target_dialect":"duckdb","compile_only":true,"force_view_delta_cascade":true}' +); +---- +materialized view 'uwd_does_not_exist' not found From 2845bc4379172e0ad0377f3fb9126fa2837c7d7b Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sun, 23 Aug 2026 23:47:39 +0000 Subject: [PATCH 10/18] build: bump third_party/lpts to b3baf0b Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- third_party/lpts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/lpts b/third_party/lpts index 754c7977..b3baf0bb 160000 --- a/third_party/lpts +++ b/third_party/lpts @@ -1 +1 @@ -Subproject commit 754c797781c9b56429e05c1781cea5ca99c628e8 +Subproject commit b3baf0bbd974fc08161f1b7d9a540d60a9df96b8 From e50ffe683816f81ceef1b69c8ffa3d72cdd0783f Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Tue, 25 Aug 2026 07:42:04 +0000 Subject: [PATCH 11/18] build: bump third_party/lpts to dbac36d Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- third_party/lpts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/lpts b/third_party/lpts index b3baf0bb..dbac36de 160000 --- a/third_party/lpts +++ b/third_party/lpts @@ -1 +1 @@ -Subproject commit b3baf0bbd974fc08161f1b7d9a540d60a9df96b8 +Subproject commit dbac36de2e7e00d22fcb48250662fd69c0c106cd From 5d71fd6c185f1e7eaeff60d817a321a82b89cafc Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Tue, 25 Aug 2026 07:43:26 +0000 Subject: [PATCH 12/18] feat: preserve Spark time-travel pins through MV compilation Compiling a materialized view whose body pins a Delta relation (`FROM billing_meter_dim VERSION AS OF 366`) previously failed with `Parser Error: syntax error at or near "as"`, so OpenIvmCompiler fell back to COMPILE_FAILED/FULL_REFRESH and silently lost the pinned snapshot. Three gaps are closed. Input parsing. A new `openivm_input_dialect` setting records the dialect the caller writes MV bodies in. `ParseMaterializedViewStatement` routes the body through LPTS `NormalizeInputSqlToDuckDB(query, dialect)` before `Parser::ParseQuery`, so `VERSION AS OF n` becomes DuckDB's `AT (VERSION => n)` instead of being rejected. `SqlDialect::DUCKDB` (the default) keeps the previous code path byte for byte. The dialect is mirrored onto a `MaterializedViewParserExtensionInfo` because the parser extension entry points receive no `ClientContext`. Binding. OpenIvmCompiler registers fact stand-ins as plain in-memory tables, which honestly reject a pinned scan with `Catalog type does not support time travel`. `TimeTravelPins` peels the `AT (...)` qualifier off each `BaseTableRef` whose catalog does not implement time travel, binds against the stand-in, and restores the qualifier onto the matching `AstGetNode::table_name` after `LogicalPlanToAst`. Peeling is driven by `Catalog::SupportsTimeTravel()` rather than by dialect, so DuckLake pins still bind natively and are never touched. Pins are keyed by qualified relation name and tracked through aliases, backticks, repeated scans, joins, CTEs (with proper CTE-name shadowing) and several differently pinned relations in one body. Genuinely ambiguous bodies -- two pins on one relation, one pin naming two relations, a relation read both pinned and unpinned -- raise rather than guess. No path reads latest silently and no qualifier is stripped globally. Output. Rendering a stored view for a foreign dialect re-attaches the pins, so Spark full-recompute and delta SQL emit `VERSION AS OF n` on the correct relation. DuckDB output stays pin-free (the stand-in catalog has no snapshots). Every other dialect fails loudly through LPTS with `LPTS_UNSUPPORTED_TIME_TRAVEL`. Alias association is verified rather than trusted. Spark writes the pin between a relation and its alias (`FROM t VERSION AS OF 2 p`) where DuckDB wants it after both (`FROM t AS p AT (VERSION => 2)`), so normalizing has to carry the alias across the rewrite -- the one step where a pin could land on a neighbouring relation, attach to the wrong alias, or be dropped, each of which reads a different snapshot while still compiling cleanly. `CollectSourceSnapshotBindings` reads every `{relation, alias, qualifier}` triple off the source text before normalization and `VerifySnapshotBindings` re-checks them against the parse tree DuckDB produced, raising rather than compiling on. It is deliberately an independent derivation of the same facts, so a regression in the normalizer fails the compile instead of passing quietly. `test/sql/time_travel.test` covers input parsing and dialect validation, binding and initial load, version and timestamp pins, relation/pin and alias/pin association (bare and `AS` aliases, a column-alias list, a deliberate alias/relation name collision, two bare aliases on differently pinned relations), the ambiguity refusals, the emitted SQL for spark/duckdb/postgres, and `EXCEPT ALL` cross-checks of incremental against full recompute. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CMakeLists.txt | 1 + README.md | 1 + docs/internals/parser.md | 32 ++ src/core/parser.cpp | 29 +- src/core/parser_parse.cpp | 54 +- src/core/time_travel_pins.cpp | 651 +++++++++++++++++++++++++ src/include/core/parser.hpp | 31 ++ src/include/core/time_travel_pins.hpp | 96 ++++ src/openivm_extension.cpp | 5 + src/rules/incremental_rewrite_rule.cpp | 2 + src/upsert/refresh_cost_model.cpp | 2 + src/upsert/refresh_sql.cpp | 53 +- test/sql/time_travel.test | 446 +++++++++++++++++ 13 files changed, 1389 insertions(+), 14 deletions(-) create mode 100644 src/core/time_travel_pins.cpp create mode 100644 src/include/core/time_travel_pins.hpp create mode 100644 test/sql/time_travel.test diff --git a/CMakeLists.txt b/CMakeLists.txt index 10e20533..58d54410 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,6 +22,7 @@ set(EXTENSION_SOURCES src/core/parser_plan_helpers.cpp src/core/parser_sql_extractors.cpp src/core/sql_utils.cpp + src/core/time_travel_pins.cpp src/core/refresh_metadata.cpp src/core/incremental_checker.cpp src/core/plan_rewrite.cpp diff --git a/README.md b/README.md index 694560ef..89115ca9 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,7 @@ MVs can be created using any SQL construct. Unsupported operators automatically | `openivm_regular_nterm` | BOOLEAN | `true` | Use N-term telescoping for eligible regular-table inner joins compiled for external engines | [Inner join](docs/operators/inner-join.md#regular-table-n-term-compilation) | | `openivm_profile_refresh` | BOOLEAN | `false` | Record per-step refresh timings in `openivm_refresh_profile` | [Automatic refresh](docs/refresh/automatic-refresh.md) | | `openivm_files_path` | VARCHAR | — | Directory for compiled SQL reference files | [Internals](docs/internals/delta-tables.md) | +| `openivm_input_dialect` | VARCHAR | `duckdb` | Dialect of incoming `CREATE MATERIALIZED VIEW` bodies (e.g. `spark` for `VERSION AS OF`) | [Parser](docs/internals/parser.md#input-dialect) | ## Pragmas diff --git a/docs/internals/parser.md b/docs/internals/parser.md index 271f67c5..eb67517b 100644 --- a/docs/internals/parser.md +++ b/docs/internals/parser.md @@ -43,6 +43,38 @@ CREATE MATERIALIZED VIEW mv REFRESH EVERY '5 minutes' AS The parsed interval (300 seconds) is stored in the `refresh_interval` column of `openivm_views`. When omitted, `refresh_interval` is `NULL` (manual refresh only). See [Automatic Refresh](../refresh/automatic-refresh.md) for how the daemon uses this. +## Input dialect + +`openivm_input_dialect` (default `duckdb`) declares the dialect the *incoming* `CREATE MATERIALIZED VIEW` body is written in. When it is not `duckdb`, the body is run through LPTS' `NormalizeInputSqlToDuckDB` before `Parser::ParseQuery`, so backtick identifiers, dialect casts, interval literals and time-travel clauses are translated into DuckDB syntax first. The setting is mirrored onto the parser extension's `ParserExtensionInfo` because `parser_override` is not given a `ClientContext`. + +```sql +SET openivm_input_dialect='spark'; +CREATE MATERIALIZED VIEW mv AS + SELECT region, SUM(amount) FROM sales VERSION AS OF 366 GROUP BY region; +``` + +## Time-travel pins + +A Spark/Delta pin (`VERSION AS OF n`, `TIMESTAMP AS OF '...'`) normalizes to DuckDB's `AT (VERSION => n)` / `AT (TIMESTAMP => '...')` qualifier. OpenIVM registers the sources of a compiled view as plain in-memory stand-in tables whose catalog reports `SupportsTimeTravel() == false`, so a pinned scan cannot be bound directly. + +`src/core/time_travel_pins.cpp` handles this by *peeling* the qualifier off each `BaseTableRef` whose catalog cannot honour it, planning against the pin-less stand-in, and *restoring* the qualifier onto the matching `AstGetNode::table_name` after `LogicalPlanToAst`. Pins are keyed by relation, so aliases, repeated scans, CTEs and joins all keep their own pin, and two relations pinned to different snapshots stay distinct. A catalog that does support time travel (DuckLake) is never peeled and binds natively. + +Because pin restoration is keyed by relation, a view that pins the same relation ambiguously is refused with `NotImplementedException` rather than compiled with a guessed snapshot: + +- the same relation pinned to two different snapshots, +- the same pin naming two differently qualified relations, +- the same relation scanned both pinned and unpinned. + +The first is a real limitation rather than a policy choice. DuckDB resolves the `AT (...)` qualifier during *catalog lookup* — it selects which snapshot of the catalog entry to bind — so neither `LogicalGet` nor `AstGetNode` carries a per-scan pin. Two scans of one relation at two versions are indistinguishable in the bound plan except by `table_index`, and pairing those back to parse-tree references would rest on the binder's index-allocation order, whose failure mode is silently reading the wrong snapshot. Repeated scans that share a pin, including across CTEs, are supported normally. + +The pin is stored in the view SQL in `openivm_views.sql_string` so it survives a restart. Every site that binds or locally executes that SQL peels it first, and refresh source qualification drops it, since the local stand-in catalog holds no snapshots. Foreign-dialect output re-attaches it: Spark renders `VERSION AS OF n`, DuckDB keeps `AT (...)`, and any dialect LPTS has no verified time-travel syntax for raises `LPTS_UNSUPPORTED_TIME_TRAVEL` instead of silently reading the latest snapshot. + +### Alias association + +Spark writes the pin *between* a relation and its alias (`FROM t VERSION AS OF n p`) where DuckDB wants it after both (`FROM t AS p AT (VERSION => n)`), so normalization has to carry the alias across the rewrite. That is the one step where a pin could land on a neighbouring relation, attach to the wrong alias, or be dropped outright — each of which reads a different snapshot while still compiling cleanly. + +The association is therefore checked rather than trusted. `CollectSourceSnapshotBindings` reads every `{relation, alias, qualifier}` triple straight off the *source* text before normalization, and `VerifySnapshotBindings` re-checks them against the parse tree DuckDB produced, raising `NotImplementedException` on any pin that did not come through on the same relation and alias. + ## IVM compatibility classification After rewriting, the parser plans the query and walks the logical plan to classify the view into a refresh type: diff --git a/src/core/parser.cpp b/src/core/parser.cpp index a12db386..41ba64ef 100644 --- a/src/core/parser.cpp +++ b/src/core/parser.cpp @@ -13,6 +13,7 @@ #include "core/ivm_view_classifier.hpp" #include "lpts_pipeline.hpp" #include "core/sql_utils.hpp" +#include "core/time_travel_pins.hpp" #include "rules/column_hider.hpp" #include "upsert/refresh_compiler.hpp" #include "duckdb/common/printer.hpp" @@ -405,6 +406,13 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC // re-derives what it needs from the plan. unordered_set table_names; auto table_names_start = create_profile_now(); + // Peel time-travel pins the catalog cannot bind before anything plans this statement, keeping + // each pin keyed by its relation so it can be re-attached to the generated SQL below. + auto time_travel_pins = openivm::TimeTravelPins::Peel(context, *statement); + // SQL OpenIVM binds or executes itself runs against the very catalog that cannot honour the pin, + // so those copies drop it. The stored view SQL keeps it, and refresh re-attaches it when + // rendering for a foreign dialect. + auto local_view_query = time_travel_pins.StripFrom(original_view_query); try { table_names = con.GetTableNames(statement->query); } catch (const std::exception &e) { @@ -438,6 +446,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC auto select_parse_plan_start = create_profile_now(); Parser select_parser; select_parser.ParseQuery(original_view_query); + openivm::TimeTravelPins::PeelForLocalBinding(context, *select_parser.statements[0]); Planner select_planner(context); select_planner.CreatePlan(std::move(select_parser.statements[0])); auto select_plan = std::move(select_planner.plan); @@ -539,6 +548,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC // Refresh-time target dialects are selected per CompileFacts. SqlDialect dialect = SqlDialect::DUCKDB; auto ast = LogicalPlanToAst(context, select_plan, dialect); + time_travel_pins.RestoreInto(*ast); auto cte_list = AstToCteList(*ast, dialect); view_query = cte_list->ToQuery(true, output_names); if (!view_query.empty() && view_query.back() == ';') { @@ -663,7 +673,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC FilteredGroupCountExtract filtered_group_count_extract; FilteredGroupCountAuxRequirement filtered_group_count_aux_candidate; if (analysis.found_nested_aggregate && - ExtractFilteredGroupCount(original_view_query, output_names, filtered_group_count_extract)) { + ExtractFilteredGroupCount(local_view_query, output_names, filtered_group_count_extract)) { string aux_table = "openivm_filtered_group_count_" + view_name; string group_q = KeywordHelper::WriteOptionallyQuoted(filtered_group_count_extract.group_col); string sum_q = KeywordHelper::WriteOptionallyQuoted(filtered_group_count_extract.sum_col); @@ -677,7 +687,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC } if (analysis.found_semi_anti_join && !analysis.found_aggregation) { - if (ExtractSemiAntiQuery(original_view_query, semi_anti_extract)) { + if (ExtractSemiAntiQuery(local_view_query, semi_anti_extract)) { string left_table_name = SqlUtils::LastIdentifierPart(semi_anti_extract.left_table); auto col_result = con.Query("SELECT column_name FROM information_schema.columns WHERE " "lower(table_name) = lower('" + @@ -803,7 +813,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC if (aux_enabled && single_source) { vector dcols; string d_input_sql, d_source, d_filter; - if (!ExtractInnerDistinct(original_view_query, dcols, d_input_sql, d_source, d_filter)) { + if (!ExtractInnerDistinct(local_view_query, dcols, d_input_sql, d_source, d_filter)) { OPENIVM_DEBUG_PRINT("[CREATE MV] DISTINCT_INCREMENTAL extractor failed — demoting to " "GROUP_RECOMPUTE\n"); } else { @@ -891,7 +901,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC candidate_group_columns.push_back(output_names[i]); } CountDistinctExtract cd_extract; - if (ExtractCountDistinctAggregate(original_view_query, candidate_group_columns, output_names, cd_extract)) { + if (ExtractCountDistinctAggregate(local_view_query, candidate_group_columns, output_names, cd_extract)) { count_distinct_aux_candidate = { "openivm_aux_" + view_name, SqlUtils::LastIdentifierPart(cd_extract.source), candidate_group_columns, cd_extract.group_exprs, @@ -1353,7 +1363,8 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC if (!current_catalog.empty() && current_catalog != default_db) { con.Query("USE " + current_catalog_schema); } - string initial_load_statement = "CREATE TABLE " + initial_load_target + " AS " + view_query; + string local_initial_load_query = time_travel_pins.StripFrom(view_query); + string initial_load_statement = "CREATE TABLE " + initial_load_target + " AS " + local_initial_load_query; string diagnostic; diagnostic += "\n[OpenIVM initial-load diagnostic]\n"; diagnostic += "view_name: " + view_name + "\n"; @@ -1363,8 +1374,8 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC diagnostic += "initial_load_statement:\n" + initial_load_statement + "\n\n"; diagnostic += "original_view_query:\n" + original_view_query + "\n\n"; diagnostic += "generated_view_query:\n" + view_query + "\n\n"; - diagnostic += ExplainInitialLoadQuery(con, "EXPLAIN original_view_query:", original_view_query); - diagnostic += ExplainInitialLoadQuery(con, "EXPLAIN generated_view_query:", view_query); + diagnostic += ExplainInitialLoadQuery(con, "EXPLAIN original_view_query:", local_view_query); + diagnostic += ExplainInitialLoadQuery(con, "EXPLAIN generated_view_query:", local_initial_load_query); diagnostic += ExplainInitialLoadQuery(con, "EXPLAIN initial_load_statement:", initial_load_statement); Printer::Print(diagnostic); @@ -1397,7 +1408,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC ddl.push_back(BuildSemiAntiInitialDataSQL(initial_load_target, aux_target, meta.join_type, meta.left_cols, meta.output_cols, meta.null_aware, meta.null_aware_left_col)); } else { - ddl.push_back("create table " + initial_load_target + " as " + view_query); + ddl.push_back("create table " + initial_load_target + " as " + time_travel_pins.StripFrom(view_query)); } if (staged_cross_catalog_replace) { // DuckDB cannot make the DuckLake objects and native metadata atomic @@ -1639,7 +1650,7 @@ MaterializedViewParserExtension::PlanFunction(ParserExtensionInfo *info, ClientC string MaterializedViewLifecycleQuery(ClientContext &context, const FunctionParameters ¶meters) { auto query = StringValue::Get(parameters.values[0]); - auto parse_result = MaterializedViewParserExtension::ParseFunction(nullptr, query); + auto parse_result = ParseMaterializedViewStatement(query, OpenIvmInputDialect(context)); if (parse_result.type != ParserExtensionResultType::PARSE_SUCCESSFUL) { throw ParserException("OpenIVM could not parse the materialized-view lifecycle statement"); } diff --git a/src/core/parser_parse.cpp b/src/core/parser_parse.cpp index 0efec8fc..74dd77fc 100644 --- a/src/core/parser_parse.cpp +++ b/src/core/parser_parse.cpp @@ -3,11 +3,15 @@ #include "core/openivm_constants.hpp" #include "core/openivm_debug.hpp" #include "core/sql_utils.hpp" +#include "core/time_travel_pins.hpp" +#include "duckdb/main/config.hpp" +#include "duckdb/main/extension_callback_manager.hpp" #include "duckdb/parser/expression/constant_expression.hpp" #include "duckdb/parser/parser.hpp" #include "duckdb/parser/qualified_name.hpp" #include "duckdb/parser/statement/drop_statement.hpp" #include "duckdb/parser/statement/pragma_statement.hpp" +#include "lpts_parser.hpp" #include @@ -20,10 +24,37 @@ static unique_ptr BuildInternalPragma(const string &name, const st return std::move(statement); } +SqlDialect OpenIvmInputDialect(ClientContext &context) { + Value setting_value; + if (!context.TryGetCurrentSetting(OPENIVM_INPUT_DIALECT_SETTING, setting_value) || setting_value.IsNull()) { + return SqlDialect::DUCKDB; + } + return ParseSqlDialectSetting(setting_value.ToString(), OPENIVM_INPUT_DIALECT_SETTING); +} + +void SetOpenIvmInputDialect(ClientContext &context, SetScope scope, Value ¶meter) { + auto dialect = parameter.IsNull() ? SqlDialect::DUCKDB + : ParseSqlDialectSetting(parameter.ToString(), OPENIVM_INPUT_DIALECT_SETTING); + // `parser_override` and `parse_function` are handed the extension info, not a ClientContext, so + // mirror the resolved dialect there. The info is owned by the DBConfig, so the mirror stays + // scoped to this database. + for (auto &extension : ExtensionCallbackManager::Get(context).ParserExtensions()) { + auto info = dynamic_cast(extension.parser_info.get()); + if (info) { + info->SetInputDialect(dialect); + } + } +} + +static SqlDialect InputDialectFromInfo(ParserExtensionInfo *info) { + auto materialized_view_info = dynamic_cast(info); + return materialized_view_info ? materialized_view_info->InputDialect() : SqlDialect::DUCKDB; +} + ParserOverrideResult MaterializedViewParserExtension::OverrideFunction(ParserExtensionInfo *info, const string &query, ParserOptions &options) { try { - auto extension_result = ParseFunction(info, query); + auto extension_result = ParseMaterializedViewStatement(query, InputDialectFromInfo(info)); if (extension_result.type == ParserExtensionResultType::PARSE_SUCCESSFUL) { vector> statements; statements.push_back(BuildInternalPragma("openivm_materialized_view_lifecycle", query)); @@ -55,6 +86,10 @@ ParserOverrideResult MaterializedViewParserExtension::OverrideFunction(ParserExt ParserExtensionParseResult MaterializedViewParserExtension::ParseFunction(ParserExtensionInfo *info, const string &query) { + return ParseMaterializedViewStatement(query, InputDialectFromInfo(info)); +} + +ParserExtensionParseResult ParseMaterializedViewStatement(const string &query, SqlDialect input_dialect) { auto query_lower = SqlUtils::SQLToLowercase(StringUtil::Replace(query, ";", "")); StringUtil::Trim(query_lower); // Strip SQL line comments (-- to end of line) before whitespace normalization. @@ -126,8 +161,25 @@ ParserExtensionParseResult MaterializedViewParserExtension::ParseFunction(Parser // at the plan level in PlanFunction via PlanRewrite + LPTS. OPENIVM_DEBUG_PRINT("[CREATE MV] After structural rewrite: %s\n", query_lower.c_str()); + vector pin_bindings; + if (input_dialect != SqlDialect::DUCKDB) { + // The body is written in another dialect, so DuckDB's parser cannot read it as-is — a + // Spark/Delta temporal clause (`FROM t VERSION AS OF 366`) dies on `AS`. LPTS rewrites the + // source spelling into the semantically equivalent DuckDB one, keeping the pin + // (`AT (VERSION => 366)`) rather than dropping it, which would silently promote the scan to + // "read latest". Record what each pin is written against first, so the rewrite can be held + // to it below. + pin_bindings = openivm::CollectSourceSnapshotBindings(query_lower, input_dialect); + query_lower = NormalizeInputSqlToDuckDB(query_lower, input_dialect); + OPENIVM_DEBUG_PRINT("[CREATE MV] After %s input normalization: %s\n", SqlDialectToString(input_dialect).c_str(), + query_lower.c_str()); + } + Parser p; p.ParseQuery(query_lower); + // Rewriting the temporal clause moves it across the relation's alias, so re-check against the + // parse tree that every pin still names the relation and alias it was written against. + openivm::VerifySnapshotBindings(*p.statements[0], pin_bindings); auto parse_data = make_uniq_base(std::move(p.statements[0]), refresh_interval); diff --git a/src/core/time_travel_pins.cpp b/src/core/time_travel_pins.cpp new file mode 100644 index 00000000..d0d6f3a3 --- /dev/null +++ b/src/core/time_travel_pins.cpp @@ -0,0 +1,651 @@ +#include "core/time_travel_pins.hpp" + +#include "core/openivm_debug.hpp" +#include "duckdb/catalog/catalog.hpp" +#include "duckdb/catalog/catalog_entry/table_catalog_entry.hpp" +#include "duckdb/parser/expression/subquery_expression.hpp" +#include "duckdb/parser/parsed_expression_iterator.hpp" +#include "duckdb/parser/query_node/cte_node.hpp" +#include "duckdb/parser/query_node/recursive_cte_node.hpp" +#include "duckdb/parser/query_node/select_node.hpp" +#include "duckdb/parser/query_node/set_operation_node.hpp" +#include "duckdb/parser/statement/create_statement.hpp" +#include "duckdb/parser/statement/insert_statement.hpp" +#include "duckdb/parser/statement/select_statement.hpp" +#include "duckdb/parser/tableref/basetableref.hpp" +#include "duckdb/parser/tableref/joinref.hpp" +#include "duckdb/parser/tableref/pivotref.hpp" +#include "duckdb/parser/tableref/subqueryref.hpp" +#include "duckdb/parser/tableref/table_function_ref.hpp" +#include "duckdb/parser/parsed_data/create_table_info.hpp" +#include "duckdb/parser/parser.hpp" +#include "lpts_sql_scanner.hpp" + +#include +#include + +namespace duckdb { +namespace openivm { + +using BaseTableRefCallback = std::function; + +// A `BaseTableRef` whose unqualified name matches a CTE visible at that point is a reference to +// the CTE, not a scan of a same-named relation, so it must never be treated as an unpinned scan. +struct RefVisitor { + const BaseTableRefCallback &callback; + case_insensitive_set_t cte_names; +}; + +static void VisitQueryNode(QueryNode &node, const RefVisitor &visitor); + +static void VisitExpression(ParsedExpression &expression, const RefVisitor &visitor) { + if (expression.GetExpressionClass() == ExpressionClass::SUBQUERY) { + auto &subquery = expression.Cast(); + if (subquery.subquery && subquery.subquery->node) { + VisitQueryNode(*subquery.subquery->node, visitor); + } + } + ParsedExpressionIterator::EnumerateChildren(expression, + [&](ParsedExpression &child) { VisitExpression(child, visitor); }); +} + +static void VisitTableRef(TableRef &ref, const RefVisitor &visitor) { + switch (ref.type) { + case TableReferenceType::BASE_TABLE: { + auto &base_table = ref.Cast(); + bool unqualified = base_table.catalog_name.empty() && base_table.schema_name.empty(); + if (unqualified && visitor.cte_names.find(base_table.table_name) != visitor.cte_names.end()) { + break; + } + visitor.callback(base_table); + break; + } + case TableReferenceType::JOIN: { + auto &join = ref.Cast(); + if (join.left) { + VisitTableRef(*join.left, visitor); + } + if (join.right) { + VisitTableRef(*join.right, visitor); + } + if (join.condition) { + VisitExpression(*join.condition, visitor); + } + break; + } + case TableReferenceType::SUBQUERY: { + auto &subquery = ref.Cast(); + if (subquery.subquery && subquery.subquery->node) { + VisitQueryNode(*subquery.subquery->node, visitor); + } + break; + } + case TableReferenceType::PIVOT: { + auto &pivot = ref.Cast(); + if (pivot.source) { + VisitTableRef(*pivot.source, visitor); + } + break; + } + case TableReferenceType::TABLE_FUNCTION: { + auto &table_function = ref.Cast(); + if (table_function.function) { + VisitExpression(*table_function.function, visitor); + } + break; + } + default: + break; + } +} + +static void VisitQueryNode(QueryNode &node, const RefVisitor &visitor) { + // A CTE body is bound in the scope that precedes its own name, so walk the bodies first with + // the incoming scope and only then extend the scope for the node that references them. + for (auto &cte : node.cte_map.map) { + if (cte.second->query && cte.second->query->node) { + VisitQueryNode(*cte.second->query->node, visitor); + } + } + RefVisitor scoped {visitor.callback, visitor.cte_names}; + for (auto &cte : node.cte_map.map) { + scoped.cte_names.insert(cte.first); + } + switch (node.type) { + case QueryNodeType::SELECT_NODE: { + auto &select = node.Cast(); + if (select.from_table) { + VisitTableRef(*select.from_table, scoped); + } + auto visit = [&](unique_ptr &expression) { + if (expression) { + VisitExpression(*expression, scoped); + } + }; + for (auto &expression : select.select_list) { + visit(expression); + } + for (auto &expression : select.groups.group_expressions) { + visit(expression); + } + visit(select.where_clause); + visit(select.having); + visit(select.qualify); + break; + } + case QueryNodeType::SET_OPERATION_NODE: { + auto &set_operation = node.Cast(); + for (auto &child : set_operation.children) { + if (child) { + VisitQueryNode(*child, scoped); + } + } + break; + } + case QueryNodeType::RECURSIVE_CTE_NODE: { + auto &recursive_cte = node.Cast(); + RefVisitor recursive_scope {visitor.callback, scoped.cte_names}; + recursive_scope.cte_names.insert(recursive_cte.ctename); + if (recursive_cte.left) { + VisitQueryNode(*recursive_cte.left, recursive_scope); + } + if (recursive_cte.right) { + VisitQueryNode(*recursive_cte.right, recursive_scope); + } + break; + } + case QueryNodeType::CTE_NODE: { + auto &cte = node.Cast(); + if (cte.query) { + VisitQueryNode(*cte.query, scoped); + } + if (cte.child) { + RefVisitor child_scope {visitor.callback, scoped.cte_names}; + child_scope.cte_names.insert(cte.ctename); + VisitQueryNode(*cte.child, child_scope); + } + break; + } + default: + break; + } + ParsedExpressionIterator::EnumerateQueryNodeModifiers(node, [&](unique_ptr &expression) { + if (expression) { + VisitExpression(*expression, scoped); + } + }); +} + +static void VisitStatement(SQLStatement &statement, const RefVisitor &visitor) { + switch (statement.type) { + case StatementType::SELECT_STATEMENT: { + auto &select = statement.Cast(); + if (select.node) { + VisitQueryNode(*select.node, visitor); + } + break; + } + case StatementType::CREATE_STATEMENT: { + auto &create = statement.Cast(); + if (create.info && create.info->type == CatalogType::TABLE_ENTRY) { + auto &table_info = create.info->Cast(); + if (table_info.query && table_info.query->node) { + VisitQueryNode(*table_info.query->node, visitor); + } + } + break; + } + case StatementType::INSERT_STATEMENT: { + auto &insert = statement.Cast(); + if (insert.select_statement && insert.select_statement->node) { + VisitQueryNode(*insert.select_statement->node, visitor); + } + break; + } + default: + break; + } +} + +// Whether the catalog backing `ref` implements time travel, so its pin binds natively and must be +// left in place. An unresolvable relation is left alone as well: DuckDB owns that error message. +static bool CatalogHonoursPin(ClientContext &context, BaseTableRef &ref) { + QueryErrorContext error_context; + EntryLookupInfo lookup(CatalogType::TABLE_ENTRY, ref.table_name, error_context); + optional_ptr entry; + try { + entry = Catalog::GetEntry(context, ref.catalog_name, ref.schema_name, lookup, OnEntryNotFound::RETURN_NULL); + } catch (const std::exception &) { + return true; + } + if (!entry) { + return true; + } + return entry->ParentCatalog().SupportsTimeTravel(); +} + +[[noreturn]] static void ThrowAmbiguousPin(const string &table_name, const string &reason) { + throw NotImplementedException( + "OpenIVM cannot compile a materialized view that pins relation '%s' ambiguously: %s. DuckDB resolves a " + "time-travel qualifier during catalog lookup, so the bound plan keeps no per-scan record of it and " + "re-attaching one can only be keyed by relation — conflating scans that must read different snapshots is " + "not something OpenIVM will do silently. Give every scan of the relation the same pin, or split them into " + "separate views.", + table_name, reason); +} + +TimeTravelPins TimeTravelPins::Peel(ClientContext &context, SQLStatement &statement) { + TimeTravelPins result; + case_insensitive_set_t unpinned; + BaseTableRefCallback callback = [&](BaseTableRef &ref) { + if (CatalogHonoursPin(context, ref)) { + return; + } + if (!ref.at_clause) { + unpinned.insert(ref.table_name); + return; + } + Pin pin; + // INVALID_CATALOG / INVALID_SCHEMA are the empty string, so an unqualified reference already + // stores the "" this map treats as "matches any qualifier". + pin.catalog = ref.catalog_name; + pin.schema = ref.schema_name; + pin.suffix = " " + ref.at_clause->ToString(); + auto existing = result.pins.find(ref.table_name); + if (existing != result.pins.end()) { + if (existing->second.suffix != pin.suffix) { + ThrowAmbiguousPin(ref.table_name, "it is pinned to both '" + existing->second.suffix.substr(1) + + "' and '" + pin.suffix.substr(1) + "'"); + } + if (existing->second.catalog != pin.catalog || existing->second.schema != pin.schema) { + ThrowAmbiguousPin(ref.table_name, "the same pin names two differently qualified relations"); + } + } + OPENIVM_DEBUG_PRINT("[TIME TRAVEL] Peeled pin '%s' off relation '%s'\n", pin.suffix.c_str(), + ref.table_name.c_str()); + ref.at_clause.reset(); + result.pins[ref.table_name] = std::move(pin); + }; + RefVisitor visitor {callback, case_insensitive_set_t()}; + VisitStatement(statement, visitor); + for (auto &entry : result.pins) { + if (unpinned.find(entry.first) != unpinned.end()) { + ThrowAmbiguousPin(entry.first, "it is scanned both pinned and unpinned"); + } + } + return result; +} + +void TimeTravelPins::PeelForLocalBinding(ClientContext &context, SQLStatement &statement) { + Peel(context, statement); +} + +TimeTravelPins TimeTravelPins::FromViewSql(ClientContext &context, const string &view_query_sql) { + Parser parser(context.GetParserOptions()); + parser.ParseQuery(view_query_sql); + if (parser.statements.empty()) { + return TimeTravelPins(); + } + return Peel(context, *parser.statements[0]); +} + +void TimeTravelPins::RestoreInto(AstNode &ast) const { + if (pins.empty()) { + return; + } + auto get_node = dynamic_cast(&ast); + if (get_node && get_node->table_name.find(" AT (") == string::npos) { + auto entry = pins.find(get_node->table_name); + if (entry != pins.end()) { + auto &pin = entry->second; + bool catalog_matches = pin.catalog.empty() || get_node->catalog.empty() || + StringUtil::CIEquals(pin.catalog, get_node->catalog); + bool schema_matches = + pin.schema.empty() || get_node->schema.empty() || StringUtil::CIEquals(pin.schema, get_node->schema); + if (catalog_matches && schema_matches) { + get_node->table_name += pin.suffix; + } + } + } + for (auto &child : ast.children) { + if (child) { + RestoreInto(*child); + } + } +} + +static bool IsIdentifierStart(char c) { + return std::isalpha(static_cast(c)) || c == '_'; +} + +static bool IsIdentifierPart(char c) { + return std::isalnum(static_cast(c)) || c == '_' || c == '$'; +} + +// Copy the quoted run starting at `sql[start]` (whose delimiter is `sql[start]`) into `result`, +// returning the index just past the closing delimiter. Doubled delimiters escape. +static idx_t CopyQuotedRun(const string &sql, idx_t start, string &result) { + char quote = sql[start]; + result += quote; + idx_t i = start + 1; + while (i < sql.size()) { + if (sql[i] == quote) { + if (i + 1 < sql.size() && sql[i + 1] == quote) { + result += quote; + result += quote; + i += 2; + continue; + } + result += quote; + return i + 1; + } + result += sql[i]; + i++; + } + return i; +} + +// Index just past the `)` matching the `(` at `sql[open]`, skipping quoted runs. +static idx_t MatchingParen(const string &sql, idx_t open) { + idx_t depth = 0; + for (idx_t i = open; i < sql.size(); i++) { + char c = sql[i]; + if (c == '\'' || c == '"' || c == '`') { + string ignored; + i = CopyQuotedRun(sql, i, ignored) - 1; + continue; + } + if (c == '(') { + depth++; + } else if (c == ')') { + depth--; + if (depth == 0) { + return i + 1; + } + } + } + return DConstants::INVALID_INDEX; +} + +string TimeTravelPins::StripFrom(const string &sql) const { + if (pins.empty()) { + return sql; + } + string result; + result.reserve(sql.size()); + string last_identifier; + idx_t i = 0; + while (i < sql.size()) { + char c = sql[i]; + if (c == '\'') { + i = CopyQuotedRun(sql, i, result); + last_identifier.clear(); + continue; + } + if (c == '"' || c == '`') { + idx_t start = result.size(); + i = CopyQuotedRun(sql, i, result); + last_identifier = result.substr(start + 1, result.size() - start - 2); + continue; + } + if (c == '-' && i + 1 < sql.size() && sql[i + 1] == '-') { + while (i < sql.size() && sql[i] != '\n') { + result += sql[i++]; + } + continue; + } + if (c == '/' && i + 1 < sql.size() && sql[i + 1] == '*') { + auto close = sql.find("*/", i + 2); + auto end = close == string::npos ? sql.size() : close + 2; + result.append(sql, i, end - i); + i = end; + continue; + } + if (IsIdentifierStart(c)) { + idx_t end = i; + while (end < sql.size() && IsIdentifierPart(sql[end])) { + end++; + } + auto token = sql.substr(i, end - i); + idx_t after = end; + while (after < sql.size() && std::isspace(static_cast(sql[after]))) { + after++; + } + if (StringUtil::CIEquals(token, "at") && after < sql.size() && sql[after] == '(' && + pins.find(last_identifier) != pins.end()) { + auto close = MatchingParen(sql, after); + if (close != DConstants::INVALID_INDEX) { + while (!result.empty() && std::isspace(static_cast(result.back()))) { + result.pop_back(); + } + i = close; + continue; + } + } + result += token; + last_identifier = token; + i = end; + continue; + } + result += c; + if (c != '.' && !std::isspace(static_cast(c))) { + last_identifier.clear(); + } + i++; + } + return result; +} + +// Collapse runs of whitespace so two spellings of the same qualifier compare equal. +static string NormalizeQualifierText(const string &qualifier) { + string normalized; + normalized.reserve(qualifier.size()); + bool pending_space = false; + for (auto c : qualifier) { + if (std::isspace(static_cast(c))) { + pending_space = !normalized.empty(); + continue; + } + if (pending_space) { + normalized += ' '; + pending_space = false; + } + normalized += c; + } + return normalized; +} + +// Words that may legally follow a table reference; none of them can be a bare alias. +static bool CanBeBareAlias(const string &token) { + // DuckDB's `alias_clause` takes a `ColId`: plain identifiers plus the unreserved and column-name + // keywords, but not the reserved or type/function ones. So `WHERE`, `JOIN` and `NATURAL` end the + // relation instead of naming it. + auto category = Parser::IsKeyword(StringUtil::Lower(token)); + return category != KeywordCategory::KEYWORD_RESERVED && category != KeywordCategory::KEYWORD_TYPE_FUNC; +} + +// Map a Spark/Delta temporal keyword at `pos` to the DuckDB `AT (...)` parameter carrying the same +// snapshot. `VERSION`/`SYSTEM_VERSION` pin a commit version, `TIMESTAMP`/`SYSTEM_TIME` a point in time. +static bool ReadTemporalKeyword(const string &sql, idx_t pos, idx_t &end, string &at_parameter) { + struct TemporalKeyword { + const char *spelling; + const char *at_parameter; + }; + static const TemporalKeyword KEYWORDS[] = {{"system_version", "VERSION"}, + {"version", "VERSION"}, + {"system_time", "TIMESTAMP"}, + {"timestamp", "TIMESTAMP"}}; + for (const auto &candidate : KEYWORDS) { + if (MatchesKeywordAt(sql, pos, candidate.spelling)) { + end = pos + strlen(candidate.spelling); + at_parameter = candidate.at_parameter; + return true; + } + } + return false; +} + +// Read a whole `[FOR] VERSION|TIMESTAMP AS OF ` clause at `pos`, yielding the equivalent +// DuckDB qualifier. Requiring the full sequence keeps a column merely named `version` from matching. +static bool TryReadSourcePin(const string &sql, idx_t pos, idx_t &end, string &qualifier) { + idx_t keyword_start = pos; + if (MatchesKeywordAt(sql, pos, "for")) { + keyword_start = SkipWhitespace(sql, pos + 3); + } + idx_t keyword_end; + string at_parameter; + if (!ReadTemporalKeyword(sql, keyword_start, keyword_end, at_parameter)) { + return false; + } + idx_t as_pos = SkipWhitespace(sql, keyword_end); + if (!MatchesKeywordAt(sql, as_pos, "as")) { + return false; + } + idx_t of_pos = SkipWhitespace(sql, as_pos + 2); + if (!MatchesKeywordAt(sql, of_pos, "of")) { + return false; + } + idx_t value_start = SkipWhitespace(sql, of_pos + 2); + idx_t value_end; + string literal; + string value_sql; + if (TryReadSingleQuotedLiteral(sql, value_start, value_end, literal)) { + value_sql = SingleQuotedSqlString(literal); + } else if (!TryReadNumericToken(sql, value_start, value_end, value_sql)) { + return false; + } + qualifier = "AT (" + at_parameter + " => " + value_sql + ")"; + end = value_end; + return true; +} + +// Read the `[AS] alias` the source dialect allows *after* a temporal clause, returning `pos` +// unchanged when the relation is unaliased and what follows just continues the query. +static idx_t ReadAliasAfterPin(const string &sql, idx_t pos, string &alias) { + idx_t cursor = SkipWhitespace(sql, pos); + bool explicit_as = MatchesKeywordAt(sql, cursor, "as"); + if (explicit_as) { + cursor = SkipWhitespace(sql, cursor + 2); + } + if (cursor < sql.size() && (sql[cursor] == '"' || sql[cursor] == '`')) { + string quoted; + idx_t quoted_end = CopyQuotedRun(sql, cursor, quoted); + alias = quoted.size() >= 2 ? quoted.substr(1, quoted.size() - 2) : quoted; + return quoted_end; + } + idx_t token_end; + string token; + if (TryReadIdentifierToken(sql, cursor, token_end, token) && (explicit_as || CanBeBareAlias(token))) { + alias = token; + return token_end; + } + return pos; +} + +vector CollectSourceSnapshotBindings(const string &sql, SqlDialect dialect) { + vector bindings; + if (dialect != SqlDialect::SPARK) { + return bindings; + } + // The relation the next temporal clause would pin. Reset by anything that cannot be part of a + // qualified relation name, so a pin is never credited to an unrelated identifier. + string last_identifier; + idx_t i = 0; + while (i < sql.size()) { + char c = sql[i]; + if (c == '\'') { + string ignored; + i = CopyQuotedRun(sql, i, ignored); + last_identifier.clear(); + continue; + } + if (c == '"' || c == '`') { + string quoted; + i = CopyQuotedRun(sql, i, quoted); + last_identifier = quoted.size() >= 2 ? quoted.substr(1, quoted.size() - 2) : quoted; + continue; + } + if (c == '-' && i + 1 < sql.size() && sql[i + 1] == '-') { + while (i < sql.size() && sql[i] != '\n') { + i++; + } + continue; + } + if (c == '/' && i + 1 < sql.size() && sql[i + 1] == '*') { + auto close = sql.find("*/", i + 2); + i = close == string::npos ? sql.size() : close + 2; + continue; + } + if (!IsIdentifierStart(c)) { + if (c != '.' && !std::isspace(static_cast(c))) { + last_identifier.clear(); + } + i++; + continue; + } + idx_t pin_end; + string qualifier; + if (!TryReadSourcePin(sql, i, pin_end, qualifier)) { + idx_t end = i; + while (end < sql.size() && IsIdentifierPart(sql[end])) { + end++; + } + last_identifier = sql.substr(i, end - i); + i = end; + continue; + } + string alias; + idx_t alias_end = ReadAliasAfterPin(sql, pin_end, alias); + // A keyword in front of the clause names no relation, so there is no association to assert; + // the normalized text will fail to parse on its own. + if (!last_identifier.empty() && CanBeBareAlias(last_identifier)) { + bindings.push_back(SnapshotBinding {last_identifier, alias, qualifier}); + OPENIVM_DEBUG_PRINT("[TIME TRAVEL] Source pin '%s' on relation '%s' aliased '%s'\n", qualifier.c_str(), + last_identifier.c_str(), alias.c_str()); + } + last_identifier.clear(); + i = alias_end; + } + return bindings; +} + +void VerifySnapshotBindings(SQLStatement &statement, const vector &bindings) { + if (bindings.empty()) { + return; + } + vector matched(bindings.size(), false); + BaseTableRefCallback callback = [&](BaseTableRef &ref) { + if (!ref.at_clause) { + return; + } + auto qualifier = ref.at_clause->ToString(); + for (idx_t i = 0; i < bindings.size(); i++) { + if (matched[i]) { + continue; + } + auto &binding = bindings[i]; + if (StringUtil::CIEquals(binding.relation, ref.table_name) && + StringUtil::CIEquals(binding.alias, ref.alias) && + StringUtil::CIEquals(NormalizeQualifierText(binding.qualifier), NormalizeQualifierText(qualifier))) { + matched[i] = true; + return; + } + } + }; + RefVisitor visitor {callback, case_insensitive_set_t()}; + VisitStatement(statement, visitor); + for (idx_t i = 0; i < bindings.size(); i++) { + if (!matched[i]) { + auto &binding = bindings[i]; + throw NotImplementedException( + "OpenIVM lost the time-travel pin '%s' written on relation '%s' (alias '%s') while normalizing the " + "view body: no scan of that relation carries it after parsing. The source dialect writes the pin " + "between the relation and its alias and DuckDB wants it after both, so the clause is reordered " + "before parsing; compiling on would read a different snapshot.", + binding.qualifier, binding.relation, binding.alias); + } + } +} + +} // namespace openivm +} // namespace duckdb diff --git a/src/include/core/parser.hpp b/src/include/core/parser.hpp index bebef42a..7fbe8b85 100644 --- a/src/include/core/parser.hpp +++ b/src/include/core/parser.hpp @@ -2,18 +2,40 @@ #define OPENIVM_PARSER_HPP #include "duckdb.hpp" +#include "duckdb/main/setting_info.hpp" #include "duckdb/parser/parser_extension.hpp" +#include "sql_dialect.hpp" +#include #include namespace duckdb { +//! Name of the setting that declares which SQL dialect the caller writes materialized-view bodies in. +constexpr const char *OPENIVM_INPUT_DIALECT_SETTING = "openivm_input_dialect"; + +//! Parser-extension state. `parse_function` and `parser_override` run before any ClientContext +//! exists, so the input dialect is mirrored here from the `openivm_input_dialect` setting: it +//! decides whether a materialized-view body must be normalized out of its source dialect before +//! DuckDB's parser ever sees it. +struct MaterializedViewParserExtensionInfo : ParserExtensionInfo { + std::atomic input_dialect {static_cast(SqlDialect::DUCKDB)}; + + SqlDialect InputDialect() const { + return static_cast(input_dialect.load()); + } + void SetInputDialect(SqlDialect dialect) { + input_dialect.store(static_cast(dialect)); + } +}; + class MaterializedViewParserExtension : public ParserExtension { public: explicit MaterializedViewParserExtension() { parse_function = ParseFunction; plan_function = PlanFunction; parser_override = OverrideFunction; + parser_info = make_shared_ptr(); } static ParserExtensionParseResult ParseFunction(ParserExtensionInfo *info, const string &query); @@ -23,6 +45,15 @@ class MaterializedViewParserExtension : public ParserExtension { unique_ptr parse_data); }; +//! Parse a materialized-view lifecycle statement written in `input_dialect`. +ParserExtensionParseResult ParseMaterializedViewStatement(const string &query, SqlDialect input_dialect); + +//! The dialect materialized-view bodies arrive in for this session (`openivm_input_dialect`). +SqlDialect OpenIvmInputDialect(ClientContext &context); + +//! `openivm_input_dialect` set callback: validates the value and mirrors it onto the parser extension. +void SetOpenIvmInputDialect(ClientContext &context, SetScope scope, Value ¶meter); + string MaterializedViewLifecycleQuery(ClientContext &context, const FunctionParameters ¶meters); string MaterializedViewDropQuery(ClientContext &context, const FunctionParameters ¶meters); diff --git a/src/include/core/time_travel_pins.hpp b/src/include/core/time_travel_pins.hpp new file mode 100644 index 00000000..95148be2 --- /dev/null +++ b/src/include/core/time_travel_pins.hpp @@ -0,0 +1,96 @@ +#ifndef OPENIVM_TIME_TRAVEL_PINS_HPP +#define OPENIVM_TIME_TRAVEL_PINS_HPP + +#pragma once + +#include "duckdb.hpp" +#include "duckdb/common/case_insensitive_map.hpp" +#include "lpts_ast.hpp" +#include "sql_dialect.hpp" + +namespace duckdb { + +class SQLStatement; + +namespace openivm { + +// A time-travel pin (`FROM t AT (VERSION => 366)`, the DuckDB spelling LPTS normalises Spark's +// `FROM t VERSION AS OF 366` into) is only bindable when the relation lives in a catalog that +// implements time travel. OpenIVM routinely binds against catalogs that do not: the Spark bridge +// registers schema-only stand-ins with plain in-memory `CREATE TABLE`, so binding a pinned scan +// fails with `Catalog type does not support time travel`. +// +// Dropping the pin to make binding succeed would silently turn a pinned scan into a read of the +// latest snapshot, so instead the pin is *peeled* off the parsed statement, kept here keyed by the +// relation it belongs to, and re-attached to the matching `AstGetNode` once the plan has been +// converted back to an AST. LPTS then renders it in the target dialect (`VERSION AS OF 366` for +// Spark) and refuses for dialects with no verified time-travel syntax. +// +// Pins on catalogs that *do* support time travel (DuckLake) are left in place: those bind natively +// and already round-trip through LPTS. +class TimeTravelPins { +public: + // Peel every unbindable pin out of `statement`, recording relation -> qualifier. Throws when a + // relation cannot be given one unambiguous pin (two different pins, or pinned in one scan and + // unpinned in another), because re-attaching by relation would conflate the two. + static TimeTravelPins Peel(ClientContext &context, SQLStatement &statement); + + // Peel the pins out of `statement` and discard them: the statement is only being bound or + // planned locally, where the pin cannot be honoured and is not rendered back out. + static void PeelForLocalBinding(ClientContext &context, SQLStatement &statement); + + // Peel the pins recorded by a stored materialized-view body without mutating anything. + static TimeTravelPins FromViewSql(ClientContext &context, const string &view_query_sql); + + bool Empty() const { + return pins.empty(); + } + + // Re-attach every recorded qualifier to the `AstGetNode`s naming that relation. + void RestoreInto(AstNode &ast) const; + + // Remove every recorded qualifier from `sql`. Generated SQL that OpenIVM executes itself binds + // against the same catalog the pin was peeled from, so it must not carry the pin; the pin is + // kept in the stored view SQL and re-attached when rendering for a foreign dialect. + string StripFrom(const string &sql) const; + +private: + struct Pin { + string catalog; + string schema; + string suffix; // " AT (VERSION => 366)" + }; + + // Keyed by the bare table name: `AstGetNode` carries catalog/schema separately, and an + // unqualified reference resolves its catalog/schema only during binding. + case_insensitive_map_t pins; +}; + +// Spark writes a pin *between* a relation and its alias (`FROM t VERSION AS OF 366 v`) where DuckDB +// wants the alias first (`FROM t AS v AT (VERSION => 366)`), so normalising the body has to carry +// the alias across the rewrite. That is the one step where a pin could silently land on a +// neighbouring relation, attach to the wrong alias, or be dropped altogether — and any of those +// reads a different snapshot while still compiling cleanly. +// +// So the association is checked end to end rather than trusted: the bindings are read straight off +// the *source* text, before normalisation, and re-checked against the parse tree DuckDB actually +// produced. This is deliberately an independent derivation of the same facts, not a reuse of the +// normaliser's own bookkeeping, so a regression there fails the compile instead of passing quietly. +struct SnapshotBinding { + string relation; // relation the pin was written against + string alias; // alias that relation carries, unquoted; empty when it has none + string qualifier; // "AT (VERSION => 366)" +}; + +// Read every time-travel pin written in `sql` using `dialect`'s own spelling, together with the +// relation and alias it is written against. Empty for dialects with no temporal syntax of their own. +vector CollectSourceSnapshotBindings(const string &sql, SqlDialect dialect); + +// Throw unless every source binding survived normalisation and parsing with its pin still on the +// same relation and alias. Compiling a mis-associated or dropped pin would read the wrong snapshot. +void VerifySnapshotBindings(SQLStatement &statement, const vector &bindings); + +} // namespace openivm +} // namespace duckdb + +#endif // OPENIVM_TIME_TRAVEL_PINS_HPP diff --git a/src/openivm_extension.cpp b/src/openivm_extension.cpp index 3c347ed6..e41c9033 100644 --- a/src/openivm_extension.cpp +++ b/src/openivm_extension.cpp @@ -8,6 +8,7 @@ #include "core/refresh_daemon.hpp" #include "core/refresh_locks.hpp" #include "core/sql_utils.hpp" +#include "core/time_travel_pins.hpp" #include "rules/column_hider.hpp" #include "upsert/refresh_cost_model.hpp" #include "upsert/refresh.hpp" @@ -127,6 +128,7 @@ static duckdb::unique_ptr ComputeDeltaBind(ClientContext &context, Parser parser; parser.ParseQuery(view_query); auto statement = parser.statements[0].get(); + duckdb::openivm::TimeTravelPins::PeelForLocalBinding(context, *statement); Planner planner(context); planner.CreatePlan(statement->Copy()); OPENIVM_DEBUG_PRINT("[ComputeDelta Bind] Plan:\n%s\n", planner.plan->ToString().c_str()); @@ -205,6 +207,9 @@ static void LoadInternal(ExtensionLoader &loader) { db_config.AddExtensionOption("openivm_emit_spark_hints", "emit Spark optimizer hints in target_dialect=spark compiled refresh SQL", LogicalType::BOOLEAN, Value::BOOLEAN(false)); + db_config.AddExtensionOption(duckdb::OPENIVM_INPUT_DIALECT_SETTING, + "SQL dialect materialized-view bodies are written in: duckdb (default) or spark", + LogicalType::VARCHAR, Value("duckdb"), duckdb::SetOpenIvmInputDialect); db_config.AddExtensionOption("openivm_skip_aggregate_delete", "skip zero-row DELETE for grouped aggregates when deltas are insert-only", LogicalType::BOOLEAN, Value::BOOLEAN(true)); diff --git a/src/rules/incremental_rewrite_rule.cpp b/src/rules/incremental_rewrite_rule.cpp index 55e6a5a8..a328c3eb 100644 --- a/src/rules/incremental_rewrite_rule.cpp +++ b/src/rules/incremental_rewrite_rule.cpp @@ -6,6 +6,7 @@ #include "core/parser_plan_helpers.hpp" #include "core/scoped_optimizer_settings.hpp" #include "core/sql_utils.hpp" +#include "core/time_travel_pins.hpp" #include "delta/delta_compiler.hpp" #include "duckdb/catalog/catalog_entry/table_catalog_entry.hpp" #include "duckdb/optimizer/optimizer.hpp" @@ -109,6 +110,7 @@ void IncrementalRewriteRule::IncrementalRewriteRuleFunction(OptimizerExtensionIn throw Exception(ExceptionType::PARSER, "IVM: empty view definition for '" + view + "'"); } auto statement = parser.statements[0].get(); + openivm::TimeTravelPins::PeelForLocalBinding(input.context, *statement); OPENIVM_DEBUG_PRINT("[REWRITE] About to CreatePlan for view query\n"); planner.CreatePlan(statement->Copy()); diff --git a/src/upsert/refresh_cost_model.cpp b/src/upsert/refresh_cost_model.cpp index d7e98acc..bf4e5e49 100644 --- a/src/upsert/refresh_cost_model.cpp +++ b/src/upsert/refresh_cost_model.cpp @@ -6,6 +6,7 @@ #include "core/openivm_constants.hpp" #include "core/refresh_metadata.hpp" #include "core/sql_utils.hpp" +#include "core/time_travel_pins.hpp" #include "core/openivm_debug.hpp" #include "rules/column_hider.hpp" #include "storage/ducklake_scan.hpp" @@ -907,6 +908,7 @@ string RefreshCostQuery(ClientContext &context, const FunctionParameters ¶me throw ParserException("View '" + view_name + "' has an empty IVM metadata query"); } Planner planner(con_ctx); + openivm::TimeTravelPins::PeelForLocalBinding(con_ctx, *p.statements[0]); planner.CreatePlan(p.statements[0]->Copy()); Optimizer optimizer(*planner.binder, con_ctx); auto plan = optimizer.Optimize(std::move(planner.plan)); diff --git a/src/upsert/refresh_sql.cpp b/src/upsert/refresh_sql.cpp index 43cbeb5d..09dd615c 100644 --- a/src/upsert/refresh_sql.cpp +++ b/src/upsert/refresh_sql.cpp @@ -5,6 +5,7 @@ #include "core/openivm_debug.hpp" #include "core/scoped_optimizer_settings.hpp" #include "core/sql_utils.hpp" +#include "core/time_travel_pins.hpp" #include "rules/column_hider.hpp" #include "upsert/refresh_compiler.hpp" #include "upsert/refresh_cost_model.hpp" @@ -46,7 +47,8 @@ static string SparkPortableRefreshSQL(string sql) { } static string RenderStoredViewQueryForDialect(ClientContext &context, const string &view_query_sql, - const vector &output_names, SqlDialect dialect) { + const vector &output_names, SqlDialect dialect, + const openivm::TimeTravelPins &time_travel_pins) { Parser parser(context.GetParserOptions()); parser.ParseQuery(view_query_sql); if (parser.statements.size() != 1) { @@ -54,9 +56,17 @@ static string RenderStoredViewQueryForDialect(ClientContext &context, const stri static_cast(parser.statements.size())); } Planner planner(context); + // Source qualification already peeled any time-travel pin the local catalog cannot bind, so the + // plan builds here. Re-attach the pins only when rendering for a foreign engine: DuckDB-dialect + // output runs against this same pin-less catalog, while Spark (and any other dialect LPTS can + // render) must read exactly the snapshot the view was defined against. + openivm::TimeTravelPins::PeelForLocalBinding(context, *parser.statements[0]); planner.CreatePlan(parser.statements[0]->Copy()); auto plan = std::move(planner.plan); auto ast = LogicalPlanToAst(context, plan, dialect); + if (dialect != SqlDialect::DUCKDB) { + time_travel_pins.RestoreInto(*ast); + } auto cte_list = AstToCteList(*ast, dialect); auto rendered = cte_list->ToQuery(true, output_names); if (!rendered.empty() && rendered.back() == ';') { @@ -782,6 +792,21 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam if (view_query_sql.empty()) { throw ParserException("View not found! Please call IVM with a materialized view."); } + // The stored view SQL is the only place the per-relation time-travel pins survive: source + // qualification below rewrites every base reference to its delta-qualified name and drops the + // trailing `AT (...)` qualifier so the refresh binds against the local stand-in tables. Capture + // the pins first so foreign-dialect output can re-attach them to the very same relations. + openivm::TimeTravelPins view_time_travel_pins; + { + con.BeginTransaction(); + try { + view_time_travel_pins = openivm::TimeTravelPins::FromViewSql(planning_context, view_query_sql); + con.Rollback(); + } catch (...) { + con.Rollback(); + throw; + } + } RefreshType view_query_type = metadata.GetViewType(view_name); OPENIVM_DEBUG_PRINT("[UPSERT] View: %s, Type: %d, Query: %s\n", view_name.c_str(), (int)view_query_type, view_query_sql.c_str()); @@ -875,6 +900,7 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam Parser cost_parser; cost_parser.ParseQuery(view_query_sql); Planner cost_planner(planning_context); + openivm::TimeTravelPins::PeelForLocalBinding(planning_context, *cost_parser.statements[0]); cost_planner.CreatePlan(cost_parser.statements[0]->Copy()); Optimizer cost_optimizer(*cost_planner.binder, planning_context); auto cost_plan = cost_optimizer.Optimize(std::move(cost_planner.plan)); @@ -906,8 +932,21 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam if (use_full_recompute && !full_recompute_needs_cascade_delta) { auto full_refresh_start = profile_now(); + string recompute_source_sql = view_query_sql; + if (!view_time_travel_pins.Empty() && active_facts.target_dialect != SqlDialect::DUCKDB) { + con.BeginTransaction(); + try { + recompute_source_sql = + RenderStoredViewQueryForDialect(planning_context, view_query_sql, vector(), + active_facts.target_dialect, view_time_travel_pins); + con.Rollback(); + } catch (...) { + con.Rollback(); + throw; + } + } auto recompute_query = - BuildRecomputeQuery(metadata, view_name, view_query_sql, cross_system, attached_db_catalog_name, + BuildRecomputeQuery(metadata, view_name, recompute_source_sql, cross_system, attached_db_catalog_name, attached_db_schema_name, internal_catalog_prefix, metadata_prefix, out_post_meta); add_profile_step("generate_refresh_sql.dispatch", full_refresh_start, "full_recompute=true; metadata_requires_full_refresh=" + @@ -1454,8 +1493,8 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam } con.BeginTransaction(); try { - full_recompute_query = RenderStoredViewQueryForDialect(planning_context, view_query_sql, output_names, - active_facts.target_dialect); + full_recompute_query = RenderStoredViewQueryForDialect( + planning_context, view_query_sql, output_names, active_facts.target_dialect, view_time_travel_pins); con.Rollback(); } catch (...) { con.Rollback(); @@ -1702,6 +1741,12 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam auto lpts_start = profile_now(); SqlDialect dialect = active_facts.target_dialect; auto ast = LogicalPlanToAst(con_ctx, plan, dialect); + if (dialect != SqlDialect::DUCKDB) { + // The delta plan still scans the pinned base relations alongside the delta + // tables; re-attach each pin so the target engine reads the snapshot the view + // was defined against. + view_time_travel_pins.RestoreInto(*ast); + } bool emit_spark_hints = dialect == SqlDialect::SPARK && (active_facts.emit_spark_hints || SqlUtils::GetBoolSetting(con_ctx, "openivm_emit_spark_hints", false)); diff --git a/test/sql/time_travel.test b/test/sql/time_travel.test new file mode 100644 index 00000000..8ad04fab --- /dev/null +++ b/test/sql/time_travel.test @@ -0,0 +1,446 @@ +# name: test/sql/time_travel.test +# description: Spark/Delta time-travel pins survive parsing, binding and dialect rendering. +# group: [sql] + +require openivm + +statement ok +SET openivm_files_path='__TEST_DIR__'; + +statement ok +CREATE TABLE tt_orders(o_id INT, c_id INT, amount INT); + +statement ok +CREATE TABLE tt_customers(c_id INT, region VARCHAR); + +statement ok +INSERT INTO tt_orders VALUES (1, 1, 10), (2, 2, 20); + +statement ok +INSERT INTO tt_customers VALUES (1, 'us'), (2, 'eu'); + +# ========================================== +# Input parsing: Spark syntax is only accepted when the input dialect says so +# ========================================== + +# The default input dialect is DuckDB, so raw Spark time-travel syntax must still +# be a parse error rather than being silently reinterpreted. +statement error +CREATE MATERIALIZED VIEW tt_rejected AS + SELECT c_id, SUM(amount) AS total FROM tt_orders VERSION AS OF 366 GROUP BY c_id; +---- +syntax error + +statement error +SET openivm_input_dialect='klingon'; +---- +openivm_input_dialect + +statement ok +SET openivm_input_dialect='spark'; + +# ========================================== +# Binding: a pinned scan binds against a plain in-memory table and loads data +# ========================================== + +statement ok +CREATE MATERIALIZED VIEW tt_single AS + SELECT c_id, SUM(amount) AS total FROM tt_orders VERSION AS OF 366 GROUP BY c_id; + +query II +SELECT c_id, total FROM tt_single ORDER BY c_id; +---- +1 10 +2 20 + +# The pin is preserved on the stored view SQL, in DuckDB spelling, on the pinned relation. +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_single'; +---- +1 + +# A TIMESTAMP pin is represented the same way. +statement ok +CREATE MATERIALIZED VIEW tt_stamp AS + SELECT c_id, SUM(amount) AS total + FROM tt_orders TIMESTAMP AS OF '2024-01-01 00:00:00' + GROUP BY c_id; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (TIMESTAMP => ''2024-01-01 00:00:00'')%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_stamp'; +---- +1 + +# ========================================== +# Relation/pin association: aliases, joins, two differently pinned relations +# ========================================== + +statement ok +CREATE MATERIALIZED VIEW tt_join AS + SELECT c.region AS region, SUM(o.amount) AS total + FROM tt_orders VERSION AS OF 366 o + JOIN tt_customers VERSION AS OF 12 c ON o.c_id = c.c_id + GROUP BY c.region; + +query II +SELECT region, total FROM tt_join ORDER BY region; +---- +eu 20 +us 10 + +# Each pin must land on its own relation - never conflated, never swapped. +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' + AND sql_string LIKE '%tt_customers AT (VERSION => 12)%' + AND sql_string NOT LIKE '%tt_orders AT (VERSION => 12)%' + AND sql_string NOT LIKE '%tt_customers AT (VERSION => 366)%' + THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_join'; +---- +1 + +# Repeated scans of the same relation inside CTEs keep the pin on every scan. +statement ok +CREATE MATERIALIZED VIEW tt_cte AS + WITH a AS (SELECT c_id, amount FROM tt_orders VERSION AS OF 366), + b AS (SELECT c_id, amount FROM tt_orders VERSION AS OF 366) + SELECT a.c_id AS c_id, SUM(a.amount + b.amount) AS total + FROM a JOIN b ON a.c_id = b.c_id + GROUP BY a.c_id; + +query II +SELECT (length(sql_string) - length(replace(sql_string, 'tt_orders AT (VERSION => 366)', ''))) / + length('tt_orders AT (VERSION => 366)'), + (length(sql_string) - length(replace(sql_string, 'tt_orders', ''))) / length('tt_orders') +FROM openivm_views WHERE view_name = 'tt_cte'; +---- +2 2 + +# Explicit AS aliases and Spark backtick identifiers both survive input normalization with the +# pin attached to the right relation. +statement ok +CREATE MATERIALIZED VIEW tt_backtick AS + SELECT c.region AS region, SUM(o.amount) AS total + FROM `tt_orders` VERSION AS OF 366 AS o + JOIN `tt_customers` VERSION AS OF 12 AS c ON o.c_id = c.c_id + GROUP BY c.region; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' + AND sql_string LIKE '%tt_customers AT (VERSION => 12)%' + THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_backtick'; +---- +1 + +query II +SELECT region, total FROM tt_backtick ORDER BY region; +---- +eu 20 +us 10 + +# A CTE that shadows a relation name is not a scan of that relation, so it must not be mistaken +# for an unpinned scan of the pinned table. +statement ok +CREATE MATERIALIZED VIEW tt_shadow AS + WITH tt_orders AS (SELECT c_id, amount FROM tt_orders VERSION AS OF 366) + SELECT c_id, SUM(amount) AS total FROM tt_orders GROUP BY c_id; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_shadow'; +---- +1 + +# ========================================== +# Alias-to-pin association +# ========================================== +# +# Spark writes the alias *after* the pin (`FROM t VERSION AS OF n p`) where DuckDB wants it before +# (`FROM t AS p AT (VERSION => n)`), so normalization has to carry the alias across the rewrite. +# That must keep every alias bound to the relation it was written against: an alias may never +# migrate to a neighbouring relation, and a pin may never migrate to a neighbouring alias or be +# dropped. Each case below is cross-checked against the parse tree by `VerifySnapshotBindings`. + +# A bare (no `AS`) trailing alias is the exact shape Spark produces. +statement ok +CREATE MATERIALIZED VIEW tt_bare_alias AS + SELECT p.c_id AS c_id, SUM(p.amount) AS total + FROM tt_orders VERSION AS OF 2 p + GROUP BY p.c_id; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 2)%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_bare_alias'; +---- +1 + +query II +SELECT c_id, total FROM tt_bare_alias ORDER BY c_id; +---- +1 10 +2 20 + +# Aliases that deliberately collide with the *other* relation's name: the pin belongs to the +# relation, not to whatever the scan happens to be called. +statement ok +CREATE MATERIALIZED VIEW tt_alias_swap AS + SELECT tt_orders.region AS region, SUM(tt_customers.amount) AS total + FROM tt_orders VERSION AS OF 366 tt_customers + JOIN tt_customers VERSION AS OF 12 tt_orders ON tt_customers.c_id = tt_orders.c_id + GROUP BY tt_orders.region; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' + AND sql_string LIKE '%tt_customers AT (VERSION => 12)%' + AND sql_string NOT LIKE '%tt_orders AT (VERSION => 12)%' + AND sql_string NOT LIKE '%tt_customers AT (VERSION => 366)%' + THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_alias_swap'; +---- +1 + +query II +SELECT region, total FROM tt_alias_swap ORDER BY region; +---- +eu 20 +us 10 + +# Two pinned relations, each with its own bare alias, joined: neither alias nor pin may cross over. +statement ok +CREATE MATERIALIZED VIEW tt_two_bare_aliases AS + SELECT q.region AS region, SUM(p.amount) AS total + FROM tt_orders VERSION AS OF 2 p + JOIN tt_customers VERSION AS OF 7 q ON p.c_id = q.c_id + GROUP BY q.region; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 2)%' + AND sql_string LIKE '%tt_customers AT (VERSION => 7)%' + AND sql_string NOT LIKE '%tt_orders AT (VERSION => 7)%' + AND sql_string NOT LIKE '%tt_customers AT (VERSION => 2)%' + THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_two_bare_aliases'; +---- +1 + +query II +SELECT region, total FROM tt_two_bare_aliases ORDER BY region; +---- +eu 20 +us 10 + +# A clause keyword directly after the pin is not an alias, so nothing may be consumed from the +# WHERE that follows it. +statement ok +CREATE MATERIALIZED VIEW tt_no_alias AS + SELECT c_id, SUM(amount) AS total + FROM tt_orders VERSION AS OF 366 + WHERE amount > 5 + GROUP BY c_id; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_no_alias'; +---- +1 + +query II +SELECT c_id, total FROM tt_no_alias ORDER BY c_id; +---- +1 10 +2 20 + +# A column-alias list belongs to the alias, so it has to travel with it across the pin. +statement ok +CREATE MATERIALIZED VIEW tt_column_alias AS + SELECT p.p_c_id AS c_id, SUM(p.p_amount) AS total + FROM tt_orders VERSION AS OF 366 AS p (p_o_id, p_c_id, p_amount) + GROUP BY p.p_c_id; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_column_alias'; +---- +1 + +query II +SELECT c_id, total FROM tt_column_alias ORDER BY c_id; +---- +1 10 +2 20 + +# A pinned relation joined to an unpinned one keeps the pin on its own side only. +statement ok +CREATE MATERIALIZED VIEW tt_half_pinned AS + SELECT c.region AS region, SUM(o.amount) AS total + FROM tt_orders VERSION AS OF 366 o + JOIN tt_customers c ON o.c_id = c.c_id + GROUP BY c.region; + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders AT (VERSION => 366)%' + AND sql_string NOT LIKE '%tt_customers AT (%' + THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_half_pinned'; +---- +1 + +# ========================================== +# Ambiguity is refused, never silently conflated +# ========================================== + +statement error +CREATE MATERIALIZED VIEW tt_two_pins AS + SELECT a.c_id AS c_id, SUM(a.amount + b.amount) AS total + FROM tt_orders VERSION AS OF 366 a JOIN tt_orders VERSION AS OF 12 b ON a.c_id = b.c_id + GROUP BY a.c_id; +---- +pins relation 'tt_orders' ambiguously + +statement error +CREATE MATERIALIZED VIEW tt_mixed_pins AS + SELECT a.c_id AS c_id, SUM(a.amount + b.amount) AS total + FROM tt_orders VERSION AS OF 366 a JOIN tt_orders b ON a.c_id = b.c_id + GROUP BY a.c_id; +---- +scanned both pinned and unpinned + +# ========================================== +# Emitted SQL: Spark re-emits VERSION AS OF, DuckDB drops it, others fail loudly +# ========================================== + +statement ok +INSERT INTO tt_orders VALUES (3, 1, 5); + +# The incremental delta program still scans the pinned dimension, so Spark output +# must carry that relation's pin in Spark spelling - not the DuckDB AT (...) form. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers` VERSION AS OF 12%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_join', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +statement ok +SET openivm_refresh_mode='full'; + +# Full recompute for Spark renders both pins on their own relations. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders` VERSION AS OF 366%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers` VERSION AS OF 12%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_join', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# DuckDB-dialect output runs against this catalog, which holds no snapshots, so the +# pin must be gone entirely rather than emitted as unbindable syntax. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%VERSION AS OF%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_join', '{"target_dialect":"duckdb","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# A dialect with no verified time-travel syntax must fail explicitly instead of +# quietly reading the latest snapshot. +statement error +SELECT sql FROM openivm_compile_with_facts('tt_join', '{"target_dialect":"postgres","compile_only":true}'); +---- +LPTS_UNSUPPORTED_TIME_TRAVEL + +statement ok +SET openivm_refresh_mode='auto'; + +# ========================================== +# Refresh still maintains a pinned view against this catalog +# ========================================== + +statement ok +PRAGMA refresh('tt_join'); + +query II +SELECT region, total FROM tt_join ORDER BY region; +---- +eu 20 +us 15 + +query I +SELECT COUNT(*) FROM ( + SELECT region, total FROM tt_join + EXCEPT ALL + SELECT c.region AS region, SUM(o.amount) AS total + FROM tt_orders o JOIN tt_customers c ON o.c_id = c.c_id + GROUP BY c.region +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + SELECT c.region AS region, SUM(o.amount) AS total + FROM tt_orders o JOIN tt_customers c ON o.c_id = c.c_id + GROUP BY c.region + EXCEPT ALL + SELECT region, total FROM tt_join +); +---- +0 + +# ========================================== +# Unpinned views are untouched by the Spark input dialect +# ========================================== + +statement ok +CREATE MATERIALIZED VIEW tt_plain AS + SELECT c_id, SUM(amount) AS total FROM tt_orders GROUP BY c_id; + +query I +SELECT CASE WHEN sql_string NOT LIKE '%AT (%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_plain'; +---- +1 + +statement ok +INSERT INTO tt_orders VALUES (4, 2, 7); + +statement ok +PRAGMA refresh('tt_plain'); + +query I +SELECT COUNT(*) FROM ( + SELECT c_id, total FROM tt_plain + EXCEPT ALL + SELECT c_id, SUM(amount) AS total FROM tt_orders GROUP BY c_id +); +---- +0 + +query I +SELECT COUNT(*) FROM ( + SELECT c_id, SUM(amount) AS total FROM tt_orders GROUP BY c_id + EXCEPT ALL + SELECT c_id, total FROM tt_plain +); +---- +0 + +statement ok +SET openivm_input_dialect='duckdb'; + +# With the input dialect back to DuckDB, the native AT (...) spelling still binds +# unchanged for catalogs that support it, and Spark syntax is a parse error again. +statement error +CREATE MATERIALIZED VIEW tt_rejected_again AS + SELECT c_id, SUM(amount) AS total FROM tt_orders VERSION AS OF 366 GROUP BY c_id; +---- +syntax error From 1061cf45230f1b7dcc055340d416119560c92374 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Wed, 26 Aug 2026 00:43:46 +0000 Subject: [PATCH 13/18] fix: keep time-travel pins on the paths that never reach the AST Three ways a pin could be lost or misplaced after compilation: - `StripFrom` tracked only the most recent identifier, so a pin written after an alias (`FROM t p AT (VERSION => 366)`, the shape normalization produces from Spark's `FROM t VERSION AS OF 366 p`) was credited to the alias and left in place. The view then executed locally against a catalog that cannot bind the qualifier and failed at CREATE. - Refresh programs assembled as SQL text rather than rendered from the AST (min/max aggregates, group recompute, interrupted-refresh recovery) never passed through `RestoreInto`, so they shipped to the target engine reading the latest snapshot instead of the pinned one. `RestoreIntoSql` re-attaches every pin, in the target dialect's own spelling via LPTS, to FROM/JOIN scans that do not already carry it. - `Peel` skipped unpinned scans in catalogs that honour pins natively. Re-attachment is keyed by relation name, so a pin peeled off one relation could land on a same-named relation that was deliberately read unpinned; that ambiguity is now refused like any other. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/core/time_travel_pins.cpp | 185 ++++++++++++++++++++++++-- src/include/core/time_travel_pins.hpp | 9 ++ src/upsert/refresh_sql.cpp | 17 ++- test/sql/time_travel.test | 85 ++++++++++++ test/sql/time_travel_ducklake.test | 49 +++++++ 5 files changed, 333 insertions(+), 12 deletions(-) create mode 100644 test/sql/time_travel_ducklake.test diff --git a/src/core/time_travel_pins.cpp b/src/core/time_travel_pins.cpp index d0d6f3a3..ba1844fb 100644 --- a/src/core/time_travel_pins.cpp +++ b/src/core/time_travel_pins.cpp @@ -19,6 +19,7 @@ #include "duckdb/parser/tableref/table_function_ref.hpp" #include "duckdb/parser/parsed_data/create_table_info.hpp" #include "duckdb/parser/parser.hpp" +#include "lpts_helpers.hpp" #include "lpts_sql_scanner.hpp" #include @@ -238,13 +239,17 @@ TimeTravelPins TimeTravelPins::Peel(ClientContext &context, SQLStatement &statem TimeTravelPins result; case_insensitive_set_t unpinned; BaseTableRefCallback callback = [&](BaseTableRef &ref) { - if (CatalogHonoursPin(context, ref)) { - return; - } if (!ref.at_clause) { + // Every unpinned scan is recorded, including one in a catalog that honours pins + // natively: re-attachment is keyed by relation name and an unqualified pin matches any + // catalog, so a pin peeled off one relation would otherwise land on a same-named + // relation that was deliberately read unpinned. unpinned.insert(ref.table_name); return; } + if (CatalogHonoursPin(context, ref)) { + return; + } Pin pin; // INVALID_CATALOG / INVALID_SCHEMA are the empty string, so an unqualified reference already // stores the "" this map treats as "matches any qualifier". @@ -367,25 +372,55 @@ static idx_t MatchingParen(const string &sql, idx_t open) { return DConstants::INVALID_INDEX; } +// The identifier a trailing qualifier belongs to. DuckDB spells a pin *after* the alias +// (`FROM t AS v AT (VERSION => 366)`, the shape LPTS normalises Spark's `FROM t VERSION AS OF 366 v` +// into), so the pinned relation is one alias-step behind the most recent identifier. Tracking only +// the most recent one credits the qualifier to the alias and leaves the pin in place, which sends a +// scan the local catalog cannot bind into SQL OpenIVM executes itself. +struct RelationCursor { + string last; // most recent identifier: the alias when the relation carries one + string previous; // the identifier before it: the relation itself when `last` is its alias + + void Push(const string &identifier) { + // `AS` introduces the alias of the relation already in `last`, so it must not shift it out. + if (StringUtil::CIEquals(identifier, "as")) { + return; + } + previous = last; + last = identifier; + } + + void Reset() { + last.clear(); + previous.clear(); + } +}; + string TimeTravelPins::StripFrom(const string &sql) const { if (pins.empty()) { return sql; } string result; result.reserve(sql.size()); - string last_identifier; + RelationCursor cursor; + auto pinned_relation = [&]() { + if (pins.find(cursor.last) != pins.end()) { + return true; + } + return pins.find(cursor.previous) != pins.end(); + }; idx_t i = 0; while (i < sql.size()) { char c = sql[i]; if (c == '\'') { i = CopyQuotedRun(sql, i, result); - last_identifier.clear(); + cursor.Reset(); continue; } if (c == '"' || c == '`') { idx_t start = result.size(); i = CopyQuotedRun(sql, i, result); - last_identifier = result.substr(start + 1, result.size() - start - 2); + cursor.Push(result.substr(start + 1, result.size() - start - 2)); continue; } if (c == '-' && i + 1 < sql.size() && sql[i + 1] == '-') { @@ -411,8 +446,7 @@ string TimeTravelPins::StripFrom(const string &sql) const { while (after < sql.size() && std::isspace(static_cast(sql[after]))) { after++; } - if (StringUtil::CIEquals(token, "at") && after < sql.size() && sql[after] == '(' && - pins.find(last_identifier) != pins.end()) { + if (StringUtil::CIEquals(token, "at") && after < sql.size() && sql[after] == '(' && pinned_relation()) { auto close = MatchingParen(sql, after); if (close != DConstants::INVALID_INDEX) { while (!result.empty() && std::isspace(static_cast(result.back()))) { @@ -423,13 +457,144 @@ string TimeTravelPins::StripFrom(const string &sql) const { } } result += token; - last_identifier = token; + cursor.Push(token); i = end; continue; } result += c; if (c != '.' && !std::isspace(static_cast(c))) { - last_identifier.clear(); + cursor.Reset(); + } + i++; + } + return result; +} + +// Copy the `[catalog.][schema.]relation` chain starting at `sql[start]` into `result`, honouring +// quoted components, and report the unquoted final component — the relation name pins are keyed by. +static idx_t CopyQualifiedIdentifierChain(const string &sql, idx_t start, string &result, string &final_component) { + idx_t i = start; + while (true) { + if (i < sql.size() && (sql[i] == '"' || sql[i] == '`')) { + idx_t quoted_start = result.size(); + i = CopyQuotedRun(sql, i, result); + final_component = result.substr(quoted_start + 1, result.size() - quoted_start - 2); + } else if (i < sql.size() && IsIdentifierStart(sql[i])) { + idx_t end = i; + while (end < sql.size() && IsIdentifierPart(sql[end])) { + end++; + } + final_component = sql.substr(i, end - i); + result.append(sql, i, end - i); + i = end; + } else { + break; + } + if (i < sql.size() && sql[i] == '.') { + result += '.'; + i++; + continue; + } + break; + } + return i; +} + +// Whether `sql` already carries `qualifier` at `pos` (ignoring how its whitespace is spelled), so an +// AST-rendered scan is never given a second copy of its own pin. +static bool CarriesQualifierAt(const string &sql, idx_t pos, const string &qualifier) { + idx_t sql_pos = pos; + idx_t qualifier_pos = 0; + while (qualifier_pos < qualifier.size()) { + if (std::isspace(static_cast(qualifier[qualifier_pos]))) { + bool sql_has_space = sql_pos < sql.size() && std::isspace(static_cast(sql[sql_pos])); + while (qualifier_pos < qualifier.size() && + std::isspace(static_cast(qualifier[qualifier_pos]))) { + qualifier_pos++; + } + while (sql_pos < sql.size() && std::isspace(static_cast(sql[sql_pos]))) { + sql_pos++; + } + if (!sql_has_space) { + return false; + } + continue; + } + if (sql_pos >= sql.size() || std::tolower(static_cast(sql[sql_pos])) != + std::tolower(static_cast(qualifier[qualifier_pos]))) { + return false; + } + sql_pos++; + qualifier_pos++; + } + return true; +} + +// Render `at_suffix` (` AT (VERSION => 366)`) in `dialect`'s own spelling. LPTS owns both the +// spelling and the refusal for dialects with no verified time-travel syntax, so this never guesses. +static string DialectPinSuffix(const string &at_suffix, SqlDialect dialect) { + string base_name; + string dialect_suffix; + if (!TrySplitDialectSnapshotSuffix("openivm_pinned_relation" + at_suffix, dialect, base_name, dialect_suffix)) { + throw InternalException("OpenIVM could not render the time-travel pin '%s'", at_suffix); + } + return dialect_suffix; +} + +string TimeTravelPins::RestoreIntoSql(const string &sql, SqlDialect dialect) const { + if (pins.empty()) { + return sql; + } + string result; + result.reserve(sql.size()); + // Only a relation directly behind FROM or JOIN is a scan; anything else naming the relation is a + // column reference, a delta/metadata table or a literal, none of which take a pin. + bool expect_relation = false; + idx_t i = 0; + while (i < sql.size()) { + char c = sql[i]; + if (c == '\'') { + i = CopyQuotedRun(sql, i, result); + expect_relation = false; + continue; + } + if (c == '-' && i + 1 < sql.size() && sql[i + 1] == '-') { + while (i < sql.size() && sql[i] != '\n') { + result += sql[i++]; + } + continue; + } + if (c == '/' && i + 1 < sql.size() && sql[i + 1] == '*') { + auto close = sql.find("*/", i + 2); + auto end = close == string::npos ? sql.size() : close + 2; + result.append(sql, i, end - i); + i = end; + continue; + } + if (IsIdentifierStart(c) || c == '"' || c == '`') { + string final_component; + i = CopyQualifiedIdentifierChain(sql, i, result, final_component); + if (expect_relation) { + auto entry = pins.find(final_component); + if (entry != pins.end()) { + auto dialect_suffix = DialectPinSuffix(entry->second.suffix, dialect); + if (!CarriesQualifierAt(sql, i, dialect_suffix)) { + result += dialect_suffix; + OPENIVM_DEBUG_PRINT("[TIME TRAVEL] Restored pin '%s' onto rendered scan of '%s'\n", + dialect_suffix.c_str(), final_component.c_str()); + } + } + expect_relation = false; + continue; + } + expect_relation = + StringUtil::CIEquals(final_component, "from") || StringUtil::CIEquals(final_component, "join"); + continue; + } + result += c; + // A comma continues a FROM list; anything else that is not whitespace ends the table position. + if (c != ',' && !std::isspace(static_cast(c))) { + expect_relation = false; } i++; } diff --git a/src/include/core/time_travel_pins.hpp b/src/include/core/time_travel_pins.hpp index 95148be2..294de13b 100644 --- a/src/include/core/time_travel_pins.hpp +++ b/src/include/core/time_travel_pins.hpp @@ -54,6 +54,15 @@ class TimeTravelPins { // kept in the stored view SQL and re-attached when rendering for a foreign dialect. string StripFrom(const string &sql) const; + // Re-attach every recorded qualifier, in `dialect`'s own spelling, to the scans of that relation + // in already-rendered `sql`. Refresh programs for several view shapes (min/max aggregates, + // group recompute, interrupted-refresh recovery, ...) are assembled as SQL text rather than + // through the AST, so `RestoreInto` never sees them; without this they would ship to the target + // engine reading the latest snapshot instead of the pinned one. Scans that already carry the + // qualifier are left alone, so it is safe to run over AST-rendered SQL as well. Throws through + // LPTS for dialects with no verified time-travel syntax rather than emitting an unpinned scan. + string RestoreIntoSql(const string &sql, SqlDialect dialect) const; + private: struct Pin { string catalog; diff --git a/src/upsert/refresh_sql.cpp b/src/upsert/refresh_sql.cpp index 09dd615c..1024f925 100644 --- a/src/upsert/refresh_sql.cpp +++ b/src/upsert/refresh_sql.cpp @@ -807,6 +807,18 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam throw; } } + // Only the AST-rendered refresh paths run the pins back through `RestoreInto`. Several view + // shapes (min/max aggregates, group recompute, interrupted-refresh recovery, ...) assemble + // their refresh program as SQL text instead, which the pins never reach — those would ship to + // the target engine reading the latest snapshot rather than the pinned one. Every exit is + // therefore finalized here: scans that already carry their qualifier are left untouched, and a + // dialect with no time-travel syntax still refuses through LPTS. + auto finalize_refresh_sql = [&](string refresh_sql) { + if (view_time_travel_pins.Empty() || active_facts.target_dialect == SqlDialect::DUCKDB) { + return refresh_sql; + } + return view_time_travel_pins.RestoreIntoSql(refresh_sql, active_facts.target_dialect); + }; RefreshType view_query_type = metadata.GetViewType(view_name); OPENIVM_DEBUG_PRINT("[UPSERT] View: %s, Type: %d, Query: %s\n", view_name.c_str(), (int)view_query_type, view_query_sql.c_str()); @@ -838,7 +850,7 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam " SET refresh_in_progress = false WHERE view_name = '" + SqlUtils::EscapeValue(view_name) + "';\n"; } - return recovery_query; + return finalize_refresh_sql(recovery_query); } } add_profile_step("generate_refresh_sql.recovery_check", recovery_start); @@ -953,7 +965,7 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam string(metadata_requires_full_refresh ? "true" : "false") + "; adaptive_recompute=" + string(adaptive_recompute ? "true" : "false") + "; sql_bytes=" + to_string(recompute_query.size())); - return recompute_query; + return finalize_refresh_sql(recompute_query); } RefreshType dispatch_refresh_type = use_full_recompute ? RefreshType::FULL_REFRESH : view_query_type; refresh_plan.refresh_type = dispatch_refresh_type; @@ -1938,6 +1950,7 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam } else { clean_query = meta_pre_sql + data_sql + meta_post_sql; } + clean_query = finalize_refresh_sql(std::move(clean_query)); Value files_path_val; if (context.TryGetCurrentSetting("openivm_files_path", files_path_val) && !files_path_val.IsNull()) { string refresh_file_path = files_path_val.ToString() + "/openivm_upsert_queries_" + view_name + ".sql"; diff --git a/test/sql/time_travel.test b/test/sql/time_travel.test index 8ad04fab..7fd96be6 100644 --- a/test/sql/time_travel.test +++ b/test/sql/time_travel.test @@ -396,6 +396,91 @@ SELECT COUNT(*) FROM ( ---- 0 +# ========================================== +# Pins survive the paths that do not go through the AST +# ========================================== +# +# Bodies that never reach the AST keep their pin in the normalized *text*, where the alias sits +# between the relation and its `AT (...)` qualifier. Stripping the pin for local execution has to +# find it there too, or the view is executed against this catalog with a qualifier it cannot bind. +statement ok +CREATE MATERIALIZED VIEW tt_fallback_alias AS + SELECT p.c_id AS c_id, SUM(p.amount) AS total + FROM tt_orders VERSION AS OF 366 p + WHERE p.c_id IN (SELECT c_id FROM tt_customers EXCEPT SELECT c_id FROM tt_customers WHERE region = 'zz') + GROUP BY p.c_id; + +query II +SELECT c_id, total FROM tt_fallback_alias ORDER BY c_id; +---- +1 15 +2 20 + +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders p AT (VERSION => 366)%' THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_fallback_alias'; +---- +1 + +# Refresh programs that are assembled as SQL text rather than rendered from the AST (min/max +# aggregates, group recompute, recovery) must still pin every scan they emit: reading the latest +# snapshot instead of the pinned one has to be impossible, not merely unlikely. +statement ok +CREATE MATERIALIZED VIEW tt_minmax AS + SELECT c_id, MIN(amount) AS lo, MAX(amount) AS hi + FROM tt_orders VERSION AS OF 366 + GROUP BY c_id; + +query III +SELECT c_id, lo, hi FROM tt_minmax ORDER BY c_id; +---- +1 5 10 +2 20 20 + +statement ok +INSERT INTO tt_orders VALUES (5, 1, 3); + +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%VERSION AS OF 366%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_minmax', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# An interrupted refresh recovers by recomputing from source, and that recovery program is built as +# text as well, so both pins have to survive it. +statement ok +UPDATE openivm_views SET refresh_in_progress = true WHERE view_name = 'tt_join'; + +query I +SELECT CASE WHEN (string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders VERSION AS OF 366%' + OR string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders` VERSION AS OF 366%') + AND (string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers VERSION AS OF 12%' + OR string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers` VERSION AS OF 12%') + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%tt_orders VERSION AS OF 12%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%tt_customers VERSION AS OF 366%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_join', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# The same recovery program for DuckDB drops the pins instead, since this catalog holds no snapshots. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%VERSION AS OF%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_join', '{"target_dialect":"duckdb","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +statement ok +UPDATE openivm_views SET refresh_in_progress = false WHERE view_name = 'tt_join'; + # ========================================== # Unpinned views are untouched by the Spark input dialect # ========================================== diff --git a/test/sql/time_travel_ducklake.test b/test/sql/time_travel_ducklake.test new file mode 100644 index 00000000..b80a65c2 --- /dev/null +++ b/test/sql/time_travel_ducklake.test @@ -0,0 +1,49 @@ +# name: test/sql/time_travel_ducklake.test +# description: A pin may never leak onto a same-named relation in a catalog that honours pins. +# group: [sql] + +require openivm + +require parquet + +statement ok +INSTALL ducklake; + +statement ok +LOAD ducklake; + +statement ok +SET openivm_files_path='__TEST_DIR__'; + +statement ok +ATTACH '__TEST_DIR__/time_travel_ducklake.db' AS dl (TYPE ducklake); + +statement ok +CREATE TABLE dl.main.tt_orders(o_id INT, c_id INT, amount INT); + +statement ok +INSERT INTO dl.main.tt_orders VALUES (1, 1, 10), (2, 2, 20); + +statement ok +CREATE TABLE tt_orders(o_id INT, c_id INT, amount INT); + +statement ok +INSERT INTO tt_orders VALUES (1, 1, 10), (2, 2, 20); + +statement ok +SET openivm_input_dialect='spark'; + +# `memory.tt_orders` is pinned, `dl.main.tt_orders` is not. Pins are keyed by relation name, so a +# catalog that honours time travel is exactly where a stray pin would bind and silently read the +# wrong snapshot of a different table. The ambiguity has to be refused instead. +statement error +CREATE MATERIALIZED VIEW tt_mixed AS + SELECT a.c_id AS c_id, SUM(a.amount + b.amount) AS total + FROM tt_orders VERSION AS OF 366 a + JOIN dl.main.tt_orders b ON a.c_id = b.c_id + GROUP BY a.c_id; +---- +scanned both pinned and unpinned + +statement ok +SET openivm_input_dialect='duckdb'; From c173da721c8612758f7000705d5765a5493b12b9 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Wed, 26 Aug 2026 01:41:33 +0000 Subject: [PATCH 14/18] fix: translate text-carried time-travel pins in place `RestoreIntoSql` treated every pinned scan as if it were AST-rendered. Bodies that never reach the AST keep their pin in the normalized text, where DuckDB spells it after the alias, so the qualifier was inserted a second time in front of that alias and the raw one was left standing: `FROM t VERSION AS OF 366 o AT (VERSION => 366)` is not valid in any dialect. It also armed relation scanning on FROM and JOIN only, so in a comma FROM list every relation after the first went out unpinned, silently reading the latest snapshot. The scanner now looks past an optional `[AS] alias` for a raw `AT (...)` belonging to the relation, replaces it with the target dialect's spelling directly behind the relation and re-emits the alias after it, and keeps the FROM list armed across commas at its own parenthesis depth so every implicit cross join is pinned. Names bound by a `WITH` clause are collected up front so a CTE reference that shadows a pinned relation is never handed a qualifier it cannot carry. Translating stays idempotent, comment- and string-safe, and unsupported dialects still refuse. DuckDB-dialect output now strips the pins from these text-assembled programs instead of passing them through: they run against a catalog that holds no snapshots, where a raw qualifier failed the refresh with `Catalog type does not support time travel`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/core/time_travel_pins.cpp | 228 +++++++++++++++++++++++--- src/include/core/time_travel_pins.hpp | 8 +- src/upsert/refresh_sql.cpp | 8 +- test/sql/time_travel.test | 182 ++++++++++++++++++++ 4 files changed, 394 insertions(+), 32 deletions(-) diff --git a/src/core/time_travel_pins.cpp b/src/core/time_travel_pins.cpp index ba1844fb..77ad2d1a 100644 --- a/src/core/time_travel_pins.cpp +++ b/src/core/time_travel_pins.cpp @@ -470,10 +470,22 @@ string TimeTravelPins::StripFrom(const string &sql) const { return result; } +// Words that may legally follow a table reference; none of them can be a bare alias. +static bool CanBeBareAlias(const string &token) { + // DuckDB's `alias_clause` takes a `ColId`: plain identifiers plus the unreserved and column-name + // keywords, but not the reserved or type/function ones. So `WHERE`, `JOIN` and `NATURAL` end the + // relation instead of naming it. + auto category = Parser::IsKeyword(StringUtil::Lower(token)); + return category != KeywordCategory::KEYWORD_RESERVED && category != KeywordCategory::KEYWORD_TYPE_FUNC; +} + // Copy the `[catalog.][schema.]relation` chain starting at `sql[start]` into `result`, honouring -// quoted components, and report the unquoted final component — the relation name pins are keyed by. -static idx_t CopyQualifiedIdentifierChain(const string &sql, idx_t start, string &result, string &final_component) { +// quoted components. Reports the unquoted final component — the relation name pins are keyed by — +// and whether the chain carried a catalog/schema prefix, which a CTE reference never does. +static idx_t CopyQualifiedIdentifierChain(const string &sql, idx_t start, string &result, string &final_component, + bool &qualified) { idx_t i = start; + qualified = false; while (true) { if (i < sql.size() && (sql[i] == '"' || sql[i] == '`')) { idx_t quoted_start = result.size(); @@ -493,6 +505,7 @@ static idx_t CopyQualifiedIdentifierChain(const string &sql, idx_t start, string if (i < sql.size() && sql[i] == '.') { result += '.'; i++; + qualified = true; continue; } break; @@ -541,15 +554,151 @@ static string DialectPinSuffix(const string &at_suffix, SqlDialect dialect) { return dialect_suffix; } +// Advance past whitespace and comments so a qualifier written behind either is still found. +static idx_t SkipIgnorableSpan(const string &sql, idx_t pos) { + while (pos < sql.size()) { + if (std::isspace(static_cast(sql[pos]))) { + pos++; + continue; + } + if (sql[pos] == '-' && pos + 1 < sql.size() && sql[pos + 1] == '-') { + auto newline = sql.find('\n', pos); + pos = newline == string::npos ? sql.size() : newline + 1; + continue; + } + if (sql[pos] == '/' && pos + 1 < sql.size() && sql[pos + 1] == '*') { + auto close = sql.find("*/", pos + 2); + pos = close == string::npos ? sql.size() : close + 2; + continue; + } + break; + } + return pos; +} + +// Read a raw DuckDB `AT (...)` qualifier belonging to the relation that ends at `pos`. Bodies that +// never reach the AST keep their pin in normalized text, where DuckDB spells it *after* the alias +// (`t AS p AT (VERSION => 366)`); every other dialect wants it directly behind the relation. The +// alias text in between is handed back verbatim so it can be re-emitted after the translated pin, +// and `end` reports where the raw clause stops so the caller drops it instead of keeping both. +static bool TryReadRawPinAfterRelation(const string &sql, idx_t pos, string &alias_text, idx_t &end) { + string buffer; + idx_t cursor = pos; + // `[AS] alias` is at most two tokens, so the qualifier has to appear within three. + for (idx_t token_index = 0; token_index < 3; token_index++) { + idx_t token_start = SkipIgnorableSpan(sql, cursor); + idx_t token_end; + string token; + if (token_start < sql.size() && (sql[token_start] == '"' || sql[token_start] == '`')) { + token_end = CopyQuotedRun(sql, token_start, token); + } else if (!TryReadIdentifierToken(sql, token_start, token_end, token)) { + return false; + } + if (StringUtil::CIEquals(token, "at")) { + idx_t paren = SkipIgnorableSpan(sql, token_end); + if (paren >= sql.size() || sql[paren] != '(') { + return false; + } + auto close = MatchingParen(sql, paren); + if (close == DConstants::INVALID_INDEX) { + return false; + } + alias_text = buffer; + end = close; + return true; + } + if (!StringUtil::CIEquals(token, "as") && !CanBeBareAlias(token)) { + return false; + } + buffer.append(sql, cursor, token_end - cursor); + cursor = token_end; + } + return false; +} + +// Words that close a FROM list, so a comma past them separates something other than relations. +static bool EndsFromList(const string &token) { + static const char *const TERMINATORS[] = {"where", "group", "having", "qualify", "window", "order", + "limit", "offset", "union", "except", "intersect", "select", + "values", "returning", "set", "insert", "update", "delete"}; + for (auto terminator : TERMINATORS) { + if (StringUtil::CIEquals(token, terminator)) { + return true; + } + } + return false; +} + +// Names a `WITH` clause binds in `sql`. A CTE reference is a name, not a scan, so it must never be +// handed a snapshot qualifier even when it shadows a pinned relation. +static case_insensitive_set_t CollectCteNames(const string &sql) { + case_insensitive_set_t names; + string candidate; + idx_t i = 0; + while (i < sql.size()) { + char c = sql[i]; + if (c == '\'') { + string ignored; + i = CopyQuotedRun(sql, i, ignored); + candidate.clear(); + continue; + } + if (c == '"' || c == '`') { + string quoted; + i = CopyQuotedRun(sql, i, quoted); + candidate = quoted.size() >= 2 ? quoted.substr(1, quoted.size() - 2) : quoted; + continue; + } + if (IsIdentifierStart(c)) { + idx_t end = i; + while (end < sql.size() && IsIdentifierPart(sql[end])) { + end++; + } + auto token = sql.substr(i, end - i); + i = end; + if (!StringUtil::CIEquals(token, "as")) { + candidate = token; + continue; + } + // `name AS (`, `name (columns) AS (` and `name AS [NOT] MATERIALIZED (` all define a CTE; + // nothing else puts a parenthesis directly behind `AS`. + idx_t cursor = SkipIgnorableSpan(sql, i); + for (idx_t modifiers = 0; modifiers < 2; modifiers++) { + idx_t keyword_end; + string keyword; + if (!TryReadIdentifierToken(sql, cursor, keyword_end, keyword) || + (!StringUtil::CIEquals(keyword, "not") && !StringUtil::CIEquals(keyword, "materialized"))) { + break; + } + cursor = SkipIgnorableSpan(sql, keyword_end); + } + if (cursor < sql.size() && sql[cursor] == '(' && !candidate.empty()) { + names.insert(candidate); + } + continue; + } + // A column-alias list sits between the CTE name and its `AS`, so parentheses keep the name. + if (c != '(' && c != ')' && !std::isspace(static_cast(c))) { + candidate.clear(); + } + i++; + } + return names; +} + string TimeTravelPins::RestoreIntoSql(const string &sql, SqlDialect dialect) const { if (pins.empty()) { return sql; } string result; result.reserve(sql.size()); - // Only a relation directly behind FROM or JOIN is a scan; anything else naming the relation is a - // column reference, a delta/metadata table or a literal, none of which take a pin. + // Only a relation directly behind FROM, JOIN or a FROM-list comma is a scan; anything else naming + // the relation is a column reference, a delta/metadata table or a literal, none of which take a + // pin. Each parenthesis nests its own FROM list so a subquery never leaks the enclosing one. bool expect_relation = false; + vector from_list_open; + from_list_open.push_back(false); + auto cte_names = CollectCteNames(sql); idx_t i = 0; while (i < sql.size()) { char c = sql[i]; @@ -573,30 +722,64 @@ string TimeTravelPins::RestoreIntoSql(const string &sql, SqlDialect dialect) con } if (IsIdentifierStart(c) || c == '"' || c == '`') { string final_component; - i = CopyQualifiedIdentifierChain(sql, i, result, final_component); + bool qualified; + i = CopyQualifiedIdentifierChain(sql, i, result, final_component, qualified); if (expect_relation) { + expect_relation = false; auto entry = pins.find(final_component); - if (entry != pins.end()) { - auto dialect_suffix = DialectPinSuffix(entry->second.suffix, dialect); - if (!CarriesQualifierAt(sql, i, dialect_suffix)) { - result += dialect_suffix; - OPENIVM_DEBUG_PRINT("[TIME TRAVEL] Restored pin '%s' onto rendered scan of '%s'\n", - dialect_suffix.c_str(), final_component.c_str()); - } + if (entry == pins.end()) { + continue; } - expect_relation = false; + auto dialect_suffix = DialectPinSuffix(entry->second.suffix, dialect); + if (CarriesQualifierAt(sql, i, dialect_suffix)) { + continue; + } + string alias_text; + idx_t raw_pin_end; + bool carries_raw_pin = TryReadRawPinAfterRelation(sql, i, alias_text, raw_pin_end); + if (!carries_raw_pin && !qualified && cte_names.find(final_component) != cte_names.end()) { + // A bare name this query itself binds: the scan it stands for was pinned where the + // CTE was defined, and only a real relation can carry a qualifier. + continue; + } + if (carries_raw_pin) { + i = raw_pin_end; + } + result += dialect_suffix; + result += alias_text; + OPENIVM_DEBUG_PRINT("[TIME TRAVEL] Restored pin '%s' onto rendered scan of '%s'\n", + dialect_suffix.c_str(), final_component.c_str()); continue; } - expect_relation = - StringUtil::CIEquals(final_component, "from") || StringUtil::CIEquals(final_component, "join"); + if (StringUtil::CIEquals(final_component, "from")) { + expect_relation = true; + from_list_open.back() = true; + } else if (StringUtil::CIEquals(final_component, "join")) { + expect_relation = true; + } else if (EndsFromList(final_component)) { + from_list_open.back() = false; + } continue; } result += c; - // A comma continues a FROM list; anything else that is not whitespace ends the table position. - if (c != ',' && !std::isspace(static_cast(c))) { + i++; + if (c == '(') { + from_list_open.push_back(false); + expect_relation = false; + } else if (c == ')') { + if (from_list_open.size() > 1) { + from_list_open.pop_back(); + } + expect_relation = false; + } else if (c == ',') { + // An implicit cross join: the next relation is a scan of its own and needs its own pin. + expect_relation = from_list_open.back(); + } else if (c == ';') { + from_list_open.assign(1, false); + expect_relation = false; + } else if (!std::isspace(static_cast(c))) { expect_relation = false; } - i++; } return result; } @@ -620,15 +803,6 @@ static string NormalizeQualifierText(const string &qualifier) { return normalized; } -// Words that may legally follow a table reference; none of them can be a bare alias. -static bool CanBeBareAlias(const string &token) { - // DuckDB's `alias_clause` takes a `ColId`: plain identifiers plus the unreserved and column-name - // keywords, but not the reserved or type/function ones. So `WHERE`, `JOIN` and `NATURAL` end the - // relation instead of naming it. - auto category = Parser::IsKeyword(StringUtil::Lower(token)); - return category != KeywordCategory::KEYWORD_RESERVED && category != KeywordCategory::KEYWORD_TYPE_FUNC; -} - // Map a Spark/Delta temporal keyword at `pos` to the DuckDB `AT (...)` parameter carrying the same // snapshot. `VERSION`/`SYSTEM_VERSION` pin a commit version, `TIMESTAMP`/`SYSTEM_TIME` a point in time. static bool ReadTemporalKeyword(const string &sql, idx_t pos, idx_t &end, string &at_parameter) { diff --git a/src/include/core/time_travel_pins.hpp b/src/include/core/time_travel_pins.hpp index 294de13b..250642b3 100644 --- a/src/include/core/time_travel_pins.hpp +++ b/src/include/core/time_travel_pins.hpp @@ -58,9 +58,11 @@ class TimeTravelPins { // in already-rendered `sql`. Refresh programs for several view shapes (min/max aggregates, // group recompute, interrupted-refresh recovery, ...) are assembled as SQL text rather than // through the AST, so `RestoreInto` never sees them; without this they would ship to the target - // engine reading the latest snapshot instead of the pinned one. Scans that already carry the - // qualifier are left alone, so it is safe to run over AST-rendered SQL as well. Throws through - // LPTS for dialects with no verified time-travel syntax rather than emitting an unpinned scan. + // engine reading the latest snapshot instead of the pinned one. A raw DuckDB `AT (...)` left on + // such a scan is replaced rather than duplicated, and scans that already carry the qualifier in + // the target spelling are left alone, so it is safe to run over AST-rendered SQL as well. Throws + // through LPTS for dialects with no verified time-travel syntax rather than emitting an unpinned + // scan. string RestoreIntoSql(const string &sql, SqlDialect dialect) const; private: diff --git a/src/upsert/refresh_sql.cpp b/src/upsert/refresh_sql.cpp index 1024f925..f140fc59 100644 --- a/src/upsert/refresh_sql.cpp +++ b/src/upsert/refresh_sql.cpp @@ -812,11 +812,15 @@ string GenerateRefreshSQL(ClientContext &context, const string &view_catalog_nam // their refresh program as SQL text instead, which the pins never reach — those would ship to // the target engine reading the latest snapshot rather than the pinned one. Every exit is // therefore finalized here: scans that already carry their qualifier are left untouched, and a - // dialect with no time-travel syntax still refuses through LPTS. + // dialect with no time-travel syntax still refuses through LPTS. DuckDB output runs against this + // catalog, which holds no snapshots, so there the pin is stripped instead of translated. auto finalize_refresh_sql = [&](string refresh_sql) { - if (view_time_travel_pins.Empty() || active_facts.target_dialect == SqlDialect::DUCKDB) { + if (view_time_travel_pins.Empty()) { return refresh_sql; } + if (active_facts.target_dialect == SqlDialect::DUCKDB) { + return view_time_travel_pins.StripFrom(refresh_sql); + } return view_time_travel_pins.RestoreIntoSql(refresh_sql, active_facts.target_dialect); }; RefreshType view_query_type = metadata.GetViewType(view_name); diff --git a/test/sql/time_travel.test b/test/sql/time_travel.test index 7fd96be6..61b90d4d 100644 --- a/test/sql/time_travel.test +++ b/test/sql/time_travel.test @@ -481,6 +481,188 @@ WHERE stmt_kind = 'data'; statement ok UPDATE openivm_views SET refresh_in_progress = false WHERE view_name = 'tt_join'; +# ========================================== +# Text-carried pins: the qualifier is translated in place, never duplicated +# ========================================== +# +# A body LPTS cannot serialize keeps its pin in the normalized text, where DuckDB spells it *after* +# the alias (`t p AT (VERSION => 366)`). Every other dialect wants the pin directly behind the +# relation, so the raw clause has to be replaced rather than left standing next to a translated one: +# `t VERSION AS OF 366 p AT (VERSION => 366)` is not valid anywhere. + +# An unpinned relation of its own, so the set operation that forces the fallback never scans a +# pinned relation unpinned (which would be refused as ambiguous instead). +statement ok +CREATE TABLE tt_regions(region VARCHAR); + +statement ok +INSERT INTO tt_regions VALUES ('us'), ('eu'); + +statement ok +CREATE MATERIALIZED VIEW tt_text_join AS + SELECT c.region AS region, SUM(o.amount) AS total + FROM tt_orders VERSION AS OF 366 o + JOIN tt_customers VERSION AS OF 12 c ON o.c_id = c.c_id + WHERE c.region IN (SELECT region FROM tt_regions EXCEPT SELECT region FROM tt_regions WHERE region = 'zz') + GROUP BY c.region; + +query II +SELECT region, total FROM tt_text_join ORDER BY region; +---- +eu 20 +us 18 + +# The stored SQL is the text shape: alias between the relation and its qualifier, one per relation. +query I +SELECT CASE WHEN sql_string LIKE '%tt_orders o AT (VERSION => 366)%' + AND sql_string LIKE '%tt_customers c AT (VERSION => 12)%' + THEN 1 ELSE 0 END +FROM openivm_views WHERE view_name = 'tt_text_join'; +---- +1 + +statement ok +CREATE MATERIALIZED VIEW tt_text_comma AS + SELECT c.region AS region, MIN(o.amount) AS lo, MAX(o.amount) AS hi + FROM tt_orders VERSION AS OF 366 o, tt_customers VERSION AS OF 12 c + WHERE o.c_id = c.c_id + AND c.region IN (SELECT region FROM tt_regions EXCEPT SELECT region FROM tt_regions WHERE region = 'zz') + GROUP BY c.region; + +query III +SELECT region, lo, hi FROM tt_text_comma ORDER BY region; +---- +eu 20 20 +us 3 10 + +statement ok +INSERT INTO tt_orders VALUES (6, 2, 4); + +# Ordinary incremental/group-recompute output for Spark: both relations pinned in Spark spelling, +# aliases intact, and no DuckDB qualifier left behind on either. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders% VERSION AS OF 366%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers% VERSION AS OF 12%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_comma', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# Compiling the same view twice must produce the same SQL: translating a pin is idempotent. +query I +SELECT CASE WHEN (SELECT string_agg(sql, ' ' ORDER BY stmt_order) + FROM openivm_compile_with_facts('tt_text_comma', '{"target_dialect":"spark","compile_only":true}') + WHERE stmt_kind = 'data') = + (SELECT string_agg(sql, ' ' ORDER BY stmt_order) + FROM openivm_compile_with_facts('tt_text_comma', '{"target_dialect":"spark","compile_only":true}') + WHERE stmt_kind = 'data') + THEN 1 ELSE 0 END; +---- +1 + +statement ok +UPDATE openivm_views SET refresh_in_progress = true WHERE view_name = 'tt_text_join'; + +statement ok +UPDATE openivm_views SET refresh_in_progress = true WHERE view_name = 'tt_text_comma'; + +# Interrupted-refresh recovery recomputes from the stored text, so this is where a raw qualifier +# would survive: an explicit JOIN keeps one pin per side... +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders VERSION AS OF 366 o%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers VERSION AS OF 12 c%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_join', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# ... and so does a comma FROM list, where every implicit cross join is a scan of its own. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders VERSION AS OF 366 o, %' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers VERSION AS OF 12 c %' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_comma', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# A dialect with no verified time-travel syntax still refuses rather than dropping the qualifier. +statement error +SELECT sql FROM openivm_compile_with_facts('tt_text_comma', '{"target_dialect":"postgres","compile_only":true}'); +---- +LPTS_UNSUPPORTED_TIME_TRAVEL + +# DuckDB output executes against this catalog, which holds no snapshots, so recovery drops the +# qualifier entirely instead of handing the binder syntax it cannot resolve. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%VERSION AS OF%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_join', '{"target_dialect":"duckdb","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +statement ok +PRAGMA refresh('tt_text_join'); + +statement ok +PRAGMA refresh('tt_text_comma'); + +query II +SELECT region, total FROM tt_text_join ORDER BY region; +---- +eu 24 +us 18 + +query III +SELECT region, lo, hi FROM tt_text_comma ORDER BY region; +---- +eu 4 20 +us 3 10 + +# A body whose CTE shadows the pinned relation still ends up with a pin on every scan of that +# relation and no raw qualifier anywhere: source qualification resolves the shadowed name against +# the catalog, and a name the query binds itself is never handed a qualifier it cannot carry. +statement ok +CREATE TABLE tt_spare(c_id INT, amount INT); + +statement ok +CREATE MATERIALIZED VIEW tt_text_shadow AS + WITH tt_orders AS ( + SELECT c_id, amount FROM tt_orders VERSION AS OF 366 + EXCEPT ALL + SELECT c_id, amount FROM tt_spare + ) + SELECT c_id, SUM(amount) AS total FROM tt_orders GROUP BY c_id; + +statement ok +UPDATE openivm_views SET refresh_in_progress = true WHERE view_name = 'tt_text_shadow'; + +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders VERSION AS OF 366%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%VERSION AS OF 366 VERSION AS OF%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_shadow', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +statement ok +PRAGMA refresh('tt_text_shadow'); + +query II +SELECT c_id, total FROM tt_text_shadow ORDER BY c_id; +---- +1 18 +2 24 + # ========================================== # Unpinned views are untouched by the Spark input dialect # ========================================== From 99e24388492b9b36ef3e7c805deae5e23ba3cdd8 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Wed, 26 Aug 2026 02:34:50 +0000 Subject: [PATCH 15/18] fix: keep a parenthesized join list in table position A parenthesis in table position opens either a derived table or a join list. Treating every one as a derived table dropped the relation expectation, so the first scan of `FROM (a JOIN b)` kept its DuckDB `AT (...)` spelling when the refresh SQL was assembled as text for another dialect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/core/time_travel_pins.cpp | 28 +++++++- test/sql/time_travel.test | 119 ++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 2 deletions(-) diff --git a/src/core/time_travel_pins.cpp b/src/core/time_travel_pins.cpp index 77ad2d1a..bb0f6983 100644 --- a/src/core/time_travel_pins.cpp +++ b/src/core/time_travel_pins.cpp @@ -629,6 +629,25 @@ static bool EndsFromList(const string &token) { return false; } +// Whether the parenthesis that ends at `pos` opens a derived table rather than a parenthesized join +// list. `(SELECT ...)`, `(WITH ...)`, `(VALUES ...)`, `(TABLE t)` and DuckDB's `(FROM t ...)` all +// start a query of their own; anything else in table position is a relation, and a nested +// parenthesis just defers the question one level. +static bool OpensDerivedTable(const string &sql, idx_t pos) { + idx_t cursor = SkipIgnorableSpan(sql, pos); + while (cursor < sql.size() && sql[cursor] == '(') { + cursor = SkipIgnorableSpan(sql, cursor + 1); + } + idx_t token_end; + string token; + if (!TryReadIdentifierToken(sql, cursor, token_end, token)) { + return false; + } + return StringUtil::CIEquals(token, "select") || StringUtil::CIEquals(token, "with") || + StringUtil::CIEquals(token, "values") || StringUtil::CIEquals(token, "table") || + StringUtil::CIEquals(token, "from"); +} + // Names a `WITH` clause binds in `sql`. A CTE reference is a name, not a scan, so it must never be // handed a snapshot qualifier even when it shadows a pinned relation. static case_insensitive_set_t CollectCteNames(const string &sql) { @@ -764,8 +783,13 @@ string TimeTravelPins::RestoreIntoSql(const string &sql, SqlDialect dialect) con result += c; i++; if (c == '(') { - from_list_open.push_back(false); - expect_relation = false; + // In table position a parenthesis opens either a derived table, which starts its own + // query, or a parenthesized join list, whose first element is still a scan that needs its + // pin. Only the query keywords tell the two apart; a table function's argument list never + // reaches here because its own name already consumed the table position. + bool table_list = expect_relation && !OpensDerivedTable(sql, i); + from_list_open.push_back(table_list); + expect_relation = table_list; } else if (c == ')') { if (from_list_open.size() > 1) { from_list_open.pop_back(); diff --git a/test/sql/time_travel.test b/test/sql/time_travel.test index 61b90d4d..af7ca335 100644 --- a/test/sql/time_travel.test +++ b/test/sql/time_travel.test @@ -663,6 +663,125 @@ SELECT c_id, total FROM tt_text_shadow ORDER BY c_id; 1 18 2 24 +# A parenthesized join list is still table position: its first element is a scan and needs its pin, +# where a derived table starts a query of its own and takes none. +statement ok +CREATE MATERIALIZED VIEW tt_text_paren AS + SELECT c.region AS region, SUM(o.amount) AS total + FROM (tt_orders VERSION AS OF 366 o JOIN tt_customers VERSION AS OF 12 c ON o.c_id = c.c_id) + WHERE c.region IN (SELECT region FROM tt_regions EXCEPT SELECT region FROM tt_regions WHERE region = 'zz') + GROUP BY c.region; + +query II +SELECT region, total FROM tt_text_paren ORDER BY region; +---- +eu 24 +us 18 + +statement ok +CREATE MATERIALIZED VIEW tt_text_paren_nested AS + SELECT c.region AS region, MIN(o.amount) AS lo, MAX(o.amount) AS hi + FROM ((tt_orders VERSION AS OF 366 o JOIN tt_customers VERSION AS OF 12 c ON o.c_id = c.c_id)) + WHERE c.region IN (SELECT region FROM tt_regions EXCEPT SELECT region FROM tt_regions WHERE region = 'zz') + GROUP BY c.region; + +query III +SELECT region, lo, hi FROM tt_text_paren_nested ORDER BY region; +---- +eu 4 20 +us 3 10 + +# A derived table is not a relation, so the pin belongs to the scan inside it and to nothing else. +statement ok +CREATE MATERIALIZED VIEW tt_text_derived AS + SELECT d.c_id AS c_id, SUM(d.amount) AS total + FROM (SELECT c_id, amount FROM tt_orders VERSION AS OF 366 + EXCEPT ALL + SELECT c_id, amount FROM tt_spare) d + GROUP BY d.c_id; + +query II +SELECT c_id, total FROM tt_text_derived ORDER BY c_id; +---- +1 18 +2 24 + +statement ok +INSERT INTO tt_orders VALUES (7, 1, 2); + +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders% VERSION AS OF 366%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers% VERSION AS OF 12%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_paren', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +statement ok +UPDATE openivm_views SET refresh_in_progress = true WHERE view_name = 'tt_text_paren'; + +statement ok +UPDATE openivm_views SET refresh_in_progress = true WHERE view_name = 'tt_text_paren_nested'; + +statement ok +UPDATE openivm_views SET refresh_in_progress = true WHERE view_name = 'tt_text_derived'; + +# Recovery recomputes from the stored text, where the parenthesized list is written out verbatim. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%(tt_orders VERSION AS OF 366 o join%' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers VERSION AS OF 12 c %' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_paren', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# Nested parentheses defer the question one level, so the innermost element is still a scan. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders VERSION AS OF 366 o %' + AND string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_customers VERSION AS OF 12 c%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_paren_nested', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +# The derived table keeps exactly one pin - on the scan inside it, never on the subquery itself. +query I +SELECT CASE WHEN string_agg(sql, ' ' ORDER BY stmt_order) LIKE '%tt_orders VERSION AS OF 366%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%) VERSION AS OF%' + AND string_agg(sql, ' ' ORDER BY stmt_order) NOT LIKE '%AT (VERSION%' + THEN 1 ELSE 0 END +FROM openivm_compile_with_facts('tt_text_derived', '{"target_dialect":"spark","compile_only":true}') +WHERE stmt_kind = 'data'; +---- +1 + +statement ok +PRAGMA refresh('tt_text_paren'); + +statement ok +PRAGMA refresh('tt_text_paren_nested'); + +statement ok +PRAGMA refresh('tt_text_derived'); + +query II +SELECT region, total FROM tt_text_paren ORDER BY region; +---- +eu 24 +us 20 + +query II +SELECT c_id, total FROM tt_text_derived ORDER BY c_id; +---- +1 20 +2 24 + # ========================================== # Unpinned views are untouched by the Spark input dialect # ========================================== From d07fe161fdf1954cbb789ab824f59339366798cc Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sun, 30 Aug 2026 14:13:18 +0000 Subject: [PATCH 16/18] chore: pin third_party/lpts to final PR #18 head (6980a13) Update LPTS submodule gitlink from dbac36d to 6980a13 which includes the sqllogictest separator fix for the Spark dialect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- third_party/lpts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/lpts b/third_party/lpts index dbac36de..6980a13b 160000 --- a/third_party/lpts +++ b/third_party/lpts @@ -1 +1 @@ -Subproject commit dbac36de2e7e00d22fcb48250662fd69c0c106cd +Subproject commit 6980a13bedcef63e751087dcc25cac1a0db9a635 From 22c03e5809e48d108921736c987c4b2cd1cf5072 Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sun, 30 Aug 2026 16:34:27 +0000 Subject: [PATCH 17/18] =?UTF-8?q?fix:=20update=20stale=20auto=5Frefresh=20?= =?UTF-8?q?profile=20statement=20count=20expectation=20(6=E2=86=927)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AGGREGATE_GROUP MERGE refresh path produces 7 profiled statements: 1. SET refresh_in_progress = true 2. INSERT INTO openivm_delta_ (delta computation) 3. MERGE INTO openivm_data_ (applies delta) 4. DELETE FROM openivm_delta_ (view delta cleanup) 5. DELETE FROM openivm_delta_ (source delta cleanup) 6. UPDATE openivm_delta_tables (metadata timestamp) 7. SET refresh_in_progress = false The expectation of 6 was stale since the transactional lifecycle hardening added the 7th statement. All statements are distinct and required — no duplicate. The old comment incorrectly referenced the "full-recompute path" but this view uses the MERGE path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/sql/auto_refresh.test | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/sql/auto_refresh.test b/test/sql/auto_refresh.test index 30490eb7..8ca7dc12 100644 --- a/test/sql/auto_refresh.test +++ b/test/sql/auto_refresh.test @@ -895,15 +895,18 @@ SELECT count(*) FROM openivm_refresh_profile WHERE view_name = 'mv_lt' AND step_ ---- 3 -# 6, not 4: the full-recompute path for AGGREGATE_GROUP views emits four statements -# (CREATE TEMP TABLE / DELETE vanished keys / INSERT OR REPLACE / DROP) instead of a plain -# DELETE + INSERT, because their data table carries a UNIQUE index and DuckDB's on-disk unique -# index rejects re-inserting a key deleted earlier in the same transaction. See -# test/sql/group_recompute_persistent_unique_index.test. +# 7 profiled statements per refresh for an AGGREGATE_GROUP MERGE view: +# 1. SET refresh_in_progress = true +# 2. INSERT INTO openivm_delta_ (delta computation) +# 3. MERGE INTO openivm_data_ (applies delta via CTE from the delta view) +# 4. DELETE FROM openivm_delta_ (view delta cleanup) +# 5. DELETE FROM openivm_delta_ (source delta cleanup) +# 6. UPDATE openivm_delta_tables SET last_update (metadata timestamp) +# 7. SET refresh_in_progress = false query I SELECT count(*) FROM openivm_refresh_profile WHERE view_name = 'mv_lt' AND step_name = 'execute_refresh_sql_stmt'; ---- -6 +7 query I SELECT CASE WHEN count(*) >= 13 THEN 1 ELSE 0 END FROM openivm_refresh_profile WHERE view_name = 'mv_lt' AND duration_ms >= 0; From c5f0845f499c1a86194d30056bcd808ebdbe97ef Mon Sep 17 00:00:00 2001 From: Raki Rahman Date: Sun, 30 Aug 2026 19:06:21 +0000 Subject: [PATCH 18/18] fix: make profiling test deterministic by disabling adaptive refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The profiling statement-count assertion was flaky because Test 18 left openivm_adaptive_refresh = true. After 3+ history entries, the adaptive cost model could nondeterministically choose full recompute (6 stmts) vs incremental MERGE (7 stmts) depending on prior refresh durations. Fix: bracket the profiled refresh with SET openivm_adaptive_refresh = false / true, so the assertion always sees the 7-statement incremental path. Re-enable immediately after so subsequent refreshes still record history for Test 20 (expected count 6→5 since the profiled refresh no longer records a history entry). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/sql/auto_refresh.test | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/sql/auto_refresh.test b/test/sql/auto_refresh.test index 8ca7dc12..7eb598c0 100644 --- a/test/sql/auto_refresh.test +++ b/test/sql/auto_refresh.test @@ -881,6 +881,14 @@ SELECT count(*) FROM openivm_refresh_profile WHERE view_name = 'mv_lt'; ---- 0 +# Force deterministic incremental path for the profiling assertions below. +# The adaptive cost model (still enabled from Test 18 setup) can nondeterministically +# choose full recompute vs incremental depending on prior refresh durations, which +# changes the statement count (6 vs 7). The profiling feature is path-independent; +# the adaptive cost model is already validated by the history assertions above. +statement ok +SET openivm_adaptive_refresh = false; + statement ok SET openivm_profile_refresh = true; @@ -908,6 +916,10 @@ SELECT count(*) FROM openivm_refresh_profile WHERE view_name = 'mv_lt' AND step_ ---- 7 +# Re-enable adaptive refresh so subsequent refreshes record history for Test 20. +statement ok +SET openivm_adaptive_refresh = true; + query I SELECT CASE WHEN count(*) >= 13 THEN 1 ELSE 0 END FROM openivm_refresh_profile WHERE view_name = 'mv_lt' AND duration_ms >= 0; ---- @@ -1013,10 +1025,13 @@ SET openivm_skip_empty_deltas = true; # Test 20: Learned cost model — history cleanup on REPLACE # ========================================== +# 5 history rows: 3 from Test 18 setup + 2 from profiling retention tests. +# The profiled refresh itself ran with openivm_adaptive_refresh = false (for deterministic +# statement count), so it did not record a history entry. query I SELECT count(*) FROM openivm_refresh_history WHERE view_name = 'mv_lt'; ---- -6 +5 statement ok CREATE OR REPLACE MATERIALIZED VIEW mv_lt AS SELECT grp, sum(val) as total FROM lt GROUP BY grp;