shoemoney opened a new pull request, #25562:
URL: https://github.com/apache/datafusion/pull/25562
## Which issue does this PR close?
- Closes #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 #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 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]
