jayzhan211 commented on code in PR #22906:
URL: https://github.com/apache/datafusion/pull/22906#discussion_r4218979964
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -124,48 +666,21 @@ fn is_lossy_temporal_cast(from_type: &DataType, to_type:
&DataType) -> bool {
|| (is_date_type(to_type) && from_type.is_temporal())
}
-/// Returns true when casting a timestamp from `from_type` to `to_type` loses
-/// timestamp precision.
-///
-/// This is used by comparison cast unwrapping to avoid rewrites such as
-/// `CAST(ts_ns AS timestamp(ms)) = lit_ms` -> `ts_ns = lit_ns`. The original
-/// predicate can match any nanosecond value in the same millisecond, while the
-/// rewritten predicate only matches the exact millisecond boundary.
-pub fn is_timestamp_precision_narrowing_cast(
- from_type: &DataType,
- to_type: &DataType,
-) -> bool {
- let (DataType::Timestamp(from_unit, _), DataType::Timestamp(to_unit, _)) =
- (from_type, to_type)
- else {
- return false;
- };
-
- timestamp_unit_scale(from_unit) > timestamp_unit_scale(to_unit)
-}
-
/// Returns true when casting a date column from `from_type` to `to_type`
narrows
/// `Date64` (milliseconds) to `Date32` (days).
///
-/// Like [`is_timestamp_precision_narrowing_cast`], this guards comparison cast
-/// unwrapping against a many-to-one column cast. `CAST(date64 AS Date32) =
lit_day`
-/// matches any millisecond within that day, but the rewritten `date64 =
lit_ms`
-/// matches only midnight. Arrow does not require `Date64` values to be whole
days
-/// (see arrow-rs#5288), so the column may carry sub-day values the planner
cannot
-/// see; the widening direction (`Date32 -> Date64`) is injective and stays
allowed.
+/// `CAST(date64 AS Date32) = lit_day` matches any millisecond within that day,
+/// but the rewritten `date64 = lit_ms` matches only midnight. Arrow does not
+/// require `Date64` values to be whole days (see arrow-rs#5288), so the column
+/// may carry sub-day values the planner cannot see; the widening direction
+/// (`Date32 -> Date64`) is injective and stays allowed.
+///
+/// This pair is already rejected by the `is_exact_cast_safe` allowlist
+/// (including when wrapped in a `Dictionary`), so predicate rewrites do not
need
+/// a separate `Date64 -> Date32` early return.
pub fn is_date_narrowing_cast(from_type: &DataType, to_type: &DataType) ->
bool {
Review Comment:
```suggestion
#[deprecated(
since = "56.0.0",
note = "Date64 -> Date32 is rejected by the cast_predicate_preimage
allowlist"
)]
pub fn is_date_narrowing_cast(from_type: &DataType, to_type: &DataType) ->
bool {
```
##########
datafusion/optimizer/src/simplify_expressions/cast_preimage.rs:
##########
@@ -0,0 +1,577 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Preimage rewrites for cast comparisons.
+//!
+//! This module computes source-domain predicates for expressions such as
+//! `CAST(expr AS target_type) OP literal`. For casts that are many-to-one, a
+//! same-operator unwrap is not equivalent; the correct rewrite is a preimage
+//! range over the input expression.
+
+use arrow::datatypes::DataType;
+use datafusion_common::{Result, internal_err, tree_node::Transformed};
+use datafusion_expr::expr::InList;
+use datafusion_expr::{
+ BinaryExpr, Cast, Expr, Operator, TryCast, lit, simplify::SimplifyContext,
+};
+use datafusion_expr_common::casts::{
+ CastPredicatePreimage, cast_predicate_preimage, exact_preimage_cast,
+};
+
+use super::udf_preimage::rewrite_with_preimage;
+
+pub(super) fn rewrite_cast_predicate_for_binary(
+ info: &SimplifyContext,
+ cast_expr: Expr,
+ literal: Expr,
+ op: Operator,
+) -> Result<Transformed<Expr>> {
+ let Some((expr, target_type)) = cast_input_and_type(cast_expr) else {
+ return internal_err!("Expect cast expr");
+ };
+ let Expr::Literal(lit_value, _) = literal else {
+ return internal_err!("Expect literal expr");
+ };
+
+ let source_type = info.get_data_type(&expr)?;
+ match cast_predicate_preimage(&source_type, &target_type, op, &lit_value)?
{
+ Some(CastPredicatePreimage::Range(interval)) => {
+ rewrite_with_preimage(interval, op, *expr)
+ }
+ Some(CastPredicatePreimage::Exact(value)) => {
+ Ok(Transformed::yes(Expr::BinaryExpr(BinaryExpr {
+ left: expr,
+ op,
+ right: Box::new(lit(value)),
+ })))
+ }
+ None => internal_err!(
+ "Can't compute cast predicate preimage for source type {} target
type {} literal {:?}",
+ source_type,
+ target_type,
+ lit_value
+ ),
+ }
+}
+
+pub(super) fn supports_cast_predicate_for_binary(
+ info: &SimplifyContext,
+ expr: &Expr,
+ op: Operator,
+ literal: &Expr,
+) -> bool {
+ if !matches!(
+ op,
+ Operator::Eq
+ | Operator::NotEq
+ | Operator::Lt
+ | Operator::LtEq
+ | Operator::Gt
+ | Operator::GtEq
+ | Operator::IsDistinctFrom
+ | Operator::IsNotDistinctFrom
+ ) {
+ return false;
+ }
+
+ let Some((inner_expr, target_type)) = cast_input_and_type_ref(expr) else {
+ return false;
+ };
+ let Expr::Literal(lit_value, _) = literal else {
+ return false;
+ };
+ let Ok(source_type) = info.get_data_type(inner_expr) else {
+ return false;
+ };
+
+ let Ok(preimage) = cast_predicate_preimage(&source_type, target_type, op,
lit_value)
+ else {
+ return false;
+ };
+ if preimage.is_none() {
+ return false;
+ }
+ // Equality-like range predicates duplicate their input; volatile
expressions
+ // cannot be duplicated.
+ !(matches!(preimage, Some(CastPredicatePreimage::Range(_)))
+ && matches!(
+ op,
+ Operator::Eq
+ | Operator::NotEq
+ | Operator::IsDistinctFrom
+ | Operator::IsNotDistinctFrom
+ )
+ && inner_expr.is_volatile())
Review Comment:
```suggestion
// Volatile expressions cannot be duplicated.
preimage.is_some_and(|p| !(p.duplicates_input(op) &&
inner_expr.is_volatile()))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
