Skip to content

fix: unwrap identity Date cast in comparison unwrapping#23727

Open
adriangb wants to merge 3 commits into
apache:mainfrom
pydantic:fix-identity-date-cast-unwrap-upstream
Open

fix: unwrap identity Date cast in comparison unwrapping#23727
adriangb wants to merge 3 commits into
apache:mainfrom
pydantic:fix-identity-date-cast-unwrap-upstream

Conversation

@adriangb

@adriangb adriangb commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

No existing issue; this PR both reports and fixes the bug. Happy to file a tracking issue if preferred.

Rationale for this change

unwrap_cast_in_comparison fails to fold an identity CAST(col AS DATE) on a Date32 column. Instead of rewriting the predicate to compare against the bare column, it leaves a residual Cast(col AS Date32) in place, which defeats downstream optimizations (pruning / filter pushdown) that expect a bare-column comparison.

Reproduction (logical plan for SELECT * FROM t WHERE cast(d AS date) = DATE '2024-01-01', where d is Date32): the cast(d AS Date32) survives simplification rather than collapsing to d. Note arrow_cast(d, 'Date32') already folds, because the arrow_cast UDF's simplify() short-circuits an identity cast; the SQL CAST ... AS date planner plants a real Expr::Cast with no such elision, so it reaches try_cast_literal_to_type and is wrongly rejected.

The root cause is in is_lossy_temporal_cast (datafusion/expr-common/src/casts.rs):

(is_date_type(from_type) && to_type.is_temporal())
    || (is_date_type(to_type) && from_type.is_temporal())

For an identity Date32 -> Date32 cast this evaluates to true && true, because DataType::is_temporal() is true for both Date32 and Date64. The identity cast is therefore misclassified as a lossy temporal cast, try_cast_literal_to_type returns None, and unwrap_cast_in_comparison leaves the cast in the plan.

What changes are included in this PR?

Short-circuit an identity cast as non-lossy at the top of is_lossy_temporal_cast:

if from_type == to_type {
    return false; // an identity cast never changes comparison semantics
}

This is deliberately limited to identical types, not "any date-to-date". Date32 counts days while Date64 counts milliseconds, but try_cast_numeric_literal uses mul = 1 for both, so allowing a Date32 <-> Date64 unwrap would convert units incorrectly. The from_type == to_type identity guard is the exact correct scope, and Date32 <-> Date64 remains blocked.

Are these changes tested?

Yes. The PR is structured as a test-driven, stacked sequence so the effect of the fix is visible in the diff:

  1. test: characterize identity Date cast in comparison unwrapping — adds an SLT test to simplify_expr.slt (explain select d from dates where cast(d as date) = DATE '2024-01-01') whose assertion records the current, buggy output: the logical plan keeps the residual Filter: CAST(dates.d AS Date32) = Date32(...). Passes on main.

  2. fix: unwrap identity Date cast in comparison unwrapping — the production change plus two expr-common unit tests:

    • test_try_cast_identity_date_allowed — identity Date32 -> Date32 / Date64 -> Date64 now fold (try_cast_literal_to_type returns Some), and is_lossy_temporal_cast reports them non-lossy.
    • test_try_cast_date32_date64_still_blockedDate32 <-> Date64 stays lossy/blocked (guards the units caveat above, which SLT can't easily express).

    At this commit the SLT test is intentionally red, proving it exercises the bug.

  3. test: update identity Date cast assertion to folded plan — updates the SLT assertion to the corrected Filter: dates.d = Date32(...). The one-line diff is the observable effect of the fix.

cargo fmt --check and cargo clippy -D warnings are clean on the changed crate; datafusion-expr-common and the simplify_expr sqllogictest pass.

Are there any user-facing changes?

No API changes. Queries with CAST(date_col AS DATE) predicates on Date32 columns simplify to bare-column comparisons, which can enable additional pruning/pushdown. No behavioral change to query results.

@codecov-commenter

codecov-commenter commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.70%. Comparing base (5b65e70) to head (0ec7b72).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #23727   +/-   ##
=======================================
  Coverage   80.70%   80.70%           
=======================================
  Files        1089     1089           
  Lines      368137   368170   +33     
  Branches   368137   368170   +33     
=======================================
+ Hits       297121   297149   +28     
- Misses      53311    53313    +2     
- Partials    17705    17708    +3     

☔ 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.

adriangb added 3 commits July 20, 2026 19:13
Add an SLT test to `simplify_expr.slt` for the predicate
`cast(d AS date) = DATE '...'` where `d` is a `Date32` column. This is an
identity cast that should fold away, leaving a bare-column comparison.

This commit records the *current* (buggy) behavior: the logical plan retains
a residual `CAST(dates.d AS Date32)` instead of simplifying to `dates.d =
Date32(...)`. The residual cast defeats downstream pruning / filter pushdown
that expects a bare-column comparison. The next commit fixes this and this
test's assertion is updated to the corrected plan, making the fix's effect
visible in the diff.

Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
`unwrap_cast_in_comparison` refused to fold an identity `CAST(col AS DATE)`
on a `Date32` column, leaving a residual `Cast(col AS Date32)` in the
predicate instead of comparing against the bare column. SQL `cast(col AS
date)` plants a real `Expr::Cast` with no identity elision (unlike the
`arrow_cast` UDF, whose `simplify()` short-circuits identity), so this cast
reached `try_cast_literal_to_type` and was rejected. The residual cast
defeats downstream pruning/pushdown that expects a bare-column comparison.

The root cause is in `is_lossy_temporal_cast`:

    (is_date_type(from) && to.is_temporal())
        || (is_date_type(to) && from.is_temporal())

For an identity `Date32 -> Date32` cast this is `true && true`, because
`DataType::is_temporal()` is true for both `Date32` and `Date64`. The cast
is misclassified as a lossy temporal cast, `try_cast_literal_to_type`
returns `None`, and the residual cast is left in place.

Fix: short-circuit an identity cast (`from_type == to_type`) as non-lossy at
the top of `is_lossy_temporal_cast`. An identity cast can never change
comparison semantics.

This is deliberately scoped to *identical* types only, not "any date-to-date".
`Date32` counts days while `Date64` counts milliseconds, but
`try_cast_numeric_literal` uses `mul = 1` for both, so a `Date32 <-> Date64`
cast would convert units wrongly and must stay blocked. A regression test
asserts that.

The SLT characterization test added in the previous commit now fails (its
assertion still shows the residual cast); the next commit updates it to the
corrected plan, so the diff shows exactly what the fix changed.

Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
With the identity short-circuit in `is_lossy_temporal_cast`, the predicate
`cast(d AS date) = DATE '...'` on a `Date32` column now folds the identity
cast away. Update the SLT assertion from the residual

    Filter: CAST(dates.d AS Date32) = Date32("2024-01-01")

to the corrected bare-column comparison

    Filter: dates.d = Date32("2024-01-01")

The diff of this commit is the observable effect of the fix.

Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
@adriangb
adriangb force-pushed the fix-identity-date-cast-unwrap-upstream branch from 8a5e75d to 0ec7b72 Compare July 21, 2026 00:17
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) and removed optimizer Optimizer rules labels Jul 21, 2026
@adriangb
adriangb requested a review from kosiew July 21, 2026 00:37
@adriangb

Copy link
Copy Markdown
Contributor Author

@kosiew another hopefully small one to look at

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

Labels

logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants