From a224b008fc4ef07d1a5a3b5aeee6b4a1837d4a33 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Mon, 20 Jul 2026 13:05:02 -0500 Subject: [PATCH] fix: unwrap identity Date cast in comparison unwrapping `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. 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 arrow's `DataType::is_temporal()` is true for `Date32`/`Date64`. The cast is wrongly classified 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. `Date32` counts days and `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. Tests: identity `Date32 -> Date32` / `Date64 -> Date64` now fold; `Date32 <-> Date64` still does not; plus an end-to-end simplifier test that `cast(date_col AS DATE) = DATE '...'` simplifies to `date_col = `. apache/datafusion `main` carries the same `is_lossy_temporal_cast`, so this is also a candidate to upstream. Co-Authored-By: Claude Opus 4.8 (1M context) --- datafusion/expr-common/src/casts.rs | 63 +++++++++++++++++++ .../src/simplify_expressions/unwrap_cast.rs | 13 ++++ 2 files changed, 76 insertions(+) diff --git a/datafusion/expr-common/src/casts.rs b/datafusion/expr-common/src/casts.rs index dad589e4bfe9f..79227aec19bfa 100644 --- a/datafusion/expr-common/src/casts.rs +++ b/datafusion/expr-common/src/casts.rs @@ -74,7 +74,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()) } @@ -760,6 +772,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_try_cast_to_type_unsupported() { // int64 to list diff --git a/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs b/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs index a5b65d0d8e7a4..401a68e3edfc6 100644 --- a/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs +++ b/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs @@ -586,6 +586,18 @@ mod tests { assert_eq!(optimize_test(expr_lt, &schema), expected); } + #[test] + /// An identity `CAST(date_col AS DATE)` on a Date32 column must fold away so + /// the comparison is against the bare column, e.g. `date_col = DATE '...'`. + fn test_unwrap_identity_date_cast() { + let schema = expr_test_schema(); + // cast(date32_col as Date32) = Date32(19723) -> date32_col = Date32(19723) + let lit_date = lit(ScalarValue::Date32(Some(19_723))); + let expr_eq = cast(col("date32_col"), DataType::Date32).eq(lit_date.clone()); + let expected = col("date32_col").eq(lit_date); + assert_eq!(optimize_test(expr_eq, &schema), expected); + } + fn optimize_test(expr: Expr, schema: &DFSchemaRef) -> Expr { let simplifier = ExprSimplifier::new( SimplifyContext::builder() @@ -608,6 +620,7 @@ mod tests { Field::new("c6", DataType::UInt32, false), Field::new("ts_nano_none", timestamp_nano_none_type(), false), Field::new("ts_nano_utf", timestamp_nano_utc_type(), false), + Field::new("date32_col", DataType::Date32, false), Field::new("str1", DataType::Utf8, false), Field::new("largestr", DataType::LargeUtf8, false), Field::new("tag", dictionary_tag_type(), false),