Skip to content

fix(cubesql): Cast DATE +/- INTERVAL explicitly in temporal comparisons - #11831

Open
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/fix-date-interval-pushdown-cast
Open

fix(cubesql): Cast DATE +/- INTERVAL explicitly in temporal comparisons#11831
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/fix-date-interval-pushdown-cast

Conversation

@MazterQyou

Copy link
Copy Markdown
Member

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 makes the implicit TIMESTAMP cast of DATE +/- INTERVAL arithmetic explicit in temporal comparisons, so SQL pushdown of filters like time >= CURRENT_DATE - INTERVAL '28 days' is valid on BigQuery. Related test is included.

@MazterQyou
MazterQyou requested a review from a team as a code owner September 9, 2026 16:22
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Sep 9, 2026
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 5m 35s —— View job


Code review — no blocking bug found; 2 medium design concerns, 4 low. 5 inline comments posted.

Full review
  • Gather context (read diff + surrounding code)
  • Analyze correctness / edge cases
  • Review tests
  • Check existing review threads for dedupe (none existed)
  • Post review

Verdict

The fix is correct for the case it targets, and I could not find a case where it produces wrong results or regresses existing behaviour. The new comparison block returns early only for Timestamp/Date32 operand pairs, so the DATE - DATE → DATEDIFF, non-comparison DATE-cast, and string-literal-cast paths below are all still reached with identical behaviour. The wrapper-pull-up-cast change mirrors wrapper-pull-up-binary-expr node for node.

My concerns are about generality and consistency with the code that already solves this exact problem, not about the fix being wrong.

Medium

1. The arithmetic side is cast but never constant-foldedinline

between_expr_normalize (plan_normalize.rs:1681) handles the identical problem and wraps and folds via evaluate_expr_stacked. That's why test_wrapper_between_timestamp_date_bounds asserts a bare timestamptz '...' with no CURRENT_DATE/INTERVAL in the SQL. The new path returns the Cast raw. It's inconsistent even within normalize_temporal_operand itself — the Date32 branch three lines down does call evaluate_expr.

2. Shape-specific matching where BETWEEN's is shape-agnosticinline

is_date_interval_arithmetic only walks BinaryExpr +/- trees. between_expr_normalize uses bound_is_computedanything not a Column/Literal/Cast. So this still fails on BigQuery:

WHERE order_date >= DATE_TRUNC('week', CURRENT_DATE - INTERVAL '28 days')

DATE_TRUNC is an Expr::ScalarFunction typed Timestamp(ns, None), so the let ... else bails immediately, no cast is emitted, and BigQuery sees DATETIME vs TIMESTAMP. Extracting bound_is_computed into a helper both paths call would cover the general case and delete the recursion.

Low

