Skip to content

fix(cubesql): don't abort on date filters beyond the nanosecond range - #11475

Open
igorlukanin wants to merge 4 commits into
masterfrom
igor/core-732-sql-api-planner-panics-on-date-filters-beyond-2262-04-11-i64
Open

fix(cubesql): don't abort on date filters beyond the nanosecond range#11475
igorlukanin wants to merge 4 commits into
masterfrom
igor/core-732-sql-api-planner-panics-on-date-filters-beyond-2262-04-11-i64

Conversation

@igorlukanin

Copy link
Copy Markdown
Member

Issue

A date filter with an upper bound past 2262-04-11 aborted the SQL API query planner. The client saw a dropped connection or its own timeout rather than an error.

thread 'tokio-runtime-worker' panicked at chrono/src/lib.rs:717:17:
  invalid or out-of-range datetime
Rewrite Error: Unexpected panic. Reason: invalid or out-of-range datetime

9999-12-31 is a widespread "no end date" sentinel in warehouse schemas, and it is the natural way to write a one-sided range in a tool that requires two bounds, so this is reachable from ordinary models.

Root cause

An unchecked i64 multiplication in the Date32Timestamp(Nanosecond) coercion.

date '2262-04-12' is Date32 106752. Converting it to nanoseconds computes 106752 × 86_400_000_000_000 = 9_223_372_800_000_000_000, which exceeds i64::MAX by 763_145_224_193. 2262-04-11 is 106751 and fits with 86.4 s to spare — exactly where the reported boundary sits. The representable window is symmetric at ±106_751 days: 1677-09-22 to 2262-04-11.

The multiply that overflows is arrow's multiply kernel, which is a plain math_op(left, right, |a, b| a * b). With no checked_mul the two build profiles fail differently from one cause:

  • debug traps — attempt to multiply with overflow
  • release wraps to -9_223_371_273_709_551_616, whose secs/nsecs split is then rejected by chrono's infallible NaiveDateTime::from_timestamp — the message in the report

It fires in PlanNormalize, which runs before the egg rewriter:

evaluate_expr_stacked → DataFusion ConstEvaluatorCastExpr::evaluate → arrow cast_with_optionsmultiply.

Note the literal arrives as CAST(CAST(Utf8("2262-04-12") AS Date32) AS Timestamp(Nanosecond, None)) — it is still a string at that point and only becomes a Date32 when the cast is evaluated, which is the evaluation that overflows.

Fix

A representability guard in evaluate_expr_stacked that declines to evaluate an expression carrying a date/time literal outside the nanosecond window. Because the literal is a string there, the guard parses it against the type it is cast to rather than waiting for a typed value.

Returning an error rather than the expression unevaluated is deliberate: left in the plan, the un-evaluated cast reaches the rewriter and overflows there instead, so declining silently only moves the panic.

Also fixes the same unchecked multiply in the date_to_timestamp UDF, which produced a garbage instant instead of an error. The other two 86_400_000_000_000 sites in the crate divide by the constant and cannot overflow.

Result

The affected queries now succeed, with the out-of-range date passed through verbatim — the Cube REST API takes ISO strings and never needs the nanosecond timestamp.

Filter Before After
<= date '2262-04-12' panic beforeOrOnDate = 2262-04-12T00:00:00.000Z
<= date '9999-12-31' panic beforeOrOnDate = 9999-12-31T00:00:00.000Z
BETWEEN date '2020-01-01' AND date '9999-12-31' panic range pushed down, both bounds intact
<= date '2262-04-11' worked unchanged

Tests

  • test_filter_date_beyond_nanosecond_range_is_pushed_down — 2262-04-12 and 9999-12-31 push down with the date intact
  • test_filter_date_at_nanosecond_range_boundary_is_pushed_down — 2262-04-11 still pushes down, so the guard does not over-reject
  • test_filter_between_date_beyond_nanosecond_range — the BETWEEN path, which normalizes bounds separately and propagates errors
  • test_date32_representable_boundaries — ±106_751 accepted; ±106_752 and i32::MIN/MAX rejected
  • test_string_literal_judged_through_its_date_cast — string-through-cast resolution, and that a non-temporal cast target is left alone

780 cubesql lib tests green. Both halves of the guard were mutation-checked: disabling the range check and disabling the string-through-cast resolution each turn the integration tests red with the original overflow.

@igorlukanin
igorlukanin requested a review from a team as a code owner August 5, 2026 00:02
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Aug 5, 2026
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 3m 18s —— View job


Review complete — solid fix for the reported panic; 3 medium, 2 low. Main concern: the guard declines more than the overflow requires, silently degrading the string-literal filter form.

