adriangb opened a new pull request, #24638:
URL: https://github.com/apache/datafusion/pull/24638
## Which issue does this PR close?
No existing issue tracks this; happy to file one if the project prefers that
first.
## Rationale for this change
`ArrowPredicate::evaluate` must return an `ArrowError`, so
`DatafusionArrowPredicate::evaluate` built one by `Debug`-formatting the
`DataFusionError` it received:
```rust
.map_err(|e| {
ArrowError::ComputeError(format!("Error evaluating filter predicate:
{e:?}"))
})
```
Formatting the error discards its type. Every failure inside a predicate
pushed
into the parquet decoder reaches the caller as the same untyped
`ArrowError::ComputeError` carrying a `Debug` string, so a user error such
as a
failed cast is indistinguishable from an internal engine failure. Any
embedder
that classifies errors by variant, for example to decide whether a query
failed
because of the input or because of a bug, cannot do so for this path. It also
reads badly, because the nested error is rendered with `Debug` rather than
`Display`.
Reproduction with `datafusion-cli`:
```sql
COPY (SELECT 'not_an_int' AS s) TO 't.parquet' STORED AS PARQUET;
CREATE EXTERNAL TABLE t STORED AS PARQUET LOCATION 't.parquet';
SET datafusion.execution.parquet.pushdown_filters = true;
SELECT * FROM t WHERE CAST(s AS INT) = 1;
```
Before:
```
Error: Parquet error: External: Compute error: Error evaluating filter
predicate: ArrowError(CastError("Cannot cast string 'not_an_int' to value of
Int32 type"), Some(""))
```
After:
```
Error: Parquet error: External: External error: Error evaluating filter
predicate
caused by
Arrow error: Cast error: Cannot cast string 'not_an_int' to value of Int32
type
```
## What changes are included in this PR?
`DatafusionArrowPredicate::evaluate` now converts the error instead of
formatting it:
```rust
.map_err(|e| e.context("Error evaluating filter predicate").into())
```
`From<DataFusionError> for ArrowError` is the conversion DataFusion already
documents for this boundary. It leaves the original error in the
`Error::source`
chain, and the parquet decoder propagates it as `ParquetError::External`,
which
is also source preserving, so `DataFusionError::find_root` recovers the
original
variant at the top of the stack. Wrapping the error in a
`DataFusionError::Context` first keeps the description of where the failure
happened, which the old string also carried.
One consequence worth calling out: because the context has to live somewhere,
the returned `ArrowError` variant is `ExternalError` rather than the original
Arrow variant. Callers that want the type use `find_root` (or walk
`Error::source`), which is the existing way to recover an error across an
`ArrowError` boundary in DataFusion and is used the same way in
`datafusion/common/src/scalar/mod.rs` and `datafusion/physical-plan`.
Dropping
the context would yield a bare `ArrowError::CastError` here, at the cost of
no
longer saying which stage failed.
## Are these changes tested?
Yes. Two new tests, both of which fail without the change:
* `datafusion/datasource-parquet/src/row_filter.rs`:
`evaluate_reports_the_original_error` evaluates a predicate that fails to
cast
and asserts `find_root` returns the `CastError`, and that the message still
names the predicate. Without the change it observes
`ArrowError(ComputeError("Error evaluating filter predicate:
ArrowError(CastError(...))"))`.
* `datafusion/core/tests/parquet/filter_pushdown.rs`:
`pushed_down_predicate_reports_the_original_error` runs the same failure
through a full parquet scan. It first asserts the predicate really is
pushed
into the scan and not left in a `FilterExec`, so the test cannot pass
vacuously, then asserts the same about the error the query returns.
Existing suites run locally: `cargo test -p datafusion-datasource-parquet`,
`cargo test -p datafusion --test parquet_integration`, and the full
`sqllogictest` suite, all passing. No test expectation elsewhere depended on
the
old string.
## Are there any user-facing changes?
The text of the error raised when a pushed down parquet predicate fails
changes,
as shown above. There is no public API change.
--
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]
