Skip to content

Commit a224b00

Browse files
adriangbclaude
andcommitted
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 = <lit>`. 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) <noreply@anthropic.com>
1 parent 11d6e91 commit a224b00

2 files changed

Lines changed: 76 additions & 0 deletions

File tree

datafusion/expr-common/src/casts.rs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,19 @@ fn is_date_type(data_type: &DataType) -> bool {
7474
/// For example, `CAST(ts AS DATE) = DATE '2024-01-01'` means "any timestamp
7575
/// during that day", but unwrapping it to `ts = TIMESTAMP '2024-01-01
7676
/// 00:00:00'` matches only midnight.
77+
///
78+
/// An identity cast (`from_type == to_type`, e.g. `Date32 -> Date32`) never
79+
/// changes comparison semantics and is therefore not lossy. This has to be
80+
/// handled explicitly because `DataType::is_temporal()` is true for both
81+
/// `Date32` and `Date64`, so `is_date_type(from) && to.is_temporal()` would
82+
/// otherwise report an identity `Date -> Date` cast as lossy and block the
83+
/// rewrite. Note this is deliberately limited to *identical* types: a genuine
84+
/// `Date32 <-> Date64` cast changes units (days vs milliseconds) and must
85+
/// still be treated as lossy here.
7786
fn is_lossy_temporal_cast(from_type: &DataType, to_type: &DataType) -> bool {
87+
if from_type == to_type {
88+
return false;
89+
}
7890
(is_date_type(from_type) && to_type.is_temporal())
7991
|| (is_date_type(to_type) && from_type.is_temporal())
8092
}
@@ -760,6 +772,57 @@ mod tests {
760772
);
761773
}
762774

775+
#[test]
776+
fn test_try_cast_identity_date_allowed() {
777+
// An identity Date cast (e.g. `CAST(date_col AS DATE)` where the column
778+
// is already Date32) must fold: it never changes comparison semantics,
779+
// so `try_cast_literal_to_type` should return the same value rather than
780+
// treating it as a lossy temporal cast.
781+
expect_cast(
782+
ScalarValue::Date32(Some(19_723)),
783+
DataType::Date32,
784+
ExpectedCast::Value(ScalarValue::Date32(Some(19_723))),
785+
);
786+
787+
expect_cast(
788+
ScalarValue::Date64(Some(1_704_067_200_000)),
789+
DataType::Date64,
790+
ExpectedCast::Value(ScalarValue::Date64(Some(1_704_067_200_000))),
791+
);
792+
793+
// is_lossy_temporal_cast must classify an identity cast as non-lossy.
794+
assert!(!is_lossy_temporal_cast(
795+
&DataType::Date32,
796+
&DataType::Date32
797+
));
798+
assert!(!is_lossy_temporal_cast(
799+
&DataType::Date64,
800+
&DataType::Date64
801+
));
802+
}
803+
804+
#[test]
805+
fn test_try_cast_date32_date64_still_blocked() {
806+
// `Date32` counts days and `Date64` counts milliseconds, but
807+
// try_cast_numeric_literal uses mul = 1 for both, so a cross cast would
808+
// convert units wrongly. The identity short-circuit must NOT open this
809+
// up: Date32 <-> Date64 has to stay blocked.
810+
assert!(is_lossy_temporal_cast(&DataType::Date32, &DataType::Date64));
811+
assert!(is_lossy_temporal_cast(&DataType::Date64, &DataType::Date32));
812+
813+
expect_cast(
814+
ScalarValue::Date32(Some(1)),
815+
DataType::Date64,
816+
ExpectedCast::NoValue,
817+
);
818+
819+
expect_cast(
820+
ScalarValue::Date64(Some(86_400_000)),
821+
DataType::Date32,
822+
ExpectedCast::NoValue,
823+
);
824+
}
825+
763826
#[test]
764827
fn test_try_cast_to_type_unsupported() {
765828
// int64 to list

datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,18 @@ mod tests {
586586
assert_eq!(optimize_test(expr_lt, &schema), expected);
587587
}
588588

589+
#[test]
590+
/// An identity `CAST(date_col AS DATE)` on a Date32 column must fold away so
591+
/// the comparison is against the bare column, e.g. `date_col = DATE '...'`.
592+
fn test_unwrap_identity_date_cast() {
593+
let schema = expr_test_schema();
594+
// cast(date32_col as Date32) = Date32(19723) -> date32_col = Date32(19723)
595+
let lit_date = lit(ScalarValue::Date32(Some(19_723)));
596+
let expr_eq = cast(col("date32_col"), DataType::Date32).eq(lit_date.clone());
597+
let expected = col("date32_col").eq(lit_date);
598+
assert_eq!(optimize_test(expr_eq, &schema), expected);
599+
}
600+
589601
fn optimize_test(expr: Expr, schema: &DFSchemaRef) -> Expr {
590602
let simplifier = ExprSimplifier::new(
591603
SimplifyContext::builder()
@@ -608,6 +620,7 @@ mod tests {
608620
Field::new("c6", DataType::UInt32, false),
609621
Field::new("ts_nano_none", timestamp_nano_none_type(), false),
610622
Field::new("ts_nano_utf", timestamp_nano_utc_type(), false),
623+
Field::new("date32_col", DataType::Date32, false),
611624
Field::new("str1", DataType::Utf8, false),
612625
Field::new("largestr", DataType::LargeUtf8, false),
613626
Field::new("tag", dictionary_tag_type(), false),

0 commit comments

Comments
 (0)