Full review
  • Read the diff (git diff origin/master...HEAD)
  • Review the plan_normalize.rs representability guard
  • Review the date_to_timestamp UDF checked_mul change
  • Trace all evaluate_expr / evaluate_expr_stacked call sites for over-rejection
  • Review test coverage
  • Post inline comments and summary

What's good

The diagnosis is excellent and the writeup is unusually precise — the ±106_751-day symmetry, the debug-traps/release-wraps split from one unchecked *, and the observation that the literal is still Utf8 at PlanNormalize time are all correct and non-obvious. Choosing to error rather than silently return the expression unevaluated is the right call and the rationale is in the code, not just the PR body.

The date_to_timestamp UDF change (common.rs) is clean: checked_mulok_or_else.transpose()collect::<Result<_>>()? is idiomatic, preserves nulls, and replaces a garbage instant with a real error. No issues there.

test_date32_representable_boundaries and test_string_literal_judged_through_its_date_cast are good unit tests — they pin both the boundary and the string-through-cast resolution, and the mutation check described in the PR is the right way to validate a guard.

Findings

# Severity Where Issue
1 Medium plan_normalize.rs:1650 Guard covers Date32/Date64/all Timestamp units; only Timestamp(Nanosecond) overflows. Silently degrades WHERE dim_date0 <= '9999-12-31' (string form) at line 1459, where the new Err is .ok()-swallowed
2 Medium plan_normalize.rs:1724 _ => () is fail-open — Expr::Case, AggregateFunction, WindowFunction, Like, GetIndexedField, Sort, InSubquery aren't traversed, so a literal hidden in a CASE arm still reaches the overflowing cast
3 Medium test_filters.rs:401 test_filter_between_date_beyond_nanosecond_range accepts both Ok and Err — it asserts only "did not panic" and cannot regress
4 Low plan_normalize.rs:1670 No short-circuit; full re-traversal per enclosing evaluate_* call on a path where this file otherwise tracks traversal cost and stack depth carefully
5 Low plan_normalize.rs:1732 Error embeds the whole expression (can be a 512-deep OR); wording is date-only but the guard also fires on timestamp scalars

Finding 1 is the one I'd want addressed before merge — it's the only one that changes behavior for a query that works today, and it does so quietly rather than with the new error message.

Scope note (not a blocker)

The guard sits at one call site. As the PR body acknowledges, the overflow lives in arrow's multiply kernel and is reachable from the egg rewriter too; guarding evaluate_expr_stacked fixes the reported path but leaves the kernel unchecked for any other caller. Reasonable to defer, but worth a CORE- follow-up so the next path that hits it isn't diagnosed from scratch.

Security / performance

No security surface — no user input reaches a new sink, and the change strictly narrows what gets evaluated. Performance impact is the extra traversal in finding 4; negligible for typical expressions.

Not verified

I did not compile or run the Rust test suite (a cold cubesql build exceeds the time available in this job). Findings 1–3 are from reading the call graph — in particular finding 1 traces binary_expr_normalizecast_string_literal_expr (line 1459) → evaluate_expr(...).ok(), and your own unit test assert!(!cast_to_date("9999-12-31")) against DataType::Date32 confirms the guard returns false for that cast target. Worth confirming with an actual WHERE dim_date0 <= '9999-12-31' test case.

· branch `igor/core-732-sql-api-planner-panics-on-date-filters-beyond-2262-04-11-i64`

Comment thread rust/cubesql/cubesql/src/compile/engine/df/optimizers/plan_normalize.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/test/test_filters.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/engine/df/optimizers/plan_normalize.rs Outdated
Comment thread rust/cubesql/cubesql/src/compile/engine/df/optimizers/plan_normalize.rs Outdated
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.79381% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.48%. Comparing base (a6fbbb4) to head (2cf3336).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...src/compile/engine/df/optimizers/plan_normalize.rs 81.51% 44 Missing ⚠️
...t/cubesql/cubesql/src/compile/engine/udf/common.rs 0.00% 11 Missing ⚠️
...t/cubesql/cubesql/src/compile/test/test_filters.rs 97.12% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #11475       +/-   ##
===========================================
+ Coverage   59.10%   79.48%   +20.37%     
===========================================
  Files         223      480      +257     
  Lines       17891    99185    +81294     
  Branches     3636     3636               
===========================================
+ Hits        10575    78840    +68265     
- Misses       6795    19824    +13029     
  Partials      521      521               
Flag Coverage Δ
cube-backend 59.10% <ø> (ø)
cubesql 83.97% <84.79%> (?)

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 Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 4m 40s —— View job


Re-review of the new commits — prior 5 findings all addressed; 2 new medium, 1 low. Main concerns: the Err is swallowed plan-wide by unwrap_or, and the Date64 substitution isn't renderable by the wrapper.

