zhuqi-lucas commented on PR #24638:
URL: https://github.com/apache/datafusion/pull/24638#issuecomment-5411175550
> 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
commen
t 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.
--
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]
