This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-23727-1de92c09176083bd08076090f5747daa9febf046 in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 0f2e137115327c2e355e27b4b1cf04de1fd223f3 Author: Adrian Garcia Badaracco <[email protected]> AuthorDate: Thu Jul 23 11:38:33 2026 -0500 fix: unwrap identity Date cast in comparison unwrapping (#23727) ## 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. <!-- - Closes #. --> ## 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`): ```rust (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`: ```rust 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_blocked` — `Date32 <-> 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. --------- Signed-off-by: Adrian Garcia Badaracco <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> --- datafusion/expr-common/src/casts.rs | 63 ++++++++++++++++++++++ .../sqllogictest/test_files/simplify_expr.slt | 33 ++++++++++++ 2 files changed, 96 insertions(+) diff --git a/datafusion/expr-common/src/casts.rs b/datafusion/expr-common/src/casts.rs index 320f7cec79..8c9616f7b8 100644 --- a/datafusion/expr-common/src/casts.rs +++ b/datafusion/expr-common/src/casts.rs @@ -98,7 +98,19 @@ fn is_date_type(data_type: &DataType) -> bool { /// For example, `CAST(ts AS DATE) = DATE '2024-01-01'` means "any timestamp /// during that day", but unwrapping it to `ts = TIMESTAMP '2024-01-01 /// 00:00:00'` matches only midnight. +/// +/// An identity cast (`from_type == to_type`, e.g. `Date32 -> Date32`) never +/// changes comparison semantics and is therefore not lossy. This has to be +/// handled explicitly because `DataType::is_temporal()` is true for both +/// `Date32` and `Date64`, so `is_date_type(from) && to.is_temporal()` would +/// otherwise report an identity `Date -> Date` cast as lossy and block the +/// rewrite. Note this is deliberately limited to *identical* types: a genuine +/// `Date32 <-> Date64` cast changes units (days vs milliseconds) and must +/// still be treated as lossy here. fn is_lossy_temporal_cast(from_type: &DataType, to_type: &DataType) -> bool { + if from_type == to_type { + return false; + } (is_date_type(from_type) && to_type.is_temporal()) || (is_date_type(to_type) && from_type.is_temporal()) } @@ -813,6 +825,57 @@ mod tests { ); } + #[test] + fn test_try_cast_identity_date_allowed() { + // An identity Date cast (e.g. `CAST(date_col AS DATE)` where the column + // is already Date32) must fold: it never changes comparison semantics, + // so `try_cast_literal_to_type` should return the same value rather than + // treating it as a lossy temporal cast. + expect_cast( + ScalarValue::Date32(Some(19_723)), + DataType::Date32, + ExpectedCast::Value(ScalarValue::Date32(Some(19_723))), + ); + + expect_cast( + ScalarValue::Date64(Some(1_704_067_200_000)), + DataType::Date64, + ExpectedCast::Value(ScalarValue::Date64(Some(1_704_067_200_000))), + ); + + // is_lossy_temporal_cast must classify an identity cast as non-lossy. + assert!(!is_lossy_temporal_cast( + &DataType::Date32, + &DataType::Date32 + )); + assert!(!is_lossy_temporal_cast( + &DataType::Date64, + &DataType::Date64 + )); + } + + #[test] + fn test_try_cast_date32_date64_still_blocked() { + // `Date32` counts days and `Date64` counts milliseconds, but + // try_cast_numeric_literal uses mul = 1 for both, so a cross cast would + // convert units wrongly. The identity short-circuit must NOT open this + // up: Date32 <-> Date64 has to stay blocked. + assert!(is_lossy_temporal_cast(&DataType::Date32, &DataType::Date64)); + assert!(is_lossy_temporal_cast(&DataType::Date64, &DataType::Date32)); + + expect_cast( + ScalarValue::Date32(Some(1)), + DataType::Date64, + ExpectedCast::NoValue, + ); + + expect_cast( + ScalarValue::Date64(Some(86_400_000)), + DataType::Date32, + ExpectedCast::NoValue, + ); + } + #[test] fn test_timestamp_precision_narrowing_cast() { let ts_ns = DataType::Timestamp(TimeUnit::Nanosecond, None); diff --git a/datafusion/sqllogictest/test_files/simplify_expr.slt b/datafusion/sqllogictest/test_files/simplify_expr.slt index 58ec7a1b26..a291740b91 100644 --- a/datafusion/sqllogictest/test_files/simplify_expr.slt +++ b/datafusion/sqllogictest/test_files/simplify_expr.slt @@ -146,3 +146,36 @@ logical_plan physical_plan 01)ProjectionExec: expr=[column1@0 = 1 as opt1, column1@0 = 2 AND column1@0 != 2 as noopt1, column1@0 = 4 as opt2, column1@0 != 5 AND column1@0 = 5 as noopt2] 02)--DataSourceExec: partitions=1, partition_sizes=[1] + +# Identity Date cast in a comparison predicate. +# `cast(d AS date)` where `d` is already Date32 is an identity cast and should +# fold away, so the predicate compares against the bare column `d`. This enables +# downstream pruning / filter pushdown that expects a bare-column comparison. +statement ok +create table dates(d date) as values (DATE '2024-01-01'), (DATE '2024-01-02'); + +query TT +explain select d from dates where cast(d as date) = DATE '2024-01-01'; +---- +logical_plan +01)Filter: dates.d = Date32("2024-01-01") +02)--TableScan: dates projection=[d] +physical_plan +01)FilterExec: d@0 = 2024-01-01 +02)--DataSourceExec: partitions=1, partition_sizes=[1] + +# Identity Date cast inside an `IN` predicate. `IN` goes through a separate +# validation and rewrite path but relies on the same literal-cast helper, so the +# identity `cast(d AS date)` should likewise fold to a bare-column comparison. +query TT +explain select d from dates where cast(d as date) in (DATE '2024-01-01'); +---- +logical_plan +01)Filter: dates.d = Date32("2024-01-01") +02)--TableScan: dates projection=[d] +physical_plan +01)FilterExec: d@0 = 2024-01-01 +02)--DataSourceExec: partitions=1, partition_sizes=[1] + +statement ok +drop table dates; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
