Repository navigation
fix: avoid pruning timestamp-to-time casts - #26452
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26452 +/- ##
==========================================
- Coverage 82.89% 82.89% -0.01%
==========================================
Files 1154 1154
Lines 457630 457665 +35
Branches 457630 457665 +35
==========================================
+ Hits 379370 379394 +24
- Misses 55256 55263 +7
- Partials 23004 23008 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I'm wondering if it's possible to have sqlogictests that would cover this? 🤔 |
| } | ||
|
|
||
| #[test] | ||
| fn row_group_predicate_cast_timestamp_to_time() { |
There was a problem hiding this comment.
| fn row_group_predicate_cast_timestamp_to_time() { | |
| fn row_group_predicate_cast_timestamp_to_time() { | |
| // A timezone shifts where the day boundary falls but does not remove | |
| // it, so a timezone-aware timestamp is just as unsafe to prune on. | |
| for timestamp_type in [ | |
| DataType::Timestamp(TimeUnit::Second, None), | |
| DataType::Timestamp(TimeUnit::Second, Some("+05:00".into())), | |
| DataType::Timestamp(TimeUnit::Nanosecond, Some("UTC".into())), | |
| ] { | |
| assert_timestamp_to_time_cast_is_not_prunable(timestamp_type); | |
| } | |
| } | |
| /// Asserts that no comparison of `CAST`/`TRY_CAST` from `timestamp_type` to | |
| /// a time-of-day produces a pruning expression. | |
| fn assert_timestamp_to_time_cast_is_not_prunable(timestamp_type: DataType) { |
I had claude update the test to handle timestamp with tz as I wanted to be sure that this fix covered that case as well.
There was a problem hiding this comment.
Thanks — added this in 1fc4b25. The helper now runs the same CAST/TRY_CAST assertions across all Time32/Time64 units for timestamps without a timezone, with +05:00, and with UTC at nanosecond precision.
|
I got wondering just how difficult it would be to have tests for this as slt tests so I once again pointed claude at it and here is what it came up with. All 6 Parquet-backed queries in this slt fail on main and pass in this PR. |
c874f76 to
1fc4b25
Compare
1fc4b25 to
514e0e3
Compare
@Omega359 Thanks for putting this together! I added |
|
Hi @nuno-faria, would you have a chance to take a look at this PR when you have some time? I noticed you recently reviewed #26405, which touches related pruning logic. This PR addresses a correctness issue with timestamp-to-time casts during statistics pruning. I’d appreciate your feedback. Thanks! |
nuno-faria
left a comment
There was a problem hiding this comment.
Thanks @rgbuilds, LGTM. I leave some additional suggestions below. I also don't think the Rust tests are necessary since the slt ones appear to cover everything, but it's not critical if they stay.
| @@ -0,0 +1,192 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
I think this is a bit niche to create a new slt file. Maybe it would be better to add to push_down_filter_parquet.slt.
There was a problem hiding this comment.
Agreed. I moved this regression section into push_down_filter_parquet.slt and removed the standalone SLT file.
| # Disable the listing cache so DataFusion picks up changes from COPY statements. | ||
| statement ok | ||
| set datafusion.runtime.list_files_cache_limit = "0K"; |
There was a problem hiding this comment.
You’re right, this isn’t necessary here because each file is written before the external table is created. Removed the cache override and reset.
| statement ok | ||
| RESET datafusion.catalog.create_default_catalog_and_schema; |
There was a problem hiding this comment.
This config appears unchanged, so I'm not sure why it's reset here.
There was a problem hiding this comment.
Correct, this setting is unchanged by the test. Removed the leftover reset.
514e0e3 to
8574947
Compare
Thanks @nuno-faria. I kept the Rust coverage because it complements the SLT: the unit test covers |
| // Casting a timestamp to a time-of-day wraps at midnight and does not | ||
| // preserve ordering across day boundaries. Casting timestamp min/max | ||
| // statistics could therefore produce invalid bounds and prune matches. |
There was a problem hiding this comment.
@rgbuilds thanks for the fix! The root cause is that pruning has its own order check that allows any cast unless exactly one side is a string, instead of asking is_order_preserving_cast in physical-expr/src/expressions/cast.rs, which already rejects this cast. So similar casts still drop rows here, e.g. CAST(i AS BOOLEAN) and CAST(i AS BYTEA) (#26981). #26982 moves pruning onto that shared rule and will replace this arm, so this is fine to merge as the fix for #26451.
There was a problem hiding this comment.
Thanks @jayzhan211 for identifying the broader root cause. I’ll keep this PR scoped to #26451
|
Thanks @rgbuilds @Omega359 @nuno-faria ! |
## Which issue does this PR close? - Closes apache#26981. - Related to apache#26451, the same cause for timestamp → time (fixed on its own by apache#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 (apache#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).
## Which issue does this PR close? - Closes apache#26451. ## Rationale for this change Casting timestamp statistics to time-of-day values does not preserve ordering across midnight. This can invert the minimum and maximum used by statistics pruning and incorrectly discard files or row groups that contain matching rows. ## What changes are included in this PR? - Prevent timestamp-to-`Time32` and timestamp-to-`Time64` casts from being used for statistics pruning. - Add pruning-expression coverage for both `CAST` and `TRY_CAST` across all supported time units. - Add a Parquet regression test whose timestamp bounds cross midnight. ## What is the testing strategy for this PR? The regression test verifies equality and range predicates for `Time32` and `Time64`, including the expected row counts and pruning metrics. The full extended workspace test suite also passes. ## Are there any user-facing changes? Queries that cast timestamps to time-of-day values may perform less statistics pruning, but they no longer risk incorrect results when timestamp bounds cross a day boundary. (backport of apache#26452, 6110f74) Signed-off-by: discord9 <[email protected]> (cherry picked from commit 17f5b0ba54232e76d6bb45d1e4b77b9ade691fd7)
Which issue does this PR close?
Rationale for this change
Casting timestamp statistics to time-of-day values does not preserve ordering across midnight. This can invert the minimum and maximum used by statistics pruning and incorrectly discard files or row groups that contain matching rows.
What changes are included in this PR?
Time32and timestamp-to-Time64casts from being used for statistics pruning.CASTandTRY_CASTacross all supported time units.What is the testing strategy for this PR?
The regression test verifies equality and range predicates for
Time32andTime64, including the expected row counts and pruning metrics. The full extended workspace test suite also passes.Are there any user-facing changes?
Queries that cast timestamps to time-of-day values may perform less statistics pruning, but they no longer risk incorrect results when timestamp bounds cross a day boundary.