fix(cubesql): don't abort on date filters beyond the nanosecond range - #11475
fix(cubesql): don't abort on date filters beyond the nanosecond range#11475igorlukanin wants to merge 8 commits into
Conversation
|
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
What's goodThe diagnosis is excellent and the writeup is unusually precise — the ±106_751-day symmetry, the debug-traps/release-wraps split from one unchecked The
Findings
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 Security / performanceNo 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 verifiedI did not compile or run the Rust test suite (a cold |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11475 +/- ##
===========================================
+ Coverage 59.10% 79.48% +20.38%
===========================================
Files 223 480 +257
Lines 17891 99359 +81468
Branches 3636 3636
===========================================
+ Hits 10575 78980 +68405
- Misses 6795 19859 +13064
+ Partials 521 520 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ad of dropping them
|
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
|
| 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 - DATE → DATEDIFF 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
CubeScanWrapperfor aDate64-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.
…f failing the whole rule
|
Claude finished @igorlukanin's task in 3m 15s —— View job Re-review of
|
| Prior | Status |
|---|---|
Err swallowed by unwrap_or, reverting the whole plan |
Fixed at the binary-comparison site: the (Timestamp, Date32) / (Date32, Timestamp) arms now check find_unrepresentable_datetime_literal on the built cast and return the un-coerced comparison instead of calling evaluate_expr. test_out_of_range_bound_keeps_other_normalizations pins it by asserting DATEDIFF(day, survives alongside an out-of-range bound — a good test, because it fails for the right reason (rule-wide revert) rather than on the filter itself |
Date64 not renderable by the wrapper |
Fixed — wrapper.rs:2987 renders it via generate_sql_for_timestamp!(… timestamp_millis_opt …) instead of falling to Can't generate SQL for literal |
Date64 vs TimestampMillisecond choice |
The added comment settles it: scalar_to_native_datetime (rules/filters.rs:4715) converts only `TimestampNanosecond |
Epoch recomputed per call / undocumented parse_date_str leniency |
Fixed — EPOCH_DAYS_FROM_CE const (719_163 is right) and a doc paragraph on the accepted-strings and dropped-time-component behavior |
The comment above the coercion arms explaining why the decline is side-local (and what an Err would cost) is the kind of note that keeps a future refactor from quietly undoing this.
New findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Medium | plan_normalize.rs:1602, :1663 |
The IN-list and BETWEEN-bound coercions still pass the ns cast to evaluate_expr_stacked, so the guard's Err still propagates out and unwrap_or still discards the whole normalized plan. test_filter_between_date_beyond_nanosecond_range can't see it — the un-normalized plan pushes down fine, which is precisely the blind spot the new <= test was written for |
| 2 | Low | wrapper.rs:2987 |
New arm has no test — every Date64 assertion in the PR goes through find_cube_scan().request, none through find_cube_scan_wrapped_sql, so the one path the Date64 substitution was known to break stays uncovered. Also timestamp_millis_opt(..).unwrap() panics beyond chrono's range (unreachable from parse_string_literal_as, but it replaces a graceful error for any other producer) |
Finding 1 is the same class as last round's finding 1, one call site over: extending the new test's loop to the BETWEEN / IN forms and mirroring the decline at both sites would close it.
Security / performance
No security surface. Perf note: the declined path clones the operand and traverses it once for the guard, then evaluate_expr re-traverses inside evaluate_expr_stacked. Both are bounded by the expression already being walked and negligible next to const evaluation.
Not verified
I did not compile or run the Rust suite — a cold cubesql build exceeds this job's budget. Finding 1 is from reading the call graph: between_expr_normalize → normalize_bound (bound type Date32 ≠ Timestamp(ns) → cast built) → evaluate_expr_stacked → guard → ?, with no local decline between them. Swapping the predicate in test_out_of_range_bound_keeps_other_normalizations to the BETWEEN form is the cheap way to confirm or refute it.
Re-reviewing
|
|
Claude finished @igorlukanin's task in 3m 7s —— View job Re-review of
|
| Prior | Status |
|---|---|
IN-list and BETWEEN coercions still Err out of the rule |
Fixed. in_list_expr_normalize:1602 and normalize_bound:1669 now check find_unrepresentable_datetime_literal on the built cast and return the un-coerced element/bound. The let Expr::Cast { expr: bound, .. } = casted else { unreachable!(…) } unwrap is fine — the Expr::Cast is constructed three lines above. test_out_of_range_bound_keeps_other_normalizations now loops over <=, BETWEEN and IN, so all three declines are pinned against the plan-wide revert |
timestamp_millis_opt(..).unwrap() panics for an extreme Date64 |
Fixed. The arm is written out instead of using generate_sql_for_timestamp! and returns DataFusionError::Internal on a non-Single LocalResult. The comment says exactly why the macro was bypassed, which is the part that keeps someone from "simplifying" it back |
I re-checked the escape surface: evaluate_expr/evaluate_expr_stacked is now reached from four places — the two binary arms (guarded, 1395/1406), the IN closure (guarded), normalize_bound (guarded), and cast_string_literal_expr:1463, which is .ok()-swallowed by construction. So the guard's Err no longer has a path to query_engine.rs's .unwrap_or(plan), and the plan-wide revert is closed. The find_unrepresentable_datetime_literal guard inside evaluate_expr_stacked remains as the backstop, which is the right layering.
New finding
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Low | test_filters.rs:479 |
The IN form is the one of the three declines with no assertion that the filter itself pushes down — the loop only checks DATEDIFF(day, survives, which holds even if the IN filter were dropped. It leaves a mixed-type list (one Timestamp(ns) literal, one un-evaluated CAST(Utf8 AS Date32)), and a failed pushdown leaves that comparison on the DataFusion side where Date32 → Timestamp(ns) coercion is the same unchecked multiply. Reading make_constant_in_list and scalar_to_scalar_value_string says it works; it's just not pinned |
Re-affirmed, not re-posted
The Date64 arm in wrapper.rs:2987 still has no test through find_cube_scan_wrapped_sql. The new wrapped-SQL assertions in test_out_of_range_bound_keeps_other_normalizations all use date '…' literals, which decline to Date32, so no Date64 reaches the wrapper in any test. That thread is resolved and it's a coverage gap rather than a defect, so I haven't reopened it — noting it only so the state is explicit.
Security / performance
No security surface. The two new declines each clone the operand and traverse it once for the guard before evaluate_expr_stacked re-traverses; bounded by an expression already being walked, negligible next to const evaluation.
Not verified
I did not compile or run the Rust suite — a cold cubesql build exceeds this job's budget, and there is no cached target/ on this runner. Two things I'd normally confirm by compiling: that chrono::LocalResult::Single resolves through the MappedLocalTime alias in chrono 0.4.39 (it should — type-alias enum variants are stable, and the alias isn't #[deprecated]), and finding 1, which is inference from analysis.rs:1148 and rules/filters.rs:4698 rather than a run.
|
Claude finished @igorlukanin's task in 1m 6s —— View job Re-review of
|
| Prior | Status |
|---|---|
The IN decline had no assertion that the filter itself survives pushdown |
Fixed. test_filter_in_list_date_beyond_nanosecond_range_is_pushed_down (test_filters.rs:520) pins the structured filter — member MultiTypeCube.dim_date0, operator equals, both ISO values — so the heterogeneous list is now proven to convert rather than inferred from reading make_constant_in_list / scalar_to_scalar_value_string. That was the one of the three declines resting on inference, and it's the one where a failed pushdown would have left a Date32 vs Timestamp(ns) comparison on the DataFusion side, i.e. back at the same unchecked multiply |
I re-checked that the test hits the intended path rather than passing incidentally: in in_list_expr_normalize (plan_normalize.rs:1583-1609) the tested expr is Timestamp(ns), each element normalizes to CAST(Utf8 AS Date32) so list_expr_type is Date32 and the cast is built — then date '2020-01-01' folds through evaluate_expr_stacked while date '9999-12-31' trips find_unrepresentable_datetime_literal and returns un-coerced. So the assertion is over exactly the mixed-type list the doc comment describes, and it should go red if the decline is reverted (overflow) or if the element is dropped (values shrinks). The doc comment stating that outcome above the test is the right place for it.
Final state of the change
Three coercion sites decline locally (binary_expr_normalize at 1395/1406, in_list_expr_normalize at 1602, normalize_bound at 1669), the guard inside evaluate_expr_stacked remains as the backstop for shapes that can't drop the cast, and the only remaining caller that can see its Err is cast_string_literal_expr:1463, which is .ok()-swallowed by construction. So no path reaches query_engine.rs's .unwrap_or(plan). Each of the three declines has a pushdown test plus a control arm in test_out_of_range_bound_keeps_other_normalizations asserting DATEDIFF(day, survives alongside the out-of-range bound.
Coverage note (re-affirmed, not re-posted)
The Date64 arm in wrapper.rs:2987 still has no test through find_cube_scan_wrapped_sql — every Date64 assertion goes through find_cube_scan().request. That thread is resolved, and the reason given (every shape tried either matched the bound as a member or parameterized it to $1, so the test passed with the arm removed and was dropped rather than kept as an unfalsifiable assertion) is the right call — an untested arm justified by reachability, stated plainly, beats a test that proves nothing. Noting it only so the state stays explicit.
Security / performance
No change from last round. No security surface; the added test is test-only.
Not verified
I did not compile or run the Rust suite — a cold cubesql build exceeds this job's budget and there's no cached target/ on this runner. The expected equals operator and the two ISO values in the new assertion are the one thing I'd normally confirm by running; they're consistent with how scalar_to_scalar_value_string renders Date32/TimestampNanosecond, and CI will settle it either way.
1 participant
Issue
A date filter with an upper bound past
2262-04-11aborted the SQL API query planner. The client saw a dropped connection or its own timeout rather than an error.9999-12-31is 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
i64multiplication in theDate32→Timestamp(Nanosecond)coercion.date '2262-04-12'isDate32106752. Converting it to nanoseconds computes106752 × 86_400_000_000_000 = 9_223_372_800_000_000_000, which exceedsi64::MAXby 763_145_224_193.2262-04-11is 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'smultiplykernel, which is a plainmath_op(left, right, |a, b| a * b). With nochecked_multhe two build profiles fail differently from one cause:attempt to multiply with overflow-9_223_371_273_709_551_616, whosesecs/nsecssplit is then rejected by chrono's infallibleNaiveDateTime::from_timestamp— the message in the reportIt fires in
PlanNormalize, which runs before the egg rewriter:evaluate_expr_stacked→ DataFusionConstEvaluator→CastExpr::evaluate→ arrowcast_with_options→multiply.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 aDate32when the cast is evaluated, which is the evaluation that overflows.Fix
A representability guard in
evaluate_expr_stackedthat 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_timestampUDF, which produced a garbage instant instead of an error. The other two86_400_000_000_000sites 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.
<= date '2262-04-12'beforeOrOnDate=2262-04-12T00:00:00.000Z<= date '9999-12-31'beforeOrOnDate=9999-12-31T00:00:00.000ZBETWEEN date '2020-01-01' AND date '9999-12-31'<= date '2262-04-11'Tests
test_filter_date_beyond_nanosecond_range_is_pushed_down— 2262-04-12 and 9999-12-31 push down with the date intacttest_filter_date_at_nanosecond_range_boundary_is_pushed_down— 2262-04-11 still pushes down, so the guard does not over-rejecttest_filter_between_date_beyond_nanosecond_range— theBETWEENpath, which normalizes bounds separately and propagates errorstest_date32_representable_boundaries— ±106_751 accepted; ±106_752 andi32::MIN/MAXrejectedtest_string_literal_judged_through_its_date_cast— string-through-cast resolution, and that a non-temporal cast target is left alone780
cubesqllib 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.