Comment length The 6-line block at plan_normalize.rs:1385 carries one load-bearing sentence; the rest is restated in normalize_temporal_operand's doc and again in all three tests. inline
Operator coverage IsDistinctFrom / IsNotDistinctFrom are comparisons with the same mismatch, excluded from is_comparison.
Stale doc The module doc at line 36 gained the new bullet; the duplicate list in binary_expr_normalize's own doc (line 1341) did not. The two have now drifted — consider deleting the function-level list.
Date64 asymmetry is_date accepts Date64, but the target_type match only admits Date32, so that arm is reachable only through nesting.
Unguarded recursion This file routes recursion through expr_normalize_stacked / #[inline(never)] after past stack-overflow issues (test_stack_overflow_deeply_nested_or); the new recursion isn't.
Template gate Only expressions/cast is checked, but Expr::Cast generation also needs the types/* template sql_type() resolves to (transport/service.rs:1073). inline

Testing

All three new tests early-return on !Rewriter::sql_push_down_enabled(), so the plain CubeScan path is untested — yet PlanNormalize runs unconditionally (query_engine.rs:147) and now injects an Expr::Cast into every such filter. CastExpr does participate in e-graph constant folding (analysis.rs:1109) so it likely still yields an afterDate filter, but that's the highest-blast-radius consequence of this change and nothing pins it. inline

A negative test for the new template gate (drop expressions/cast via the already-used convert_select_to_query_plan_customized, assert the query stays unwrapped) would also be cheap.

What's good

  • The wrapper-pull-up-cast gating follows the binary_expr.rs precedent exactly — same 7-var context on both sides, same get_data_source + can_rewrite_template transform, unification handling the relationship rather than a hand-rolled comparison.
  • Testing both the Postgres default templates and a BigQuery-shaped template override is the right pair; the third test is what actually demonstrates the bug is fixed.
  • The unit test in plan_normalize.rs asserts the normalized Expr directly rather than going through SQL, which is the right level for that layer.

Not verified

I did not compile or run the test suite — there are no prebuilt artifacts in rust/cubesql/target, and a cold cargo check on this crate exceeds the time budget here. Relying on CI for cargo test / cargo clippy / cargo fmt.

· branch [`cubesql/fix-date-interval-pushdown-cast`](https://github.com/cube-js/cube/tree/cubesql/fix-date-interval-pushdown-cast)

Comment thread rust/cubesql/cubesql/src/compile/engine/df/optimizers/plan_normalize.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/cast.rs
Comment thread rust/cubesql/cubesql/src/compile/test/test_wrapper.rs
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.57542% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.39%. Comparing base (5019312) to head (3e5100c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
rust/cubesql/cubesql/src/transport/service.rs 72.72% 9 Missing ⚠️
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs 96.98% 6 Missing ⚠️
...src/compile/engine/df/optimizers/plan_normalize.rs 93.90% 5 Missing ⚠️
.../cubesql/src/compile/rewrite/rules/wrapper/cast.rs 93.33% 2 Missing ⚠️
...l/cubesql/src/compile/rewrite/rules/wrapper/mod.rs 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11831      +/-   ##
==========================================
+ Coverage   80.35%   80.39%   +0.04%     
==========================================
  Files         500      500              
  Lines      106555   106879     +324     
  Branches     3881     3881              
==========================================
+ Hits        85619    85923     +304     
- Misses      20386    20406      +20     
  Partials      550      550              
Flag Coverage Δ
cube-backend 60.23% <ø> (ø)
cubesql 84.80% <93.57%> (+0.03%) ⬆️

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.

@MazterQyou
MazterQyou force-pushed the cubesql/fix-date-interval-pushdown-cast branch from 9b0a6ce to 06d068a Compare September 9, 2026 17:24
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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


Re-review after push — no blocking bug; 1 medium, 3 low. 4 of 5 prior threads addressed and resolved.

Full review
  • Gather context (read diff + surrounding code)
  • Check existing review threads for dedupe / stale resolution
  • Analyze correctness / edge cases
  • Review tests
  • Post review

Prior round

Four of my five threads are addressed; I resolved them:

Prior concern Status
Fold rationale unstated Now documented in the comment at plan_normalize.rs:1387
IsDistinctFrom / IsNotDistinctFrom excluded Added
is_comparison binding used once Inlined
Date64 accepted by the helper but not by target_type Date64 dropped; helper and match now both Date32-only
Unguarded recursion in is_date_interval_arithmetic Rewritten as a loop — no stack depth
types/* template not checked alongside expressions/cast sql_type() now checked too
Plain CubeScan path untested test_binary_expr_computed_date_bound_cube_scan_filter added
Negative test for the template gate test_wrapper_cast_without_template_folds_to_cube_scan_filter added

Still open from before: shape-specific matching (thread). is_date_interval_arithmetic walks only BinaryExpr +/-, so WHERE order_date >= DATE_TRUNC('week', CURRENT_DATE - INTERVAL '28 days') still emits no cast and still fails on BigQuery, while between_expr_normalize's shape-agnostic bound_is_computed covers it. Left unresolved.

Correctness

I traced the new early-return block against the paths below it and found no regression:

  • (Timestamp, Date32) / (Date32, Timestamp)normalize_temporal_operand does exactly what the match at line 1417 did (evaluate_expr(cast_to(target))).
  • (Timestamp, Timestamp) now returns early. The only path it skips is the string-literal cast, which requires one side to be Expr::Literal(Utf8) — that side would be typed Utf8, not Timestamp, so it's unreachable.
  • DATE - DATE → DATEDIFF is handled above the new block, untouched.
  • The loop in is_date_interval_arithmetic always descends into a strict subexpression, so it terminates. Sign handling is right: Minus only accepts an interval on the right; Plus accepts either side.
  • The negative template test is valid — get_test_tenant_ctx_customized filters out empty template values (test/mod.rs:813), so "" genuinely removes expressions/cast rather than registering an empty one.

Medium

Folding policy now contradicts between_expr_normalizeinline

The new comment argues a folded bound would "pin CURRENT_DATE to planning time." normalize_bound (line 1668) folds the identical bound via evaluate_expr_stacked. So the same predicate reaches the target engine in two different shapes depending on whether it was written as >= or BETWEEN, and one of the two is wrong by the other's stated reasoning. Worth reconciling, or at least cross-referencing so the divergence is deliberate on the page.

Low

Test asserts shape, not value The midnight regex passes for CURRENT_DATE, CURRENT_DATE + 28 days, or a sign flip — the 28-day offset is the thing under test and isn't pinned. inline
Duplicated DataSource match transform_cast_expr re-implements can_rewrite_template's Unrestricted => true policy inline. inline
Comment length 6 lines at plan_normalize.rs:1387. The last sentence is now pinned by a test and can go.

What's good

  • The loop rewrite of is_date_interval_arithmetic is a genuine improvement over the recursion — clearer and it removes the stack-depth question entirely.
  • sql_type() in the pull-up gate is the correct fix: the gate now checks the full precondition generate_sql_for_expr's Expr::Cast arm actually needs, not a proxy for it.
  • test_wrapper_cast_without_template_folds_to_cube_scan_filter is the right negative test — it asserts both halves (no CAST( in the SQL and the filter still lands as afterOrOnDate), so it would catch a gate that's too loose or a fallback that silently drops the predicate.
  • test_wrapper_binary_expr_date_side_computed_date_bound covering CAST(col AS DATE) >= … is the case I'd have missed; the double-cast assertion is exactly right.

Not verified

cargo check is not available to me in this environment, so I did not compile or run the suite. I did verify by inspection that every helper the new tests call exists (try_expect_root_cube_scan, find_cube_scan, find_cube_scan_wrapped_sql in test/utils.rs) and that regex::Regex is already imported in test_wrapper.rs. Relying on CI for cargo test / clippy / fmt.

· branch [`cubesql/fix-date-interval-pushdown-cast`](https://github.com/cube-js/cube/tree/cubesql/fix-date-interval-pushdown-cast)

Comment thread rust/cubesql/cubesql/src/compile/engine/df/optimizers/plan_normalize.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/test/test_wrapper.rs
Comment thread rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/cast.rs
@MazterQyou
MazterQyou force-pushed the cubesql/fix-date-interval-pushdown-cast branch from 06d068a to d0390aa Compare September 9, 2026 18:32
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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


Re-review after push — no blocking bug; 1 medium (perf), 2 low. 3 prior threads addressed and resolved, 1 still open.

Full review
  • Gather context (read diff + surrounding code)
  • Check existing review threads for dedupe / stale resolution
  • Analyze correctness / edge cases
  • Review tests
  • Post review

Prior round

Prior concern Status
Folding policy diverges from between_expr_normalize without acknowledgement Comment now says "Unlike a BETWEEN bound it is not folded" and gives the reason — resolved
Test asserted a midnight timestamp, not the 28-day offset Now computes today/today-1 minus 28 days and compares — resolved
transform_cast_expr duplicated can_rewrite_template's DataSource match Extracted as template_sql_generator in wrapper/mod.rs:259, both callers use it — resolved
Shape-specific matching (thread) Still open. is_date_interval_arithmetic walks only BinaryExpr +/-, so WHERE order_date >= DATE_TRUNC('week', CURRENT_DATE - INTERVAL '28 days') emits no cast and still fails on BigQuery, while between_expr_normalize's bound_is_computed covers it

Correctness

Re-traced the new block against the paths below it — no regression found:

  • DATE - DATE → DATEDIFF sits above the new block (line 1370), untouched.
  • (Timestamp, Date32) / (Date32, Timestamp) reproduce the old match arms exactly via normalize_temporal_operand's evaluate_expr(cast_to(target)).
  • (Timestamp, Timestamp) now returns early; the only skipped path is the Utf8 literal cast, unreachable when both sides type as Timestamp.
  • The loop in is_date_interval_arithmetic always descends to a strict subexpression, so it terminates. Sign handling checks out: Minus accepts an interval only on the right, Plus on either side; INTERVAL + INTERVAL and ts_col + INTERVAL both fall through to false.
  • Err(bool) from template_sql_generator carries Unrestricted => true / unknown-source => false correctly to both call sites.
  • The negative-template test is valid: get_test_tenant_ctx_customized filters empty values (test/mod.rs:813), so "" genuinely removes expressions/cast.

Medium

The template gate renders a jinja template on every match attemptinline

sql_type() is not a lookup — it goes through render_template (transport/service.rs:1085). This is the only jinja render in the entire rewrite-rule set; every other gate is a HashMap::contains_key. Transforms run per matching e-class per saturation iteration, so wrapper-pull-up-cast now pays a template fetch + render + String alloc per attempt, only to discard the string. The check is semantically right; a contains_sql_type sharing sql_type's match arms would keep it at lookup cost.

Low

Doc grammar wrapper/mod.rs:259: "a data source meta does not know nothing" parses as a double negative. The intended elision ("for a data source meta doesn't know: nothing may be rendered") doesn't survive the comma.
Result<_, bool> as verdict Err(true) meaning allowed inverts the usual reading of Err. ControlFlow<bool, &Arc<…>> would say the same thing without the surprise, though the current form does centralise the policy, which was the point.

What's good

  • Extracting template_sql_generator puts the Unrestricted TODO is it correct? in exactly one place, so answering it later is a one-line change rather than a hunt.
  • The comment is down to four lines and every one of them is load-bearing; the BETWEEN cross-reference makes the divergence deliberate on the page rather than something a reader has to reconstruct.
  • test_binary_expr_computed_date_bound_cube_scan_filter now pins the offset while staying non-flaky across a UTC midnight boundary — that's the right trade, and the failure message prints both accepted values.
  • test_wrapper_cast_without_template_folds_to_cube_scan_filter asserts both halves (no CAST( in the SQL and the filter still lands as afterOrOnDate), so a gate that's too loose or a fallback that silently drops the predicate both fail it.

Not verified

cargo check is not available in this environment, so I did not compile or run the suite. Verified by inspection that contains_template, sql_type, SqlGenerator, and CastExprDataType all exist with the signatures used, and that chrono is a dependency of the test crate. Relying on CI for cargo test / clippy / fmt.

· branch [`cubesql/fix-date-interval-pushdown-cast`](https://github.com/cube-js/cube/tree/cubesql/fix-date-interval-pushdown-cast)

Comment thread rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/cast.rs Outdated
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 1m 32s —— View job


Re-review after push — no blocking bug; 0 new issues, 1 prior thread still open (shape-specific matching). Perf thread addressed and resolved.

Full review
  • Gather context (read diff + surrounding code)
  • Check existing review threads for dedupe / stale resolution
  • Analyze correctness / edge cases
  • Review tests
  • Post review

Prior round

Prior concern Status
sql_type() in the pull-up gate renders a jinja template on every match attempt Resolved. The type-name match arms are extracted into SqlTemplates::sql_type_template (transport/service.rs:1053) and both sql_type and the new contains_sql_type go through it, so the gate is now a HashMap::contains_key like every other one — no render, no String alloc, no format! in the hot loop
template_sql_generator doc grammar / Result<_, bool> as verdict Resolved. Now ControlFlow<bool, &Arc<dyn SqlGenerator …>>, which reads correctly (Break = no generator to consult, carrying the verdict), and the doc sentence parses
Shape-specific matching (thread) Still open. is_date_interval_arithmetic walks only BinaryExpr +/-, so WHERE order_date >= DATE_TRUNC('week', CURRENT_DATE - INTERVAL '28 days') emits no cast and still fails on BigQuery, while between_expr_normalize's bound_is_computed covers that class

Correctness

Traced the refactor and the normalize path again; nothing new found.

  • sql_type is behaviour-preserving: the unsupported-type error text and the Decimal precision/scale context both survive the restructure, and every former "foo" name maps to the same "types/foo" key.
  • contains_sql_type is exactly the precondition generate_sql_for_expr's Expr::Cast arm needs, minus the render. The only divergence it can have from sql_type is a malformed template, which fails identically at generation time either way.
  • can_rewrite_template and transform_cast_expr now share one Unrestricted => true decision point, so the TODO is it correct? is answerable in one edit.
  • The loop in is_date_interval_arithmetic always descends to a strict subexpression → terminates. Minus accepts an interval only on the right, Plus on either side; INTERVAL + INTERVAL and ts_col + INTERVAL both fall through to false.
  • The new comparison block returns early only for Timestamp/Date32 pairs. DATE - DATE → DATEDIFF sits above it (line 1370), the (Timestamp, Date32) arms below are reproduced exactly, and the only path skipped for (Timestamp, Timestamp) is the Utf8-literal cast, unreachable when both sides type as Timestamp.
  • IsDistinctFrom / IsNotDistinctFrom casting the Date32 side is semantically safe — the cast is total, so null-equality semantics are unchanged.

Tests

Coverage is now in the right shape: pushdown (default templates + BigQuery-shaped overrides), the CAST(col AS DATE) double-cast case, the plain-CubeScan fold, and the negative template gate. test_binary_expr_computed_date_bound_cube_scan_filter pins the 28-day offset while tolerating a UTC-midnight straddle, and prints both accepted values on failure.

What's good

  • sql_type_template is the right factoring: one list of type names, two callers with different costs, and no way for the gate and the generator to drift on which types are supported.
  • ControlFlow over Result<_, bool> for template_sql_generatorBreak(true) meaning allowed reads naturally, where Err(true) did not.
  • The comment at plan_normalize.rs:1387 is four lines, every one load-bearing, and the BETWEEN cross-reference makes the folding divergence deliberate on the page.
  • test_wrapper_cast_without_template_folds_to_cube_scan_filter asserts both halves (no CAST( in the pushed-down SQL and the filter still lands as afterOrOnDate), so a gate that's too loose or a fallback that silently drops the predicate both fail it.

Not verified

cargo check is not available in this environment, so I did not compile or run the suite. Verified by inspection that contains_template is pub (transport/service.rs:482), that SqlGenerator is exported from crate::transport, and that std::ops::ControlFlow is imported in both files that use it. Relying on CI for cargo test / clippy / fmt.

· branch [`cubesql/fix-date-interval-pushdown-cast`](https://github.com/cube-js/cube/tree/cubesql/fix-date-interval-pushdown-cast)

@MazterQyou

Copy link
Copy Markdown
Member Author

/bot-review

@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 1m 28s —— View job


Re-review — no code changes since the last round; the last open thread is answered and resolved. 0 issues.

Full review
  • Gather context (read diff + surrounding code)
  • Check existing review threads for dedupe / stale resolution
  • Verify the author's rebuttal on the open thread
  • Analyze correctness / edge cases
  • Review tests
  • Post review

State

HEAD is 507959e, the same commit the previous round reviewed — git diff origin/master...HEAD is byte-identical to what I traced then. Nothing new to analyse in the code; the new input is @MazterQyou's reply on the one remaining thread.

The shape-specific-matching thread — rebuttal accepted, resolved

I checked the three load-bearing claims:

  1. ConstEvaluator panics on the placeholder UDFs. Confirmed: compile/engine/udf/common.rs:1475 is a literal panic!("Should be rewritten with UtcTimestamp function") behind an Immutable volatility declaration. So the fold-and-cast variant genuinely cannot run before the rewrite rules resolve those, and the failures cited (test_wrapper_now, test_localtimestamp, the QuickSight LOCALTIMESTAMP + interval cases) follow directly. That the BETWEEN path shares the latent panic for ts BETWEEN NOW() - INTERVAL '1 day' AND NOW() is a real, separate bug — agreed it isn't a precedent to widen.

  2. My DATE_TRUNC counterexample already works on BigQuery. Confirmed: BigqueryQuery.ts:340 renders DATETRUNC as TIMESTAMP(DATETIME_TRUNC(CAST({{ args[1] }} AS DATETIME), …)), so the expression reaches the comparison as a TIMESTAMP. My example was wrong about what BigQuery sees.

  3. The narrow shape is the principled boundary. This is the part that actually settles it. DATE +/- INTERVAL is the one place where the plan's type and the rendered SQL's type can disagree with no node in the plan saying so — DataFusion's coercion types it TIMESTAMP while both operands keep their own types. Every other computed temporal expression passes through a dialect template that owns its return type, so a wrong type there is a template bug and belongs in the template. Blanket-casting them would change pushed-down SQL on Postgres/Snowflake/Databricks for no dialect that needed it. That reasoning is stronger than the symmetry-with-BETWEEN argument I was making, and it's worth noting it's now on the page in the comment at plan_normalize.rs:1387.

Thread resolved. That was the last unresolved one — all nine from the four rounds are now closed.

Correctness (unchanged code, re-checked)

  • DATE - DATE → DATEDIFF returns at line 1384, above the new block.
  • (Timestamp, Date32) / (Date32, Timestamp) reproduce the old match arms exactly through normalize_temporal_operand's evaluate_expr(cast_to(target)).
  • (Timestamp, Timestamp) returns early; the only skipped path is the Utf8-literal cast, unreachable when both sides type as Timestamp.
  • (Date32, Date32) falls through to _ => None and reaches the paths below unchanged.
  • The loop in is_date_interval_arithmetic always descends to a strict subexpression, so it terminates without stack depth. Minus accepts an interval only on the right, Plus on either side.
  • IsDistinctFrom / IsNotDistinctFrom casting the Date32 side is safe — the cast is total, so null-equality semantics don't change.
  • contains_sql_type and sql_type share sql_type_template, so the gate can't drift from what generate_sql_for_expr's Expr::Cast arm needs, and the gate stays a HashMap::contains_key.

Tests

Coverage is in the right shape and nothing has moved: pushdown under the default templates, pushdown under BigQuery-shaped overrides, the CAST(col AS DATE) double-cast case, the plain-CubeScan fold with the 28-day offset pinned (tolerating a UTC-midnight straddle), the negative template gate asserting both halves, and the Expr-level unit test in plan_normalize.rs.

Not verified

cargo check is not available in this environment, so I did not compile or run the suite in any round. Relying on CI for cargo test / clippy / fmt.

Verdict

Nothing blocking, and nothing left open from my side. LGTM.

· branch [`cubesql/fix-date-interval-pushdown-cast`](https://github.com/cube-js/cube/tree/cubesql/fix-date-interval-pushdown-cast)

…sons

Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou
MazterQyou force-pushed the cubesql/fix-date-interval-pushdown-cast branch from 507959e to 3e5100c Compare September 11, 2026 17:07
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 1m 28s —— View job


Re-review after push — rebase onto current master, no content change; 0 issues, all 9 prior threads resolved. LGTM.

Full review
  • Gather context (diff + surrounding code)
  • Check existing review threads for dedupe / stale resolution
  • Analyze correctness / edge cases
  • Review tests
  • Post review

What changed in this push

HEAD moved from 507959e to 3e5100c, but the tree did not. git diff origin/master...HEAD is byte-identical to the state I traced last round, and the five blob SHAs on the changed files (b8c4faa, c699e88, a061d29, 16dc736, d0d22e8) are unchanged. The only difference is the base: the branch is now on top of 46f1c3a (docs: Use named imports for drivers imports (#11850)), a docs-only commit that touches nothing this PR depends on.

So there is no new code to analyse. I re-checked the mechanical things a rebase can break and re-verified the parts that would move if master had shifted under the change.

Threads

All nine threads from the four prior rounds are resolved, including the shape-specific-matching one that @MazterQyou answered last round. Nothing re-opened by the rebase — every thread's anchor line still points at live code, and the three marked isOutdated are ones whose text was rewritten in response, not ones the rebase stranded. No new inline comments to post.

Re-verified after rebase

  • No conflict damage in transport/service.rs. sql_type_template / contains_sql_type / sql_type are intact as a trio, and contains_template (line 482) is still pub. Every sql_type caller in the tree goes through WrappedSelectNode::generate_sql_type (wrapper.rs:2317), so the restructure has exactly one consumer shape to satisfy.
  • nullable_type path untouched. generate_typed_null (wrapper.rs:2321) calls sql_typenullable_typecast_expr; the Decimal context and the unsupported-type error both survive the restructure, so that path renders the same string it did before.
  • Test-file imports still resolve. regex::Regex (line 8) is still used by four other tests, so dropping the regex from test_binary_expr_computed_date_bound_cube_scan_filter left no unused import. chrono is referenced fully-qualified and is already a dependency of the crate.
  • var_iter! over CastExprDataType yields &DataType, matching contains_sql_type(&DataType); an e-class with no CastExprDataType node makes .any return false, which fails closed — the conservative direction.

Correctness (unchanged code, spot-rechecked)

  • DATE - DATE → DATEDIFF returns above the new comparison block; (Date32, Date32) falls to _ => None and reaches the paths below unchanged.
  • (Timestamp, Date32) / (Date32, Timestamp) reproduce the old match arms through normalize_temporal_operand's evaluate_expr(cast_to(target)). (Timestamp, Timestamp) returns early; the only skipped path is the Utf8-literal cast, unreachable when both sides type as Timestamp.
  • The loop in is_date_interval_arithmetic always descends to a strict subexpression, so it terminates with no stack depth. Minus accepts an interval only on the right, Plus on either side.
  • contains_sql_type and sql_type share sql_type_template, so the pull-up gate cannot drift from what generate_sql_for_expr's Expr::Cast arm needs, and the gate stays a HashMap::contains_key.

Tests

Unchanged and still in the right shape: pushdown under default templates, pushdown under BigQuery-shaped overrides, the CAST(col AS DATE) double-cast case, the plain-CubeScan fold with the 28-day offset pinned (tolerating a UTC-midnight straddle), the negative template gate asserting both halves, and the Expr-level unit test in plan_normalize.rs.

Not verified

cargo check is not available in this environment, so I did not compile or run the suite in any round — that includes this one, where the rebase is the only thing that could have introduced a build break. Worth confirming CI is green on 3e5100c before merge, since the previous green run was against the pre-rebase base.

Verdict

Nothing blocking, nothing open. LGTM.

· branch [`cubesql/fix-date-interval-pushdown-cast`](https://github.com/cube-js/cube/tree/cubesql/fix-date-interval-pushdown-cast)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant