fix(cubesql): Support DATE_ADD SQL pushdown

[MazterQyou]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Description of Changes Made

This PR adds a functions/DATE_ADD SQL template to the Postgres, Snowflake, Redshift, MSSQL, MySQL, ClickHouse, Presto/Trino, Databricks, and DuckDB dialects so that expressions using DATEADD can be pushed down instead of falling back to post-processing. Related test is included.

[claude]

Claude finished @MazterQyou's task in 3m 33s —— View job

---

Review: templates look right for MONTH/DAY; sub-day units (MILLISECOND/NANOSECOND) are the risk — 2 medium, 3 low, posted inline

Full review

• Gather context (PR diff)
• Inspect cubesql DATE_ADD rewrite / template variable semantics
• Review each dialect template for correctness
• Review test coverage
• Post review

What I checked

SqlTemplates::scalar_function (rust/cubesql/cubesql/src/transport/service.rs:657) passes args, date_part and interval into functions/DATE_ADD. For date_add, args[0] is the timestamp expression and args[1] is the interval literal; date_part and interval are both derived from that literal in wrapper.rs:3151-3234. Argument order and variable usage are correct in all nine new templates, and each matches its dialect's native signature:

Dialect	Native signature	Template	Verdict
Postgres / DuckDB	ts + interval	({{ args[0] }} + '{{ interval }} {{ date_part }}'::interval)	✅
MySQL	DATE_ADD(date, INTERVAL n unit)	matches	✅ order, ⚠️ units
MSSQL / Snowflake / Redshift	DATEADD(part, n, date)	matches	✅
Presto/Trino	date_add(unit, value, ts)	matches (unit strings are case-insensitive)	✅
ClickHouse	addDate(date, INTERVAL n unit)	matches	✅, ⚠️ 23.9+
Databricks	ts + INTERVAL '{n}' unit	matches ANSI form	⚠️ units

The Postgres change also correctly un-comments as DATE_ADD with args[0] (the old commented-out DATEADD variant used args[2], which is the right index for the 3-arg dateadd/datediff shape but wrong here). interval is always an integer stringified from a literal and date_part for date_add comes from a closed set, so there's no injection surface introduced.

Findings

Medium

1. MySQL cannot express MILLISECOND (MysqlQuery.ts:191). Sub-day intervals are emitted as MILLISECOND, so INTERVAL '2 hours' renders INTERVAL 7200000 MILLISECOND → MySQL syntax error, replacing today's silent fallback to post-processing with a hard query failure. Suggested unit mapping inline.
2. Databricks quoted ANSI interval rejects MILLISECOND/NANOSECOND (DatabricksQuery.ts:179). INTERVAL '{{ interval }}' {{ date_part }} is the typed form, limited to YEAR…SECOND; the unquoted multi-unit form is broader. Suggestion inline.

Low
3. NANOSECOND is unrepresentable in Postgres, DuckDB and Trino (PostgresQuery.ts:91). If the IntervalMonthDayNano-with-nanos branch is reachable, normalizing nanos → micros/seconds on the Rust side fixes all dialects at once instead of nine independent guards. Also note this template is inherited by CrateQuery, FireboltQuery and QuestQuery.
4. ClickHouse addDate requires 23.9+ (ClickHouseQuery.ts:269); the + INTERVAL operator form is version-agnostic.
5. The new Rust test doesn't guard this PR (test_wrapper.rs:2891-2897). functions/DATE_ADD is already defined in the test transport on master (compile/test/mod.rs:699) as DATE_ADD({{ args_concat }}), so both assertions pass on origin/master unchanged — and because the fixture is args_concat-based, it never exercises date_part/interval at all. An arg-order or variable-name typo in any of the nine templates would still pass. compile/mod.rs:15371 shows the existing pattern for overriding the template with a realistic dialect body.

Notes

• No docs update needed as far as I can tell — DATEADD pushdown isn't enumerated per-dialect in /docs-mintlify.
• I did not run the Rust test suite or yarn test (cubesql build cost); the review is static.
• No prior review threads existed on this PR, so nothing to resolve or dedupe against.

· branch `cubesql/date-add-sql-pushdown`

[claude]

NANOSECOND is a unit the generator can emit (the IntervalMonthDayNano branch in wrapper.rs:3168-3181), and Postgres has no nanosecond interval unit — '7200000000000 NANOSECOND'::interval errors out. Same for DuckDB (microsecond granularity) and Trino.

Worth checking whether that branch is reachable for the interval literals DataFusion actually produces here; if it is, converting nanos to microseconds/seconds on the Rust side (before it reaches any dialect template) would fix all nine dialects at once rather than each template guarding independently. Note this also inherits into CrateQuery/FireboltQuery/QuestQuery, which extend PostgresQuery.

[codecov]

Codecov Report

❌ Patch coverage is 96.89922% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.62%. Comparing base (33c8828) to head (f01a2b1).

Files with missing lines	Patch %	Lines
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs	96.72%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11539       +/-   ##
===========================================
+ Coverage   59.35%   79.62%   +20.26%     
===========================================
  Files         223      480      +257     
  Lines       17989    99700    +81711     
  Branches     3641     3641               
===========================================
+ Hits        10677    79382    +68705     
- Misses       6793    19799    +13006     
  Partials      519      519               

Flag	Coverage Δ
cube-backend	59.36% <100.00%> (+0.01%)	⬆️
cubesql	84.08% <96.72%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @MazterQyou's task in 2m 55s —— View job

---

Re-review after force-push: all four actionable findings addressed; one new finding on negative sub-day intervals (1 medium, 1 low carried over)