Full review
  • Read the updated diff (git diff cc61845...HEAD)
  • Check which prior findings the new commits address
  • Review the new parse_string_literal_as normalization path
  • Trace the Err propagation out of PlanNormalize
  • Review new tests
  • Resolve stale threads, post new findings

Prior findings — all five addressed

Prior Status
Guard wider than the overflow / string form silently un-normalized Fixed differently and better than suggested: cast_string_literal_expr now handles Date32/Date64/all Timestamp targets itself, so the string form never reaches the overflowing kernel, and test_filter_string_date_beyond_nanosecond_range_is_pushed_down pins it
_ => () fail-open walk Fixed — the match is now exhaustive with no catch-all, all container variants enumerated, and test_unrepresentable_literal_found_inside_containers covers CASE/BETWEEN/aggregate/within_group/Sort/alias
BETWEEN test accepted Ok or Err Fixed — now asserts the exact time_dimensions dateRange
No short-circuit in the walk Fixed — find_map short-circuits on the first hit
Error message renders the whole expression Fixed — renders only the offending literal

All five threads resolved. The exhaustiveness comment on find_unrepresentable_datetime_literal ("a newly added Expr variant must fail to compile here instead of silently becoming a hiding place") is exactly the right reasoning to leave in the code.

New findings

# Severity Where Issue
1 Medium plan_normalize.rs:1803 The Err is discarded by optimizer.optimize(…).unwrap_or(optimized_plan) in query_engine.rs:164, so it reverts the whole plan to un-normalized rather than one cast. Never surfaces to the user; and one out-of-range date disables unrelated normalizations in the same query (e.g. the DATE - DATEDATEDIFF rewrite, which exists to avoid INTERVAL on non-Postgres dialects)
2 Medium plan_normalize.rs:1492 The Timestamp(Nanosecond)Date64 fallback changes the literal's type. wrapper.rs has the Date64 arm commented out (line 2986) and falls to Can't generate SQL for literal (line 3074), so the filter breaks SQL generation whenever it's rendered by the wrapper instead of pushed down as a member. Also: Date64 → Timestamp(ns) is the same unchecked millis * 1_000_000 (the guard itself rejects this scalar), and a string with a time component yields a non-day-aligned Date64, which Arrow forbids
3 Low plan_normalize.rs:1469 Epoch recomputed per call with a ? on an infallible from_ymd_opt; and the Date32 arm now accepts strings the Arrow kernel rejected, silently dropping any time component — undocumented and untested

Finding 1 is worth resolving because it partly undercuts the design note in the code: test_filter_date_beyond_nanosecond_range_is_pushed_down passes precisely because the error is swallowed and the original single-cast expression reaches the rewriter unharmed — which is evidence that declining the coercion locally (as the cast_string_literal_expr fallthrough at line 1531 already does) works, rather than "only moving the panic".

Testing

The four integration tests plus three unit tests are a good spread, and the new string-form test closes the gap flagged last round. Two paths still uncovered:

  • SQL generation through CubeScanWrapper for a Date64-substituted bound (finding 2) — the only place the current fallback is known to break.
  • The error message itself. Nothing today can observe it (finding 1), so assert!(matches!(…)) on a plan error would either pin the behavior or reveal it's unreachable.

Security / performance

No security surface. The guard walk now short-circuits; parse_string_literal_as checks the target type before parsing. No concerns.

Not verified

I did not compile or run the Rust suite — a cold cubesql build exceeds this job's budget. Findings 1 and 2 are from reading the call graph: finding 1 traces binary_expr_normalize's (Timestamp, Date32) arm → evaluate_expr → guard → ?query_engine.rs:164 .unwrap_or; finding 2 traces the returned ScalarValue::Date64 into wrapper.rs:2986/3074. Both are worth confirming with a wrapper-path test rather than taken on my reading alone.

· branch `igor/core-732-sql-api-planner-panics-on-date-filters-beyond-2262-04-11-i64`

