Repository navigation
feat: extend interval analysis support for temporal types - #21520
Merged
berkaysynnada merged 6 commits intoApr 17, 2026
Merged
Conversation
berkaysynnada
approved these changes
Apr 17, 2026
berkaysynnada
left a comment
Contributor
There was a problem hiding this comment.
LGTM, just two minor issues
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Apr 17, 2026
Rich-T-kid
pushed a commit
to Rich-T-kid/datafusion
that referenced
this pull request
Apr 21, 2026
## Which issue does this PR close? - Closes apache#21111 related with apache#21109 and its PR apache#21473 ## Rationale for this change Temporal types can actually support internal arithmetics and with this change internal arithmetics can now narrow down column boundaries and selectivity instead of falling back to default selectivity of 1.0 ## What changes are included in this PR? Extend internal_arithmetic types with date and duration types ## Are these changes tested? Yes adjusted tests ## Are there any user-facing changes? no
alamb
pushed a commit
to rxdrccq/datafusion
that referenced
this pull request
Oct 16, 2026
## Which issue does this PR close? - Closes apache#25532. ## Rationale for this change A view that exposes integer ids as strings, queried with an integer predicate, fails with an internal error even though every value is valid: ```sql CREATE VIEW text_ids AS SELECT CAST(id AS VARCHAR) AS id FROM ids; -- ids holds 2 and 10 SELECT id FROM text_ids WHERE CAST(id AS INT) > 1; ``` ``` Internal error: Assertion failed: interval.lower.is_null() || interval.upper.is_null() || interval.lower <= interval.upper: Interval's lower bound 2 is greater than the upper bound 10. ``` The cause is in `check_support` in `datafusion/physical-expr/src/intervals/utils.rs`. On main the `CastExpr` arm (lines 55 and 56) is: ```rust } else if let Some(cast) = expr.downcast_ref::<CastExpr>() { check_support(cast.expr(), schema) ``` It recurses into the operand and never looks at `cast.cast_type()`. `is_datatype_supported` (lines 93 to 110) allows the integer, float and temporal types, and deliberately does not include `Utf8`, but that allowlist is only ever consulted for columns and literals, never for a cast target. So `check_support` returns `true` for the expression above. `filter.rs` calls `analyze(...)?` inside that branch, with no fallback, and `Interval::cast_to` in `datafusion/expr-common/src/interval_arithmetic.rs` converts the two endpoints independently before handing them to `Interval::try_new`. The numeric interval `[2, 10]` derived from Parquet statistics becomes `["2", "10"]`, and under string ordering `"2" > "10"`, so `try_new` rejects it and the query fails. One nuance worth stating, because it explains why an obvious test does not reproduce it. The simple shape `CAST(id AS VARCHAR) = '5'` is already rejected, because the literal `'5'` is `Utf8` and the existing literal check catches it. Reproducing the bug needs the unsupported type to appear only in the middle of a cast chain whose outermost type is supported, which is exactly the view scenario in the issue: the view supplies the inner cast to string, and the user predicate supplies the outer cast back to integer. Nothing in the expression tree is then string typed at a point the current checks look at. ## What changes are included in this PR? The `CastExpr` arm now requires the cast target type to be one interval arithmetic can order before it recurses: ```rust } else if let Some(cast) = expr.downcast_ref::<CastExpr>() { is_datatype_supported(cast.cast_type()) && check_support(cast.expr(), schema) ``` This is strictly analysis disabling. `check_support` is a gate that decides whether to attempt interval analysis at all, and the change can only ever turn a `true` into a `false`. It cannot alter any interval that is computed today, so no currently valid numeric or temporal interval can regress. The only effect is that an expression casting through a type outside `is_datatype_supported` falls back to not being analyzed, which is the behavior these expressions should have had all along. This also follows the direction of apache#21520, which last touched this file and extended the same allowlist for temporal types. ## What is the testing strategy for this PR? Two unit tests in a new `tests` module in `datafusion/physical-expr/src/intervals/utils.rs`, covering both the direct unsupported target and the round trip through an unsupported type, plus a positive case asserting supported cast targets are still analyzed. Plus an end to end case in `datafusion/sqllogictest/test_files/cast.slt` that reproduces the issue over Parquet, so the statistics derived interval is real rather than synthetic. Unit tests, against unmodified source: ``` ---- intervals::utils::tests::test_check_support_rejects_unsupported_cast_target stdout ---- thread 'intervals::utils::tests::test_check_support_rejects_unsupported_cast_target' panicked at datafusion/physical-expr/src/intervals/utils.rs:219:9: assertion failed: !check_support(&to_utf8, &schema) test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 1664 filtered out ``` sqllogictest, against unmodified source: ``` 1. query failed: DataFusion error: Internal error: Assertion failed: interval.lower.is_null() || interval.upper.is_null() || interval.lower <= interval.upper: Interval's lower bound 2 is greater than the upper bound 10. [SQL] SELECT id FROM cast_interval_text_ids WHERE CAST(id AS INT) > 1 ORDER BY id; at test_files/cast.slt:283 ``` That is the exact error from the issue. With the fix, `cargo test -p datafusion-physical-expr`: ``` test result: ok. 1677 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out ``` The baseline on `main` for the same crate was 1136 passing in the `intervals` filter, and 1138 after adding the two new tests, so nothing existing changed status. `cargo test --test sqllogictests -- cast.slt` passes, and `cargo fmt --all -- --check` and `cargo clippy -p datafusion-physical-expr --all-targets --all-features -- -D warnings` are both clean. ## Are there any user-facing changes? Yes, in the sense that a query that previously failed with an internal error now returns the correct rows. There are no API changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
related with #21109 and its PR #21473
Rationale for this change
Temporal types can actually support internal arithmetics and with this change internal arithmetics can now narrow down column boundaries and selectivity instead of falling back to default selectivity of 1.0
What changes are included in this PR?
Extend internal_arithmetic types with date and duration types
Are these changes tested?
Yes adjusted tests
Are there any user-facing changes?
no