Skip to content

Commit 8a5e75d

Browse files
committed
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. 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. 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>`. Signed-off-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
1 parent 67947b6 commit 8a5e75d

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
@@ -98,7 +98,19 @@ fn is_date_type(data_type: &DataType) -> bool {
9898
/// For example, `CAST(ts AS DATE) = DATE '2024-01-01'` means "any timestamp
9999
/// during that day", but unwrapping it to `ts = TIMESTAMP '2024-01-01
100100
/// 00:00:00'` matches only midnight.
101+
///
102+
/// An identity cast (`from_type == to_type`, e.g. `Date32 -> Date32`) never
103+
/// changes comparison semantics and is therefore not lossy. This has to be
104+
/// handled explicitly because `DataType::is_temporal()` is true for both
105+
/// `Date32` and `Date64`, so `is_date_type(from) && to.is_temporal()` would
106+
/// otherwise report an identity `Date -> Date` cast as lossy and block the
107+
/// rewrite. Note this is deliberately limited to *identical* types: a genuine
108+
/// `Date32 <-> Date64` cast changes units (days vs milliseconds) and must
109+
/// still be treated as lossy here.
101110
fn is_lossy_temporal_cast(from_type: &DataType, to_type: &DataType) -> bool {
111+
if from_type == to_type {
112+
return false;
113+
}
102114
(is_date_type(from_type) && to_type.is_temporal())
103115
|| (is_date_type(to_type) && from_type.is_temporal())
104116
}
@@ -813,6 +825,57 @@ mod tests {
813825
);
814826
}
815827

828+
#[test]
829+
fn test_try_cast_identity_date_allowed() {
830+
// An identity Date cast (e.g. `CAST(date_col AS DATE)` where the column
831+
// is already Date32) must fold: it never changes comparison semantics,
832+
// so `try_cast_literal_to_type` should return the same value rather than
833+
// treating it as a lossy temporal cast.
834+
expect_cast(
835+
ScalarValue::Date32(Some(19_723)),
836+
DataType::Date32,
837+
ExpectedCast::Value(ScalarValue::Date32(Some(19_723))),
838+
);
839+
840+
expect_cast(
841+
ScalarValue::Date64(Some(1_704_067_200_000)),
842+
DataType::Date64,
843+
ExpectedCast::Value(ScalarValue::Date64(Some(1_704_067_200_000))),
844+
);
845+
846+
// is_lossy_temporal_cast must classify an identity cast as non-lossy.
847+
assert!(!is_lossy_temporal_cast(
848+
&DataType::Date32,
849+
&DataType::Date32
850+
));
851+
assert!(!is_lossy_temporal_cast(
852+
&DataType::Date64,
853+
&DataType::Date64
854+
));
855+
}
856+
857+
#[test]
858+
fn test_try_cast_date32_date64_still_blocked() {
859+
// `Date32` counts days and `Date64` counts milliseconds, but
860+
// try_cast_numeric_literal uses mul = 1 for both, so a cross cast would
861+
// convert units wrongly. The identity short-circuit must NOT open this
862+
// up: Date32 <-> Date64 has to stay blocked.
863+
assert!(is_lossy_temporal_cast(&DataType::Date32, &DataType::Date64));
864+
assert!(is_lossy_temporal_cast(&DataType::Date64, &DataType::Date32));
865+
866+
expect_cast(
867+
ScalarValue::Date32(Some(1)),
868+
DataType::Date64,
869+
ExpectedCast::NoValue,
870+
);
871+
872+
expect_cast(
873+
ScalarValue::Date64(Some(86_400_000)),
874+
DataType::Date32,
875+
ExpectedCast::NoValue,
876+
);
877+
}
878+
816879
#[test]
817880
fn test_timestamp_precision_narrowing_cast() {
818881
let ts_ns = DataType::Timestamp(TimeUnit::Nanosecond, None);

datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs

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

607+
#[test]
608+
/// An identity `CAST(date_col AS DATE)` on a Date32 column must fold away so
609+
/// the comparison is against the bare column, e.g. `date_col = DATE '...'`.
610+
fn test_unwrap_identity_date_cast() {
611+
let schema = expr_test_schema();
612+
// cast(date32_col as Date32) = Date32(19723) -> date32_col = Date32(19723)
613+
let lit_date = lit(ScalarValue::Date32(Some(19_723)));
614+
let expr_eq = cast(col("date32_col"), DataType::Date32).eq(lit_date.clone());
615+
let expected = col("date32_col").eq(lit_date);
616+
assert_eq!(optimize_test(expr_eq, &schema), expected);
617+
}
618+
607619
#[test]
608620
fn test_not_unwrap_cast_timestamp_precision_narrowing() {
609621
let schema = expr_test_schema();
@@ -646,6 +658,7 @@ mod tests {
646658
Field::new("ts_nano_none", timestamp_nano_none_type(), false),
647659
Field::new("ts_millis_none", timestamp_millis_none_type(), false),
648660
Field::new("ts_nano_utf", timestamp_nano_utc_type(), false),
661+
Field::new("date32_col", DataType::Date32, false),
649662
Field::new("str1", DataType::Utf8, false),
650663
Field::new("largestr", DataType::LargeUtf8, false),
651664
Field::new("tag", dictionary_tag_type(), false),

0 commit comments

Comments
 (0)