Skip to content

feat: native Spark add_months scalar function - #18

Draft
mdrakiburrahman wants to merge 8 commits into
cwida:mainfrom
mdrakiburrahman:dev/mdrrahman/spark-add-months
Draft

feat: native Spark add_months scalar function#18
mdrakiburrahman wants to merge 8 commits into
cwida:mainfrom
mdrakiburrahman:dev/mdrrahman/spark-add-months

Conversation

@mdrakiburrahman

@mdrakiburrahman mdrakiburrahman commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a native Spark-compatible add_months(DATE, INTEGER) -> DATE scalar function to lpts.

DuckDB has no add_months. When Spark SQL containing add_months(...) is translated
through lpts (and executed by consumers such as openivm),
the missing function caused compile/binding failures. Registering it in lpts —
the Spark-compat layer — means every consumer that loads lpts (or compiles its
sources) gets correct add_months for free, rather than each re-implementing it.

Semantics

Full Spark add_months semantics:

  • day-of-month preserved;
  • clamped to the target month's length when the source day overflows;
  • end-of-month rule: if the source date is the last day of its month, the
    result is the last day of the target month.

Leap-year-correct via the DuckDB Date API. NULL propagates. Works for negative
offsets and multi-year rollovers.

Changes

  • src/spark_scalar_functions.cpp / src/include/spark_scalar_functions.hpp — implementation + RegisterSparkScalarFunctions(loader).
  • src/lpts_extension.cpp — call RegisterSparkScalarFunctions(loader) from LoadInternal.
  • CMakeLists.txt — add the new source to EXTENSION_SOURCES.
  • test/sql/spark_add_months.test — scalar-correctness coverage (month-end clamping, leap years, negative offsets, year rollover, NULL).

Local CI (tools/lpts/ci/run-all.sh, duckdb v1.5.3 / extension-ci-tools v1.5-variegata)

Leg Result
format-check PASS
tidy-check PASS
build (vcpkg + ninja, linux_amd64) PASS
test (sqllogictest) PASS — 2181 assertions / 46 test cases (incl. spark_add_months.test)

Draft pending the paired openivm pin bump.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

@mdrakiburrahman
feat: native Spark add_months scalar function
642c762 
DuckDB lacks add_months, which appears in Spark SQL translated through lpts.
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 the
DuckDB Date API for leap-year-correct month lengths.

Registered in LoadInternal so any consumer loading lpts (or compiling its
sources) gets the function. Adds test/sql/spark_add_months.test covering scalar
correctness across month-end clamping, leap years, negative offsets, year
rollover, and NULL propagation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mdrakiburrahman added a commit to mdrakiburrahman/openivm that referenced this pull request Aug 16, 2026
@mdrakiburrahman
refactor: source add_months from lpts instead of a native openivm copy
c723a24 
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>
@mdrakiburrahman mdrakiburrahman mentioned this pull request Aug 16, 2026
Draft
@mdrakiburrahman
test: require icu for SPARK dialect timestamptz assertions
592d469 
CURRENT_TIMESTAMP folds to a TIMESTAMPTZ constant during lpts_query binding,
which autoloads icu. Offline linux_amd64 CI cannot fetch icu and failed the
whole file; `require icu` makes those runners skip instead (arm64 still runs it
fully). Fixes the pre-existing dialect_spark.test amd64 failure surfaced on this
add_months PR via the merge with main.

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
@mdrakiburrahman
build: bump openivm/lpts pins to add_months-relocation heads
6228f95 
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
@mdrakiburrahman ila
build: bump openivm/lpts pins to add_months-relocation heads
a8df09d 
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>
@mdrakiburrahman mdrakiburrahman mentioned this pull request Aug 22, 2026
Draft
mdrakiburrahman and others added 6 commits August 22, 2026 20:06
@mdrakiburrahman
Fix set-op column binding remaps
77ed5e5 
Restore the lhs column_map before traversing each set-op sibling so later UNION/EXCEPT/INTERSECT branches cannot leak bindings into the parent. Also resolve projection refs above set-ops against the set-op output binding when a rewrite leaves a stale child binding in place.

Add focused regressions for downstream joins over N-ary UNION ALL and for duplicated UNION ALL key projections.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mdrakiburrahman
Fix bounded Spark HUGEINT and UNION aliases
754c797 
Map only provably bounded HUGEINT values to Spark DECIMAL(38,0), and retain trailing rewritten UNION bindings as aliases of their physical multiplicity output.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mdrakiburrahman
Merge upstream main into dev/mdrrahman/spark-add-months
b3baf0b 
Resolve Spark/Postgres merge conflicts while preserving the feature branch's Spark add_months, set-op, and bounded HUGEINT work.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot CLI
Represent Spark/Delta time-travel pins instead of failing to parse them
66bf3ae 
A Spark/Delta temporal clause (`FROM t VERSION AS OF 366`) reached DuckDB's
parser verbatim, so any pinned relation died with
`Parser Error: syntax error at or near "as"` before a plan was ever built.
Downstream that surfaced as a silent correctness fallback: OpenIVM's
`openivm_compile_with_facts` produced no result, the refresh span reported
`compile_refresh_type=COMPILE_FAILED effective_refresh_type=FULL_REFRESH
reason=compile_failed`, and the pinned snapshot the user asked for was lost.

Input side (`lpts_input_dialect = 'spark'`): the temporal clause now normalizes
to DuckDB's semantically equivalent `AT (...)` clause —
`[FOR] VERSION|SYSTEM_VERSION AS OF <n>` becomes `AT (VERSION => <n>)` and
`[FOR] TIMESTAMP|SYSTEM_TIME AS OF '<ts>'` becomes `AT (TIMESTAMP => '<ts>')`.
The pin is represented, never dropped: dropping it would silently promote every
pinned scan to "read latest" and change the meaning of the query. A match
requires the full `<keyword> AS OF <literal>` sequence, so a column or alias
merely named `version`/`timestamp` is untouched, as are string literals. A
timestamp pin given a bare number is refused rather than mis-pinned.

Output side: LPTS already carried a pin as a DuckDB `AT (...)` suffix on the
table name (DuckLake time travel), but emitted that DuckDB spelling into every
dialect. The suffix is now rendered per dialect — Spark gets
`VERSION AS OF <n>` / `TIMESTAMP AS OF '<ts>'`, DuckDB keeps `AT (...)`, and a
dialect with no verified time-travel syntax raises
`LPTS_UNSUPPORTED_TIME_TRAVEL` instead of emitting SQL the target cannot parse.
Unqualified renderings (Postgres/Redshift/Feldera, inline SQL) went through a
raw `table_name` path that also leaked the suffix and then mistook its
parentheses for a table-function argument list (`products AT (VERSION => 2)
_tf("1")`); those paths now split the pin off explicitly.

`test/sql/time_travel.test` pins the contract end to end: normalization
fixtures for both temporal forms and the negative cases, and a DuckLake table
whose version 2 holds 2 rows while the latest holds 3, so a dropped pin is
observable in the result under `lpts_check`, not just in the generated SQL.

Note for the pinned-scan compile path: a plain DuckDB catalog now answers
`Binder Error: Catalog type does not support time travel` instead of a parser
error. That is the honest outcome — LPTS represents the pin and the catalog
decides — and it is the downstream signal OpenIVM must handle when it registers
schema-only fact tables.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mdrakiburrahman
Move a pinned relation's alias ahead of the DuckDB AT (...) clause
dbac36d 
Spark spells a time-travel pin between the relation and its alias
(`FROM t VERSION AS OF 2 p`), DuckDB spells it after the alias
(`FROM t p AT (VERSION => 2)`). The Spark input normalizer emitted the
qualifier in Spark's position, so every aliased pinned relation produced
`FROM t AT (VERSION => 2) p`, which DuckDB refuses to parse -- the same
class of failure the normalizer was added to remove.

The normalizer now reads the optional `[AS] alias [(column, ...)]` that
follows the temporal clause and emits it ahead of the rewritten
qualifier, preserving the original quoting and case. A bare token is
only taken as an alias when DuckDB's grammar allows it there (the
`ColId` rule: reserved and type/function keywords are excluded), so
`WHERE`, `GROUP BY`, `CROSS`/`NATURAL`/`ANTI JOIN`, a comma and a
closing paren continue the query instead of being swallowed.

test/sql/time_travel.test replaces the text-only expectation that
asserted the unparseable form with executable DuckLake coverage: real
pinned scans with a bare alias, an `AS` alias, a quoted alias, a column
alias list, alias-qualified projections and filters, and two aliased
relations pinned to two different non-latest versions -- all run under
`lpts_check`, so the regenerated SQL is executed and its rows compared.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mdrakiburrahman
Fix Spark dialect sqllogictest separation
6980a13 
Separate the HUGEINT error expectation from the following interval query so the ICU-enabled runner parses them as distinct records.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mdrakiburrahman