Repository navigation
fix: prune statistics only through casts that keep order - #26982
Conversation
| // 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) { |
There was a problem hiding this comment.
Call is_order_preserving_cast
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the fix. Reusing the order-preserving cast rule for statistics pruning is the right approach. It prevents unsafe min/max reasoning while retaining valid pruning and sort-elimination opportunities.
The architectural boundary is sound: pruning must reject non-monotonic casts before deriving bounds from statistics, while successful-value ordering, strict/lossless ordering, and NULL-on-failure handling remain separate contracts. Consolidating this policy avoids conflicting decisions between pruning and physical expression ordering analysis.
I'm requesting changes because the new restriction invalidates an existing Bloom-filter error-path fixture, causing the required Rust test workflow to fail. Please restore coverage of genuine evaluation failures without relaxing the cast restriction. The additional sampling and SQL ordering suggestions are non-blocking.
| // 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) { |
There was a problem hiding this comment.
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.
| // 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}"), |
There was a problem hiding this comment.
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.
| # 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 |
There was a problem hiding this comment.
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.
07410bb to
87c2447
Compare
|
@kosiew thanks for the review. Rebasing onto arrow 60.1.0 (#26867) made two of the rule's timezone entries wrong, so the last commit changes the rule itself. A naive timestamp read in a named timezone no longer counts as order-preserving, because arrow now moves skipped local times forward instead of rejecting them. A named-timezone timestamp cast to Date64 is now rejected the same way as Date32. The description has the queries that return wrong results on main. Could you take another look at that commit? |
There was a problem hiding this comment.
@jayzhan211, thanks for the follow-up changes. The revised Bloom-filter fixture now exercises a genuine evaluation failure while preserving conservative row-group retention and the associated metrics. The strict cast sampling and order-sensitive SQL regression also address my earlier suggestions. I found no new issues in the follow-up.
Using is_order_preserving_cast as the shared authority for pruning and sort elimination is the right architectural boundary. Both optimizations depend on monotonic conversions, while successful casting alone cannot justify transforming min/max statistics or preserving sort order. The Arrow 60.1.0 timezone corrections and regression coverage support that distinction.
My previous blocking concern is resolved, and the relevant tests pass.
Approving.
|
Thanks @kosiew ! |
Which issue does this PR close?
Rationale for this change
Parquet statistics pruning can drop row groups that contain matching rows when the predicate casts the column. With
iin (-1, 0, 1, 255, 256) and naive timestampstsat 00:00, 01:00, 02:30, 02:45 and 03:00 on 2024-03-10, all in one row group:SELECT count(*) FROM t WHERE CAST(i AS BOOLEAN) <= falseSELECT count(*) FROM t WHERE CAST(i AS BYTEA) < X'05'SELECT count(*) FROM t WHERE ts > TIMESTAMPTZ '2024-03-10 07:15:00+00:00'withtime_zone = 'America/New_York'Pruning casts the min/max statistics and compares them with the literal, which is only correct when the cast keeps the order of the values. Pruning decides that with its own check: any cast is allowed unless exactly one side is a string. So int → boolean, int → binary (little-endian bytes) and named-timezone timestamp → date all pass, although none of them keep order.
The last row is new with arrow 60.1.0 (#26867). A naive timestamp compared with a named timezone is read as a local time, and arrow now moves a local time that a transition skips forward instead of rejecting it: 02:30, which New York skipped, becomes 07:30 UTC, after 03:00 (07:00 UTC). When such a value is the minimum of a row group, the cast bounds are inverted and the query fails with
Internal error: Interval's lower bound ... is greater than the upper bound.Sort elimination already asks the same question through
is_order_preserving_cast. The two checks disagree in both directions: pruning accepts the casts above, andis_order_preserving_castrejects casts that do keep order and that pruning relies on today (integer ↔ date/timestamp/duration, string and binary re-encodings, date → timestamp with a timezone). Arrow 60.1.0 also made one of its own entries wrong: a named-timezone timestamp →Date64now takes the local date, asDate32does, so a sort on that cast can be removed although the output is out of order.What changes are included in this PR?
is_order_preserving_cast(now public indatafusion_physical_expr::expressions) accepts the casts that keep order and that only pruning knew about:FixedSizeBinary)Date32→ timestamp with any timezone (a skipped midnight moves forward by at most a day, so it never passes the next one), andDate64→ timestamp with a fixed offsetTime64→Time32andTime64(µs)→Time64(ns), which Arrow 60.1.0 checks for overflowDate64, as it already did forDate32.verify_support_type_for_prune) asksis_order_preserving_castinstead of its own string check.is_order_preserving_castagainst the strict cast thatCastExprruns (ColumnarValue::cast_to): every pair it accepts must keep the order of sample values that convert. The samples include values around midnight, a DST gap and overlap, a timezone that moved the date backwards, and a day that Pacific/Apia skipped. A positive control asserts the samples expose the casts the rule rejects.Pruning keeps every other cast it used before (compared over all castable pairs of the numeric, string, binary and temporal types) and stops using:
Date32/Date64Date64→ named-timezone timestampNull, to list/struct, interval castsWhat is the testing strategy for this PR?
test_order_preserving_cast_matches_cast_kernel(physical-expr): checks the rule against the strict cast, with a positive control. Making named-timezone timestamp → date returntruefails it with562129259 -> 1987-10-25, but 562129260 -> 1987-10-24.row_group_predicate_cast_must_preserve_order(pruning): casts that must not prune and casts that must keep pruning, for bothCASTandTRY_CAST.push_down_filter_parquet.slt: the three queries above against an in-memory table and Parquet, plus anEXPLAINshowing that the same timestamp comparison under a fixed-offset session timezone still prunes.cast.slt: the sort onarrow_cast(i, 'Timestamp(Second, None)')is removed when the input is ordered byi(EXPLAINand the row order), and the sort on a named-timezone timestamp cast toDate64is kept.Date64sort case and the New York Parquet query return wrong results on main.bloom_filter_pruning_error_retains_row_group_without_matchrelied on the int →FixedSizeBinarycast failing at evaluation. Pruning now rejects that cast up front, so the test comparesc1withCAST('abc' AS Int32)instead, which still fails at evaluation.Are there any user-facing changes?
CAST(notTRY_CAST), so a sort on, for example,CAST(int_col AS TIMESTAMP)over input ordered byint_colcan be removed. A sort on a named-timezone timestamp cast toDate64is no longer removed.datafusion_physical_expr::expressions::is_order_preserving_cast(addition only).