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 4 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 |
Codecov Report❌ Patch coverage is 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
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.
| // 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!( |
There was a problem hiding this comment.
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:
-
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 (keepleft op rightwith the original bound, the way thecast_string_literal_exprfallthrough at line 1531 already does) reaches the same observed end state without the error. -
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 - DATE→DATEDIFFrewrite, which exists specifically because non-Postgres dialects returnINTERVALotherwise. 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())), |
There was a problem hiding this comment.
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:
- The wrapper can't render
Date64. Inwrapper.rsthe arm is commented out (// ScalarValue::Date64(_) => {}, line 2986), so it falls to the catch-all at line 3074 and returnsCan't generate SQL for literal: Date64(…). Any query where this filter is rendered as SQL byCubeScanWrapperrather than pushed down as a member now fails SQL generation; before the change the bound stayed aUtf8literal, which renders fine. The new test only covers the member-pushdown path. - The overflow can come back downstream.
Date64 → Timestamp(Nanosecond)ismillis * 1_000_000through the same unchecked arrow multiply, and253_402_214_400_000 * 1_000_000overflows. 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. Notedatetime_literal_is_representableagrees — it returnsfalsefor the very scalar this line produces, so the guard would reject it if it re-enteredevaluate_expr_stacked. - Non-day-aligned
Date64is out of Arrow spec. Arrow requiresDate64values to be evenly divisible by 86_400_000.parse_date_straccepts a time component, sodim_date0 <= '9999-12-31 12:00:00'yields a mid-dayDate64.Date64Array::value_as_datetimehappens to round-trip it correctly, so the pushdown value is right — but any kernel that treatsDate64as 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)?; |
There was a problem hiding this comment.
Minor, two notes on the new Date32 arm:
- The epoch is recomputed (and
?-ed on an infalliblefrom_ymd_opt) per call.NaiveDate::from_ymd_opt(1970, 1, 1).unwrap().num_days_from_ce()is the constant 719_163 — either aconst EPOCH_DAYS_FROM_CE: i32 = 719_163;orparsed.date_naive().signed_duration_since(epoch).num_days() as i32reads more directly than the CE-difference. - This arm is a behavior change beyond the overflow fix:
Date32targets previously went through the Arrow cast kernel, which is stricter thanparse_date_str. Strings the kernel rejected used to leave the comparison un-normalized; they now become aDate32with any time component silently dropped ('2020-01-01 15:30:00'→2020-01-01). That matches Postgres'::datesemantics so it's probably desirable, but it's untested and unmentioned in the doc comment — worth one line either way.
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.