kosiew commented on code in PR #26982:
URL: https://github.com/apache/datafusion/pull/26982#discussion_r4451994888
##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -1349,10 +1349,7 @@ fn verify_support_type_for_prune(from_type: &DataType,
to_type: &DataType) -> Re
}
_ => to_type,
};
- // If both types are strings or both are not strings (number, timestamp,
etc)
- // then we can compare them.
- // PruningPredicate does not support casting of strings to numbers and
such.
- if from_type.is_string() == to_type.is_string() {
+ if phys_expr::is_order_preserving_cast(from_type, to_type) {
Review Comment:
[P2] The new rule correctly rejects `CAST(Int32 AS FixedSizeBinary(4))`, but
`bloom_filter_pruning_error_retains_row_group_without_match` relies on that
cast to trigger an evaluation error, so CI now fails at
`row_group_filter.rs:1686`. Please replace the fixture with a genuine
evaluation failure while preserving row-group retention, an incremented error
counter, and unchanged Bloom matched/pruned counters. Do not relax the cast
restriction. [Failed CI
job](https://github.com/jayzhan211/datafusion/actions/runs/41266200712/job/124730336508).
##########
datafusion/sqllogictest/test_files/cast.slt:
##########
@@ -164,6 +164,22 @@ physical_plan
02)--SortExec: TopK(fetch=3), expr=[column1@0 ASC NULLS LAST],
preserve_partitioning=[false]
03)----DataSourceExec: partitions=1, partition_sizes=[1]
+# An integer read as a count of seconds keeps its order, so the outer sort is
+# removed.
+query TT
+EXPLAIN SELECT arrow_cast(i, 'Timestamp(Second, None)') AS ts
Review Comment:
[P3, optional] The EXPLAIN verifies that the outer sort disappears, but does
not check the resulting row order. Could you also execute the query with `query
P` (required for timestamp columns by `strict_column_validator`, without
`rowsort` or `valuesort`) and assert `1970-01-01T00:00:01`,
`1970-01-01T00:00:02`, and `1970-01-01T00:00:03` in that order? Please retain
the EXPLAIN to cover both optimization and SQL-visible ordering.
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -2360,6 +2390,231 @@ mod tests {
}
}
+ /// Sorted, distinct, non-null sample values of `data_type`, obtained by
+ /// casting a fixed set of seeds and keeping the values that convert.
+ fn order_samples(data_type: &DataType) -> Option<ArrayRef> {
+ use arrow::array::{
+ BooleanArray, FixedSizeBinaryArray, Float64Array,
TimestampSecondArray,
+ UInt32Array, make_comparator,
+ };
+ use arrow::compute::{
+ SortOptions, concat, filter, is_not_null, sort_to_indices, take,
+ };
+
+ let seeds: Vec<ArrayRef> = vec![
+ Arc::new(Int64Array::from(vec![
+ i64::MIN,
+ -2_147_483_649,
+ -129,
+ -1,
+ 0,
+ 1,
+ 9,
+ 10,
+ 127,
+ 128,
+ 255,
+ 256,
+ 86_399,
+ // America/Goose_Bay fell back from 00:01 to 23:01 the previous
+ // day at this instant, so the local date goes backwards.
+ 562_129_259,
+ 562_129_260,
+ 2_147_483_647,
+ i64::MAX,
+ ])),
+ Arc::new(Float64Array::from(vec![
+ f64::NEG_INFINITY,
+ -1.5,
+ -0.5,
+ 0.0,
+ 0.5,
+ 1.7,
+ 1e300,
+ f64::INFINITY,
+ ])),
+ Arc::new(StringArray::from(vec!["", "-1", "0", "1", "10", "9",
"a"])),
+ Arc::new(BooleanArray::from(vec![false, true])),
+ Arc::new(
+ FixedSizeBinaryArray::try_from_iter(
+ ["abcd", "abce", "zzzz"].into_iter().map(str::as_bytes),
+ )
+ .expect("valid fixed size binary"),
+ ),
+ Arc::new(TimestampSecondArray::from(vec![
+ // 23:00, 00:00 and 01:00 around midnight
+ 82_800,
+ 86_400,
+ 90_000,
+ // Local times around the America/New_York transitions of
+ // 2024: before, inside and after the skipped hour in March and
+ // the repeated hour in November.
+ 1_710_035_999,
+ 1_710_037_800,
+ 1_710_039_600,
+ 1_730_595_599,
+ 1_730_597_400,
+ 1_730_599_200,
+ ])),
+ ];
+ // Cast each seed on its own, so one value that fails to convert does
+ // not discard the others.
+ let parts = seeds
+ .iter()
+ .flat_map(|seed| (0..seed.len()).map(|i| seed.slice(i, 1)))
+ .filter_map(|seed| arrow::compute::cast(&seed, data_type).ok())
+ .map(|array| filter(&array,
&is_not_null(&array).unwrap()).unwrap())
+ .filter(|array| !array.is_empty())
+ .collect::<Vec<_>>();
+ if parts.is_empty() {
+ return None;
+ }
+ let parts = parts.iter().map(|a| a.as_ref()).collect::<Vec<_>>();
+ let all = concat(&parts).unwrap();
+ let sorted =
+ take(&all, &sort_to_indices(&all, None, None).unwrap(),
None).unwrap();
+ let cmp = make_comparator(&sorted, &sorted,
SortOptions::default()).unwrap();
+ let distinct = (0..sorted.len())
+ .filter(|&i| i == 0 || cmp(i - 1, i).is_ne())
+ .map(|i| i as u32)
+ .collect::<UInt32Array>();
+ Some(take(&sorted, &distinct, None).unwrap())
+ }
+
+ /// Casts the sorted `samples` to `target` and returns the first pair of
+ /// converted values whose order the cast reverses. Each value is cast on
+ /// its own, and values that fail to convert are skipped:
+ /// `is_order_preserving_cast` does not cover them.
+ ///
+ /// Returns `None` when the cast keeps the order, and `Some(Err(_))` when
+ /// the cast kernel panics.
+ fn order_violation(
+ samples: &ArrayRef,
+ target: &DataType,
+ ) -> Option<std::result::Result<String, String>> {
+ use arrow::array::make_comparator;
+ use arrow::compute::SortOptions;
+
+ let mut converted: Vec<(usize, ArrayRef)> = vec![];
+ for i in 0..samples.len() {
+ let value = samples.slice(i, 1);
+ let output =
std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+ arrow::compute::cast_with_options(
+ &value,
+ target,
+ &DEFAULT_SAFE_CAST_OPTIONS,
+ )
+ }));
+ match output {
+ Ok(Ok(output)) if output.is_valid(0) => converted.push((i,
output)),
+ Ok(_) => {}
+ Err(_) => return Some(Err("cast kernel panicked".to_string())),
+ }
+ }
+ converted.windows(2).find_map(|w| {
+ let ((i, left), (j, right)) = (&w[0], &w[1]);
+ let cmp = make_comparator(left, right,
SortOptions::default()).unwrap();
+ cmp(0, 0).is_gt().then(|| {
+ let value = |array: &ArrayRef, i| {
+ ScalarValue::try_from_array(array, i).unwrap().to_string()
+ };
+ Ok(format!(
+ "{} -> {}, but {} -> {}",
+ value(samples, *i),
+ value(left, 0),
+ value(samples, *j),
+ value(right, 0),
+ ))
+ })
+ })
+ }
+
+ /// Checks `is_order_preserving_cast` against the Arrow cast kernel: every
+ /// pair it accepts must keep the order of the sample values that convert.
+ /// Pruning and sort elimination both rely on this rule, so a wrong entry
+ /// drops matching rows or returns unsorted output.
+ #[test]
+ fn test_order_preserving_cast_matches_cast_kernel() {
+ use TimeUnit::*;
+
+ let tz = |unit, tz: &str| Timestamp(unit, Some(tz.into()));
+ let types = [
+ Boolean,
+ Int8,
+ Int16,
+ Int32,
+ Int64,
+ UInt8,
+ UInt16,
+ UInt32,
+ UInt64,
+ Float32,
+ Float64,
+ Decimal128(10, 2),
+ Decimal256(40, 2),
+ Utf8,
+ LargeUtf8,
+ Utf8View,
+ Binary,
+ LargeBinary,
+ BinaryView,
+ FixedSizeBinary(4),
+ Date32,
+ Date64,
+ Timestamp(Second, None),
+ Timestamp(Nanosecond, None),
+ tz(Millisecond, "UTC"),
+ tz(Second, "+05:00"),
+ tz(Second, "America/New_York"),
+ tz(Second, "America/Goose_Bay"),
+ Time32(Second),
+ Time32(Millisecond),
+ Time64(Microsecond),
+ Time64(Nanosecond),
+ Duration(Second),
+ Duration(Nanosecond),
+ ];
+ let mut violations = vec![];
+ for source in &types {
+ let Some(samples) = order_samples(source) else {
+ continue;
+ };
+ for target in &types {
+ if !can_cast_types(source, target)
+ || !is_order_preserving_cast(source, target)
+ {
+ continue;
+ }
+ match order_violation(&samples, target) {
+ None => {}
+ // A panic is an unchecked overflow in a debug build, which
+ // wraps in a release build, so it is not evidence of order
+ // either. These pairs are listed separately in the output.
+ Some(Err(e)) => println!("{source} -> {target}: {e}"),
Review Comment:
[P3, optional] The test prints `Date64 -> Timestamp(ns): cast kernel
panicked` but still passes, skipping successful-value ordering comparisons for
that pair. Could we use DataFusion's strict `ColumnarValue::cast_to` path, skip
conversion errors, and compare successful outputs? The existing
`cast_date64_array_to_timestamp_overflow` test already covers overflow
rejection, so no duplicate overflow test is needed.
--
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]