Full review

• Gather context (new diff vs origin/master)
• Diff against my previous review — check which findings still apply
• Re-check Rust-side date_part / interval derivation
• Resolve stale threads (4 resolved) / post new findings (1 inline)
• Post review

What changed since the last review

Prior finding	Status
MySQL can't express MILLISECOND	✅ Fixed — {% if date_part == "MILLISECOND" %}{{ interval }}000 MICROSECOND
Databricks quoted ANSI interval rejects MILLISECOND	✅ Fixed — switched to unquoted multi-unit form
ClickHouse addDate needs 23.9+	✅ Fixed — switched to + INTERVAL operator form
Rust test didn't guard the change	✅ Fixed — new test_wrapper_date_add_dialect_templates renders each real dialect template through the pushdown and asserts the emitted SQL for all three units
NANOSECOND unrepresentable in Postgres/DuckDB/Trino	⏳ Still open (thread left unresolved)

Each fix also carries a comment explaining why the non-obvious form was chosen, which is the right call — those templates are otherwise unreadable without the Rust context. I resolved the four stale threads; the NANOSECOND one stays open.

The new test is the substantive improvement: it's table-driven over the five distinct template shapes × the three units the rewrite can emit (MILLISECOND / DAY / MONTH), uses a per-group-aggregate filter so the expression genuinely has to go through the template rather than becoming a Cube filter, and the comments map each shape back to the query classes it covers. I spot-checked every expected string against the derivation in wrapper.rs:3151-3234 and they're right — including the MySQL 7200000 ms → 7200000000 µs concatenation.

New finding (medium) — negative sub-day intervals

Posted inline on test_wrapper.rs:2965. Every matrix case uses a positive interval (2), which is the one regime where the sub-day encoding works. transform_interval_parts_to_interval (rust/cubesql/cubesql/src/compile/rewrite/rules/dates.rs:539-541) builds IntervalDayTime(3_600_000 * n), but IntervalDayTime is a packed (days: i32, millis: i32) pair:

• DATEADD('hour', -2, …) → -7_200_000i64 → days = -1, ms = -7_200_000 → wrapper.rs returns Unsupported mixed IntervalDayTime.
• DATEADD('hour', 700, …) → 2_520_000_000 > i32::MAX → wraps to a negative ms, so the pushed-down SQL gets the wrong interval silently.

Not introduced by this PR — the same literal is what DataFusion evaluates in the post-processing path today — but this test is the natural place to catch it, and negative offsets are the common real-world shape. Suggested construction-site fix is in the inline comment.

Remaining low-severity notes

• NANOSECOND still unhandled (existing open thread on PostgresQuery.ts:91). The IntervalMonthDayNano branch at wrapper.rs:3168-3181 can emit it; Postgres/DuckDB/Trino/MySQL have no such unit. Note the new dialect test also covers only the three units the dateadd rewrite produces, so a date_add reaching the wrapper from another path with a nanosecond literal is untested. Normalizing nanos → micros on the Rust side would cover all dialects at once.
• Nit: MySQL's date_part == "MILLISECOND" is case-sensitive. It's correct as written (the date_add branch always uppercases), but nearby templates use |upper — worth matching for consistency.

Notes

• Static review only; I did not build cubesql or run cargo test / yarn test.
• Codecov's 3 uncovered lines are the sql_push_down_enabled() early-return guards — not a real gap.
• No docs change needed; DATEADD pushdown isn't enumerated per-dialect in /docs-mintlify.

· branch `cubesql/date-add-sql-pushdown`

[claude]

This matrix is a big improvement over the args_concat fixture — it genuinely pins arg order and both context variables per dialect. One gap: every case uses a positive interval (2), which is exactly where the sub-day encoding happens to work.

DATEADD('hour', -2, ...) goes through redshift-dateadd-to-interval → transform_interval_parts_to_interval (rust/cubesql/cubesql/src/compile/rewrite/rules/dates.rs:539-541), which builds the literal as IntervalDayTime(3_600_000 * n). IntervalDayTime is a packed (days: i32, millis: i32) pair, not a scalar, so a plain multiply only round-trips for non-negative values that fit in 31 bits:

• n = -2 → -7_200_000i64 = 0xFFFF_FFFF_FF91_7A00 → days = -1, ms = -7_200_000. wrapper.rs:3151 then hits the mixed branch and returns Unsupported mixed IntervalDayTime: days = -1, ms = -7200000.
• n = 700 (hours) → 2_520_000_000 > i32::MAX → ms as i32 wraps to -1_774_967_296, so the pushed-down SQL silently gets a negative interval of the wrong magnitude.

Neither is introduced by this PR — the same literal is what DataFusion evaluates in the post-processing path today, so the value is equally wrong without pushdown — but this test is the natural place to catch it, and negative offsets (DATEADD('hour', -1, ...), DATEADD('minute', -30, ...)) are the common shape in real queries. Adding -2 alongside 2 to the unit loop would fail today.

The underlying fix is a one-liner at the construction site — build the two halves separately instead of multiplying:

Some("hour") => {
    let ms = i32::try_from(3_600_000i64 * i64::from(interval_int)).ok()?; // bail if it overflows
    ScalarValue::IntervalDayTime(Some(i64::from(ms) & 0xFFFF_FFFF))
}

(and the same shape for millisecond / second / minute; day / week are exact multiples of 2^32 so they already sign-extend correctly). Happy to be wrong here if there's a normalization step I've missed — but if it holds, it's worth either fixing in this PR or filing separately, since this PR is what makes the value visible in generated SQL.