// rather than returning the expression unevaluated is deliberate: left in the plan, the cast
// reaches the rewriter and overflows there instead.
if let Some(literal) = find_unrepresentable_datetime_literal(&expr) {
return Err(DataFusionError::Plan(format!(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Err never reaches the user, and it costs more than the offending expression. query_engine.rs:160-165 runs the rules as:

optimized_plan = optimizer.optimize(&optimized_plan, &optimizer_config).unwrap_or(optimized_plan);

So a DataFusionError from PlanNormalize is discarded and the entire plan reverts to un-normalized — not just the one cast. For dim_date0 <= date '9999-12-31' that's exactly what happens today: the literal arrives as CAST(Utf8 AS Date32), binary_expr_normalize's (Timestamp, Date32) arm wraps it in CAST(… AS Timestamp(Nanosecond)) and calls evaluate_expr, the guard fires, and the whole rule is skipped. test_filter_date_beyond_nanosecond_range_is_pushed_down then passes because the original (single-cast) expression reaches the rewriter and pushes down fine.

Two consequences worth weighing:

  1. The stated rationale doesn't hold in this path. The comment says leaving the expression unevaluated "only moves the panic" — but the passing test shows the rewriter handles the un-normalized CAST(Utf8 AS Date32) without overflowing. What would overflow is returning the double cast unevaluated. Declining the coercion locally (keep left op right with the original bound, the way the cast_string_literal_expr fallthrough at line 1531 already does) reaches the same observed end state without the error.

  2. Collateral loss of normalization. Because the failure is plan-wide, one out-of-range date disables every other normalization in the query — e.g.

    WHERE dim_date0 <= date '9999-12-31' AND (dim_date1 - dim_date2) > 3

    loses the DATE - DATEDATEDIFF rewrite, which exists specifically because non-Postgres dialects return INTERVAL otherwise. That's a silent wrong-SQL risk on dialects the rewrite was added for.

Suggest making the guard a local decline at each coercion site (the two (Timestamp, Date32)/(Date32, Timestamp) arms, the IN-list loop, and normalize_bound) rather than an Err out of evaluate_expr_stacked — and, if you keep the error form, at least a test asserting the message actually surfaces, since right now nothing can observe it.

// The instant is real but no nanosecond timestamp can hold it. Milliseconds reach it
// comfortably and the filter is rendered from the scalar, not from this unit, so the
// bound survives instead of being dropped back to an un-normalized string.
None => ScalarValue::Date64(Some(parsed.timestamp_millis())),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substituting a Date64 for the requested Timestamp(Nanosecond) changes the literal's type, not just its unit, and the plan carries that mismatch forward (Timestamp(Nanosecond, None) <= Date64). Three things fall out:

  1. The wrapper can't render Date64. In wrapper.rs the arm is commented out (// ScalarValue::Date64(_) => {}, line 2986), so it falls to the catch-all at line 3074 and returns Can't generate SQL for literal: Date64(…). Any query where this filter is rendered as SQL by CubeScanWrapper rather than pushed down as a member now fails SQL generation; before the change the bound stayed a Utf8 literal, which renders fine. The new test only covers the member-pushdown path.
  2. The overflow can come back downstream. Date64 → Timestamp(Nanosecond) is millis * 1_000_000 through the same unchecked arrow multiply, and 253_402_214_400_000 * 1_000_000 overflows. If DataFusion's comparison coercion ever casts this literal to the column's type (physical planning, or a later rule), it's the original panic in a new place. Note datetime_literal_is_representable agrees — it returns false for the very scalar this line produces, so the guard would reject it if it re-entered evaluate_expr_stacked.
  3. Non-day-aligned Date64 is out of Arrow spec. Arrow requires Date64 values to be evenly divisible by 86_400_000. parse_date_str accepts a time component, so dim_date0 <= '9999-12-31 12:00:00' yields a mid-day Date64. Date64Array::value_as_datetime happens to round-trip it correctly, so the pushdown value is right — but any kernel that treats Date64 as whole days (e.g. Date64 → Date32, date_trunc) is entitled to misbehave.

If the goal is just "don't lose the bound", TimestampMillisecond(Some(millis), tz.clone()) keeps it in the timestamp family, is renderable by the wrapper (line 2996), and stays within spec — at the cost of the unit differing from the requested one, which is the same trade this line already makes. Either way, worth a test for the wrapper/SQL-generation path, since that's where the current choice breaks.

match cast_type {
// chrono's own date range is ±96_465_292 days, so the day count always fits an i32.
DataType::Date32 => {
let epoch = NaiveDate::from_ymd_opt(1970, 1, 1)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, two notes on the new Date32 arm:

  • The epoch is recomputed (and ?-ed on an infallible from_ymd_opt) per call. NaiveDate::from_ymd_opt(1970, 1, 1).unwrap().num_days_from_ce() is the constant 719_163 — either a const EPOCH_DAYS_FROM_CE: i32 = 719_163; or parsed.date_naive().signed_duration_since(epoch).num_days() as i32 reads more directly than the CE-difference.
  • This arm is a behavior change beyond the overflow fix: Date32 targets previously went through the Arrow cast kernel, which is stricter than parse_date_str. Strings the kernel rejected used to leave the comparison un-normalized; they now become a Date32 with any time component silently dropped ('2020-01-01 15:30:00'2020-01-01). That matches Postgres' ::date semantics so it's probably desirable, but it's untested and unmentioned in the doc comment — worth one line either way.

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