zhuqi-lucas commented on code in PR #24638:
URL: https://github.com/apache/datafusion/pull/24638#discussion_r3848922786
##########
datafusion/core/tests/parquet/filter_pushdown.rs:
##########
@@ -746,3 +752,51 @@ impl PredicateCacheTest {
Ok(())
}
}
+
+/// A predicate that is pushed into the parquet decoder and then fails while it
+/// is being evaluated must report the original error, so that callers can
still
+/// tell a user error apart from an internal one.
+#[tokio::test]
+async fn pushed_down_predicate_reports_the_original_error() {
+ let tempdir = TempDir::new_in(Path::new(".")).unwrap();
+ let path = tempdir.path().join("cast_error.parquet");
+
+ let batch = RecordBatch::try_from_iter(vec![(
+ "s",
+ Arc::new(StringArray::from(vec!["not_an_int"])) as ArrayRef,
Review Comment:
Forward-looking nit on test robustness: the file has a single column `s` and
the predicate is also on `s`, so the projection contains **zero** non-filter
columns. That's exactly the shape a narrow-projection pushdown heuristic would
decline, and #24426 is prototyping that kind of gating.
I checked — no such gate exists on `main` today (only unsupported nested
types make `build()` return `None`), so this passes now. But
`assert!(!plan.contains("FilterExec"))` would start failing if one lands. Cheap
insurance: write a second column that the predicate doesn't touch, so the
projection always has a non-filter column.
##########
datafusion/datasource-parquet/src/row_filter.rs:
##########
@@ -161,11 +161,10 @@ impl ArrowPredicate for DatafusionArrowPredicate {
timer.stop();
Ok(bool_arr)
})
- .map_err(|e| {
- ArrowError::ComputeError(format!(
- "Error evaluating filter predicate: {e:?}"
- ))
- })
+ // Convert rather than format: converting leaves the original error
+ // in the source chain, so callers can still recover it (for
example
+ // with `DataFusionError::find_root`)
+ .map_err(|e| e.context("Error evaluating filter predicate").into())
Review Comment:
Worth making explicit which trade this is, since `From<DataFusionError> for
ArrowError` has an arm built for exactly this case:
```rust
// common/src/error.rs:370
match e {
DataFusionError::ArrowError(e, _) => *e, // preserves the
original variant
DataFusionError::External(e) => ArrowError::ExternalError(e),
other => ArrowError::ExternalError(Box::new(other)), // <- .context()
lands here
}
```
`.context()` wraps into `DataFusionError::Context` first, which is precisely
what routes past that first arm — so a plain `.map_err(|e| e.into())` would
hand back a real `ArrowError::CastError` at this boundary, while this version
puts it in the source chain behind `ExternalError`.
You call this out in the description, so the question is just which consumer
you're targeting: does the embedder classify by matching `ArrowError` variants
(then the direct conversion serves them better), or does it go through
`find_root` (then this is fine)? I lean toward keeping the context — the outer
error only says "Parquet error", so naming the predicate stage is genuinely
useful — but it's worth pinning down the intent.
--
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]
