Skip to content

Commit 4bf32cb

Browse files
jayzhan211jayzhan211
authored
fix: prune statistics only through casts that keep order (#26982)
## Which issue does this PR close? - Closes #26981. - Related to #26451, the same cause for timestamp → time (fixed on its own by #26452). ## Rationale for this change Parquet statistics pruning can drop row groups that contain matching rows when the predicate casts the column. With `i` in (-1, 0, 1, 255, 256) and naive timestamps `ts` at 00:00, 01:00, 02:30, 02:45 and 03:00 on 2024-03-10, all in one row group: | Query | In-memory table | Parquet | |---|---|---| | `SELECT count(*) FROM t WHERE CAST(i AS BOOLEAN) <= false` | 1 | 0 | | `SELECT count(*) FROM t WHERE CAST(i AS BYTEA) < X'05'` | 3 | 0 | | `SELECT count(*) FROM t WHERE ts > TIMESTAMPTZ '2024-03-10 07:15:00+00:00'` with `time_zone = 'America/New_York'` | 2 | 0 | 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, and `is_order_preserving_cast` rejects 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 → `Date64` now takes the local date, as `Date32` does, so a sort on that cast can be removed although the output is out of order. ## What changes are included in this PR? 1. `is_order_preserving_cast` (now public in `datafusion_physical_expr::expressions`) accepts the casts that keep order and that only pruning knew about: - string ↔ string and binary ↔ binary re-encodings (including `FixedSizeBinary`) - numbers ↔ temporal types stored as a count (date, timestamp, time, duration) - `Date32` → timestamp with any timezone (a skipped midnight moves forward by at most a day, so it never passes the next one), and `Date64` → timestamp with a fixed offset - `Time64` → `Time32` and `Time64(µs)` → `Time64(ns)`, which Arrow 60.1.0 checks for overflow 2. It rejects named-timezone timestamp → `Date64`, as it already did for `Date32`. 3. Pruning (`verify_support_type_for_prune`) asks `is_order_preserving_cast` instead of its own string check. 4. A test that checks `is_order_preserving_cast` against the strict cast that `CastExpr` runs (`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: | Cast | Why | |---|---| | integer/float → boolean | -1 and 1 are true, 0 is false | | integer → binary | little-endian bytes do not sort like the integers | | named-timezone timestamp → `Date32`/`Date64` | a transition can move the local date backwards | | naive timestamp or `Date64` → named-timezone timestamp | arrow moves a skipped local time forward, past the local times just after the gap | | casts from or to `Null`, to list/struct, interval casts | the rule has no entry for them | ## What 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 return `true` fails it with `562129259 -> 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 both `CAST` and `TRY_CAST`. - `push_down_filter_parquet.slt`: the three queries above against an in-memory table and Parquet, plus an `EXPLAIN` showing that the same timestamp comparison under a fixed-offset session timezone still prunes. - `cast.slt`: the sort on `arrow_cast(i, 'Timestamp(Second, None)')` is removed when the input is ordered by `i` (`EXPLAIN` and the row order), and the sort on a named-timezone timestamp cast to `Date64` is kept. - The new pruning unit test cases and Parquet queries fail with the old pruning check. The `Date64` sort case and the New York Parquet query return wrong results on main. - `bloom_filter_pruning_error_retains_row_group_without_match` relied on the int → `FixedSizeBinary` cast failing at evaluation. Pruning now rejects that cast up front, so the test compares `c1` with `CAST('abc' AS Int32)` instead, which still fails at evaluation. ## Are there any user-facing changes? - Queries that filter Parquet through the casts in the table above return the correct rows; those predicates no longer prune row groups. With a named session timezone, comparing a naive timestamp column with a timezone-aware timestamp no longer prunes; with a fixed offset it still does. - Sort elimination treats the newly accepted casts as order-preserving under `CAST` (not `TRY_CAST`), so a sort on, for example, `CAST(int_col AS TIMESTAMP)` over input ordered by `int_col` can be removed. A sort on a named-timezone timestamp cast to `Date64` is no longer removed. - New public function `datafusion_physical_expr::expressions::is_order_preserving_cast` (addition only).
1 parent c3eeb47 commit 4bf32cb

6 files changed

Lines changed: 568 additions & 51 deletions

File tree

‎datafusion/datasource-parquet/src/row_group_filter.rs‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1663,22 +1663,25 @@ mod tests {
16631663
fn bloom_filter_pruning_error_retains_row_group_without_match() {
16641664
let schema =
16651665
Arc::new(Schema::new(vec![Field::new("c1", DataType::Int32, false)]));
1666-
let cast_expr: Arc<dyn PhysicalExpr> = Arc::new(CastExpr::new(
1667-
Arc::new(PhysicalColumn::new("c1", 0)),
1668-
DataType::FixedSizeBinary(4),
1666+
// c1 = CAST('abc' AS Int32): constant folding fails, so the
1667+
// simplifier keeps the cast, and the cast fails at evaluation.
1668+
let value: Arc<dyn PhysicalExpr> = Arc::new(CastExpr::new(
1669+
Arc::new(Literal::new(ScalarValue::from("abc"))),
1670+
DataType::Int32,
16691671
None,
16701672
));
1671-
let literal: Arc<dyn PhysicalExpr> = Arc::new(Literal::new(
1672-
ScalarValue::FixedSizeBinary(4, Some(vec![0, 0, 0, 1])),
1673+
let expr = Arc::new(BinaryExpr::new(
1674+
Arc::new(PhysicalColumn::new("c1", 0)),
1675+
Operator::Eq,
1676+
value,
16731677
));
1674-
let expr = Arc::new(BinaryExpr::new(cast_expr, Operator::Eq, literal));
16751678
let pruning_predicate = PruningPredicateBuilder::new()
16761679
.with_file_schema(schema)
16771680
.try_build(expr)
16781681
.unwrap();
16791682

16801683
// A nonempty map bypasses the no-filter path. Evaluating the predicate
1681-
// then fails because Arrow cannot cast Int32 statistics to FixedSizeBinary.
1684+
// then fails because 'abc' is not an Int32.
16821685
let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap();
16831686
sbbf.insert(&1_i32);
16841687
let mut bloom_statistics = BloomFilterStatistics::new();

0 commit comments

Comments
 (0)