feat: native Spark add_months scalar function - #10
Draft
mdrakiburrahman wants to merge 11 commits into
Draft
Conversation
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>
mdrakiburrahman
added a commit
to mdrakiburrahman/openivm-spark
that referenced
this pull request
Aug 16, 2026
Points OPENIVM_REPO/BRANCH/COMMIT at mdrakiburrahman/openivm@dev/mdrrahman/add-months-ila (6c626fd), which is current ila/openivm:main (b918351) + the native Spark add_months scalar function (openivm PR ila/openivm#10). Using add_months rebased onto recent ila keeps the openivm native refresh behavior identical to the base branch's pin (openivm@b918351) while adding add_months, so the Spark IVM suite stays green. Revert to ila/openivm:main once add_months lands upstream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Validated against the openivm CI harness (
Branch is 1 commit ahead / 0 behind |
Contributor
Author
|
Also green: tidy-check (clang-tidy) PASS on |
Contributor
Author
|
After benchmark runs green, move as much as possible over to lpts pin bump |
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>
Contributor
Author
|
Restructured per review: This PR is now minimal — implementation moved out, consumed via the pin:
Net: +13 / −125. Local openivm CI ( Pairing / draft status: blocked on cwida/lpts#18. The submodule pin |
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>
Contributor
Author
|
Bumped Note: 592d469 currently lives on the |
mdrakiburrahman
added a commit
to mdrakiburrahman/openivm-spark
that referenced
this pull request
Aug 16, 2026
OPENIVM_COMMIT -> 1220ba4 (ila/openivm#10 head: sources add_months from lpts + bumps third_party/lpts) and LPTS_COMMIT -> 592d469 (cwida/lpts#18 head: native add_months + SPARK dialect icu test guard). Both upstream PRs are green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ila
pushed a commit
to mdrakiburrahman/openivm-spark
that referenced
this pull request
Aug 17, 2026
Points OPENIVM_REPO/BRANCH/COMMIT at mdrakiburrahman/openivm@dev/mdrrahman/add-months-ila (6c626fd), which is current ila/openivm:main (b918351) + the native Spark add_months scalar function (openivm PR ila/openivm#10). Using add_months rebased onto recent ila keeps the openivm native refresh behavior identical to the base branch's pin (openivm@b918351) while adding add_months, so the Spark IVM suite stays green. Revert to ila/openivm:main once add_months lands upstream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ila
pushed a commit
to mdrakiburrahman/openivm-spark
that referenced
this pull request
Aug 17, 2026
OPENIVM_COMMIT -> 1220ba4 (ila/openivm#10 head: sources add_months from lpts + bumps third_party/lpts) and LPTS_COMMIT -> 592d469 (cwida/lpts#18 head: native add_months + SPARK dialect icu test guard). Both upstream PRs are green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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>
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>
…ution 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>
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>
…UTE recomputes
A 32-way local OpenIVM Spark canary demoted 36 of 58 incremental
materialized views to FULL_REFRESH with reason
`non_cascade_upstream:non_cascade:<parents>`; 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_<view>` 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_<view>` / `openivm_new_<view>` 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a native
add_months(date, num_months)Spark-compatible scalar function to the openivm DuckDB extension, so IVM compilation can lower Sparkadd_months(...)natively instead of falling back toFULL_REFRESH.Branch is
main+ a single add-only commit (rebased directly onto currentmain).Changes
src/functions/spark_scalar_functions.{cpp,hpp}— implementadd_months(month arithmetic with end-of-month clamping, negatives, NULL propagation).src/openivm_extension.cpp+CMakeLists.txt— register/build it.test/sql/spark_add_months.test— 22 assertions covering month add/subtract, end-of-month clamp, leap years, negatives, NULLs.Local test results (
GEN=ninja make && make test, DuckDB v1.5.4 pins frommain)spark_add_months.test: 22/22 assertions pass.test/sql/auto_refresh.test:903(AGGREGATE_GROUP full-recompute emits 7execute_refresh_sql_stmtsteps, test expects 6) — reproduces identically on pristinemainwithout this change, so it is a pre-existing issue independent ofadd_months.Opened as draft.