fix: preserve the error type of a failing parquet row filter predicate

[adriangb]

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:

.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:

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:

.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.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.44%. Comparing base (26b40dd) to head (ff38dfb).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #24638    +/-   ##
========================================
  Coverage   81.43%   81.44%            
========================================
  Files        1118     1118            
  Lines      399414   399602   +188     
  Branches   399414   399602   +188     
========================================
+ Hits       325278   325450   +172     
- Misses      55145    55153     +8     
- Partials    18991    18999     +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[saadtajwar]

Not a maintainer - but this change makes sense and LGTM!

My only nit feedback is the comment itself here may be unnecessary/redundant, but that's just a personal preference :)

            // Convert rather than format the error: the decoder requires an
            // `ArrowError`, and formatting collapses every failure into one
            // untyped `ComputeError`. `From<DataFusionError> for ArrowError`
            // leaves the original error in the source chain, so callers can
            // still recover it (for example with `DataFusionError::find_root`).

[adriangb]

@zhuqi-lucas wonder if you could take a look at this since you've also been working on row filters lately?

[zhuqi-lucas]

Nice catch — format!("{e:?}") was flattening the type and rendering the nested error with Debug, so this is a clear improvement.

I traced the chain against common/src/error.rs and it holds: context (:525) → From<DataFusionError> for ArrowError (:368) → find_root (:502) recovers ArrowError::CastError. I also grepped the whole repo (including .slt) for the old string — only this PR's two files match, so nothing else pinned it. Both tests look non-vacuous.

Two comments inline, neither blocking.

[zhuqi-lucas]

Worth making explicit which trade this is, since From<DataFusionError> for ArrowError has an arm built for exactly this case:

// 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.

[zhuqi-lucas]

Small note on the test: the file has one column s and the predicate is on s too, so the projection has zero non-filter columns. Any narrow-projection pushdown heuristic would decline this scan, and assert!(!plan.contains("FilterExec")) would then fail.

Nothing on main does that today, so this is fine as-is — just worth knowing the assertion depends on it.

[adriangb]

The direct conversion doesn't survive the trip: ArrowError::CastError is a String with no source(), so once the decoder re-wraps it (ParquetError::External) and DataFusion wraps that, there's no DataFusionError anywhere below the top of the chain. Flipping this line to .map_err(|e| e.into()) locally makes the end-to-end test fail with expected the original cast error, got ParquetError(External(CastError("Cannot cast string 'not_an_int' to value of Int32 type"))), i.e. the exact symptom this PR is fixing. ExternalError(Box<DataFusionError>) is the only shape that keeps a DataFusion node in the chain, since every other ArrowError variant carries only a String. A variant-matching embedder that walks the whole chain still reaches CastError two hops down, so it's a superset rather than a trade. Only one that matches the first ArrowError sees ExternalError. Both tests fail if someone later "simplifies" this to a plain .into(), and I've expanded the comment to say why.

Second point taken as well — the test now has a column the predicate doesn't reference, so a narrow-projection heuristic has no reason to decline the scan, and the assertion explains itself.

---

Update: rather than expanding the comment, I made the construction explicit — relying on .context() to route past the DataFusionError::ArrowError arm produced the right shape as a side effect, which is what took a paragraph to justify in the first place:

// `ExternalError` is the only `ArrowError` variant that keeps a
// source, and therefore the only one that leaves the original error
// recoverable (see `DataFusionError::find_root`)
.map_err(|e| {
    ArrowError::ExternalError(Box::new(
        e.context("Error evaluating filter predicate"),
    ))
})

Same resulting error, but the variant is named on the line instead of argued for underneath it, so the comment shrank to the one fact that isn't visible from the code. It also restored the ArrowError import to what's on main (the test module had been re-importing it), leaving the non-test diff as a one-for-one swap of ComputeError(format!(..)) for ExternalError(Box::new(..)).

[zhuqi-lucas]

The direct conversion doesn't survive the trip: ArrowError::CastError is a String with no source(), so once the decoder re-wraps it (ParquetError::External) and DataFusion wraps that, there's no DataFusionError anywhere below the top of the chain. Flipping this line to .map_err(|e| e.into()) locally makes the end-to-end test fail with expected the original cast error, got ParquetError(External(CastError("Cannot cast string 'not_an_int' to value of Int32 type"))), i.e. the exact symptom this PR is fixing. ExternalError(Box<DataFusionError>) is the only shape that keeps a DataFusion node in the chain, since every other ArrowError variant carries only a String. A variant-matching embedder that walks the whole chain still reaches CastError two hops down, so it's a superset rather than a trade. Only one that matches the first ArrowError sees ExternalError. Both tests fail if someone later "simplifies" this to a plain .into(), and I've expanded the comment to say why.

Second point taken as well — the test now has a column the predicate doesn't reference, so a narrow-projection heuristic has no reason to decline the scan, and the assertion explains itself.

You're right, and the source() implementation makes it unambiguous — every ArrowError variant except ExternalError and IoError returns None, so a plain CastError is a leaf and there's nothing left for find_root to walk once the decoder re-wraps it. "Superset rather than a trade" is the right framing; I was reasoning about the conversion in isolation rather than the whole chain. Expanding the comment so nobody re-simplifies it is exactly right.

Thanks for the test change too. LGTM.