Unwrap widening Date32 -> Date64 casts in comparison predicates - #23729
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23729 +/- ##
==========================================
+ Coverage 80.77% 80.78% +0.01%
==========================================
Files 1089 1089
Lines 368938 369200 +262
Branches 368938 369200 +262
==========================================
+ Hits 297992 298246 +254
- Misses 53197 53199 +2
- Partials 17749 17755 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bb05532 to
d27aa7f
Compare
|
@kosiew wonder if you could help take a look at this change? |
| 1 | ||
| 2 | ||
|
|
||
| # Range operators fold too (Date32 -> Date64 is monotonic). |
There was a problem hiding this comment.
Could we add one widening comparison with the Date64 literal on the left, for example <date64 literal> < CAST(d32 AS Date64)? It would be useful to verify that logical simplification moves the column to the left and swaps the operator correctly. The existing cases cover the new conversion behavior with the cast on the left, while this would add Date-specific coverage for the reversed-operand path. The physical operator-swap path already has generic unit coverage.
Add a sqllogictest block that pins the current behavior of comparison-cast unwrapping for the two date types, before any optimizer change. Today DataFusion does not unwrap either direction: - widening `CAST(date32 AS Date64) <op> <date64 literal>` keeps the cast on the column, and - narrowing `CAST(date64 AS Date32) = <date literal>` keeps the cast on the column (correctly, since it truncates milliseconds to the day). The table intentionally stores sub-day `Date64` values (arrow-rs does not enforce whole-day `Date64`, see arrow-rs#5288) and pre-epoch dates so the follow-up change can be shown to preserve every result row while only the widening plan changes. All queries pass on unmodified `main`. Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Teach comparison-cast unwrapping to fold a widening `Date32 -> Date64` column cast, which was previously declined outright. `try_cast_literal_to_type` could not convert between the two date types: it scaled both `Date32` and `Date64` literals by the same target multiplier (`mul = 1`), but `Date32` counts days and `Date64` counts milliseconds, so a cross conversion needs a factor of `MILLISECONDS_IN_DAY` (86_400_000). `is_lossy_temporal_cast` also pre-classified every `Date <-> temporal` pair as lossy, which swept in `Date32 <-> Date64`. Changes: - Relax `is_lossy_temporal_cast` so a date-to-date (and identity) cast is not pre-filtered as lossy; per-value exactness is enforced downstream. - Add `scale_date_literal`, which scales a `Date32`/`Date64` literal into the target unit with exact-only semantics: `Date32 -> Date64` multiplies by `MILLISECONDS_IN_DAY` (overflow-guarded); `Date64 -> Date32` divides only on a whole-day boundary and otherwise returns `None`, mirroring the existing Decimal scaling path. - Add `is_date_narrowing_cast` and block a narrowing `Date64 -> Date32` column cast in the comparison and in-list gates of the logical optimizer, and in the physical-expr simplifier, mirroring `is_timestamp_precision_narrowing_cast`. Narrowing truncates milliseconds to the day (many-to-one), so unwrapping it would drop sub-day rows; arrow-rs permits out-of-spec sub-day `Date64` values (arrow-rs#5288), so the column may carry values the planner cannot see. Pure-function unit tests cover `scale_date_literal` exactness and overflow, `is_date_narrowing_cast`, and the relaxed `is_lossy_temporal_cast` date-pair behavior. The user-visible behavior is characterized in `simplify_expr.slt`, whose widening-plan expectations now fail intentionally; the next commit updates them. Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Regenerate the sqllogictest expectations now that widening `Date32 -> Date64` cast comparisons are unwrapped. The diff is the user-visible behavior change: each widening query's plan flips from `CAST(d32 AS Date64) <op> Date64(..)` to `d32 <op> Date32(..)` (the in-list form folds onto each element). Every result row is unchanged, and every narrowing `CAST(date64 AS Date32)` plan still keeps the cast on the column, so no query changes its output. Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Add a widening case with the Date64 literal on the left (`literal < CAST(d32 AS Date64)`) to verify logical simplification moves the bare column to the left and swaps the operator (`< -> >`). Requested in review to give the reversed-operand path Date-specific coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
73ff3d1 to
91255d3
Compare
| } | ||
|
|
||
| #[test] | ||
| fn test_try_cast_date32_date64_still_blocked() { |
|
@kosiew I rebased and resolved conflcits with #23727, leaving a note in #23729 (comment) about the conflicts. Could you take a look and leave confirmation that you're still 👍🏻 on this change before I merge it? |
…he#23729) ## Which issue does this PR close? - N/A. Small, self-contained enhancement to `unwrap_cast_in_comparison`; happy to file a tracking issue if preferred. ## Rationale for this change `unwrap_cast_in_comparison` could not unwrap a cast between the two date types, so a predicate like `CAST(date32_col AS Date64) <op> <date64 literal>` never folded onto the bare column. Folding it lets the literal be compared against the unmodified column, keeping predicate pushdown / pruning effective on date columns. There were two coupled causes in `try_cast_literal_to_type` (`datafusion/expr-common/src/casts.rs`): 1. `try_cast_numeric_literal` scaled both `Date32` and `Date64` literals by the same target multiplier (`mul = 1`). But `Date32` counts **days** since the epoch while `Date64` counts **milliseconds**, so a cross conversion needs a factor of `MILLISECONDS_IN_DAY` (86_400_000). 2. `is_lossy_temporal_cast` classified every `Date <-> temporal` pair as lossy, which swept in `Date32 <-> Date64` and blocked the unwrap outright. The reverse direction is subtle and unsound if handled naively: narrowing a `Date64` **column** down to `Date32` truncates milliseconds to the day (many-to-one), so `CAST(date64 AS Date32) = <day>` matches any millisecond within that day. arrow-rs does not require `Date64` values to be whole-day (apache/arrow-rs#5288), so the column may carry sub-day values the planner cannot see, and unwrapping would drop those rows. That direction is therefore explicitly blocked. ## What changes are included in this PR? - Relax `is_lossy_temporal_cast` so a date-to-date (and identity) cast is not pre-classified as lossy; per-value exactness is enforced downstream. - Add `scale_date_literal` with exact-only semantics: `Date32 -> Date64` multiplies by `MILLISECONDS_IN_DAY` (overflow-guarded with checked arithmetic); `Date64 -> Date32` divides only on a whole-day boundary and otherwise returns `None`. This mirrors the existing Decimal scaling path in the same function. - Add `is_date_narrowing_cast` and block the narrowing `Date64 -> Date32` column cast in the two logical-optimizer gates (comparison and in-list) **and** in the physical-expr simplifier, mirroring `is_timestamp_precision_narrowing_cast`. The physical-expr guard is required for soundness on the pruning / row-group-filter path (verified by a unit test that fails without it); the widening `Date32 -> Date64` column cast is injective and stays supported. Scope is intentionally limited to `Date32 <-> Date64` scaling, the narrowing gate, and tests. ## Are these changes tested? Yes, at two levels. **End-to-end (`datafusion/sqllogictest/test_files/simplify_expr.slt`)** — the PR is structured as three commits so the behavior change is legible in the diff: 1. `test:` characterizes current behavior (passes on unmodified `main`): neither direction is unwrapped, results are correct. The fixture stores sub-day and pre-epoch `Date64` values on purpose. 2. `feat:` applies only the code change; the recorded widening `EXPLAIN` plans now fail intentionally. 3. `test:` regenerates the expectations. The commit-3 diff is exactly the widening plans flipping from `CAST(d32 AS Date64) <op> Date64(..)` to `d32 <op> Date32(..)`; **every result row and every narrowing plan is byte-identical**, which is the soundness proof. Coverage includes `=`/`<`/`<=`/`>`/`>=`/`IN`, whole-day vs sub-day literals (the latter yields zero rows and is left as-is), the narrowing soundness case (the noon row is still returned), pre-epoch dates (arrow's toward-zero truncation is pinned), and NULL three-valued logic. **Unit** — `scale_date_literal` exactness and `i32::MIN`/`i32::MAX` overflow, `is_date_narrowing_cast`, the relaxed `is_lossy_temporal_cast` date-pair behavior, and the physical-expr narrowing guard. `cargo fmt --check` is clean, `cargo clippy -p datafusion-expr-common -- -D warnings` passes, and the full sqllogictest suite passes. ## Are there any user-facing changes? No public API changes. The optimizer now additionally rewrites widening `Date32 -> Date64` cast comparisons where it previously left them untouched; results are unchanged, plans are simplified. Narrowing `Date64 -> Date32` cast comparisons are deliberately left as-is. ## Note for reviewers The open PR apache#23727 adds the `if from_type == to_type { return false }` identity guard to this same function. This PR is based independently on `main` and includes that identity line as part of the clean gate shape here, so depending on merge order the two may need a trivial rebase where those lines overlap. --------- Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Which issue does this PR close?
unwrap_cast_in_comparison; happy to file a tracking issue if preferred.Rationale for this change
unwrap_cast_in_comparisoncould not unwrap a cast between the two date types, so a predicate likeCAST(date32_col AS Date64) <op> <date64 literal>never folded onto the bare column. Folding it lets the literal be compared against the unmodified column, keeping predicate pushdown / pruning effective on date columns.There were two coupled causes in
try_cast_literal_to_type(datafusion/expr-common/src/casts.rs):try_cast_numeric_literalscaled bothDate32andDate64literals by the same target multiplier (mul = 1). ButDate32counts days since the epoch whileDate64counts milliseconds, so a cross conversion needs a factor ofMILLISECONDS_IN_DAY(86_400_000).is_lossy_temporal_castclassified everyDate <-> temporalpair as lossy, which swept inDate32 <-> Date64and blocked the unwrap outright.The reverse direction is subtle and unsound if handled naively: narrowing a
Date64column down toDate32truncates milliseconds to the day (many-to-one), soCAST(date64 AS Date32) = <day>matches any millisecond within that day. arrow-rs does not requireDate64values to be whole-day (apache/arrow-rs#5288), so the column may carry sub-day values the planner cannot see, and unwrapping would drop those rows. That direction is therefore explicitly blocked.What changes are included in this PR?
is_lossy_temporal_castso a date-to-date (and identity) cast is not pre-classified as lossy; per-value exactness is enforced downstream.scale_date_literalwith exact-only semantics:Date32 -> Date64multiplies byMILLISECONDS_IN_DAY(overflow-guarded with checked arithmetic);Date64 -> Date32divides only on a whole-day boundary and otherwise returnsNone. This mirrors the existing Decimal scaling path in the same function.is_date_narrowing_castand block the narrowingDate64 -> Date32column cast in the two logical-optimizer gates (comparison and in-list) and in the physical-expr simplifier, mirroringis_timestamp_precision_narrowing_cast. The physical-expr guard is required for soundness on the pruning / row-group-filter path (verified by a unit test that fails without it); the wideningDate32 -> Date64column cast is injective and stays supported.Scope is intentionally limited to
Date32 <-> Date64scaling, the narrowing gate, and tests.Are these changes tested?
Yes, at two levels.
End-to-end (
datafusion/sqllogictest/test_files/simplify_expr.slt) — the PR is structured as three commits so the behavior change is legible in the diff:test:characterizes current behavior (passes on unmodifiedmain): neither direction is unwrapped, results are correct. The fixture stores sub-day and pre-epochDate64values on purpose.feat:applies only the code change; the recorded wideningEXPLAINplans now fail intentionally.test:regenerates the expectations. The commit-3 diff is exactly the widening plans flipping fromCAST(d32 AS Date64) <op> Date64(..)tod32 <op> Date32(..); every result row and every narrowing plan is byte-identical, which is the soundness proof. Coverage includes=/</<=/>/>=/IN, whole-day vs sub-day literals (the latter yields zero rows and is left as-is), the narrowing soundness case (the noon row is still returned), pre-epoch dates (arrow's toward-zero truncation is pinned), and NULL three-valued logic.Unit —
scale_date_literalexactness andi32::MIN/i32::MAXoverflow,is_date_narrowing_cast, the relaxedis_lossy_temporal_castdate-pair behavior, and the physical-expr narrowing guard.cargo fmt --checkis clean,cargo clippy -p datafusion-expr-common -- -D warningspasses, and the full sqllogictest suite passes.Are there any user-facing changes?
No public API changes. The optimizer now additionally rewrites widening
Date32 -> Date64cast comparisons where it previously left them untouched; results are unchanged, plans are simplified. NarrowingDate64 -> Date32cast comparisons are deliberately left as-is.Note for reviewers
The open PR #23727 adds the
if from_type == to_type { return false }identity guard to this same function. This PR is based independently onmainand includes that identity line as part of the clean gate shape here, so depending on merge order the two may need a trivial rebase where those lines overlap.