Repository navigation
refactor: use DataFusion errors in integration - #7
Conversation
| pub fn to_datafusion_error(error: Error) -> DataFusionError { | ||
| DataFusionError::Context( | ||
| format!("IcebergError({})", error.kind()), | ||
| Box::new(DataFusionError::Execution(error.to_string())), | ||
| ) | ||
| } |
There was a problem hiding this comment.
If anyone has better ideas on how to encode iceberg errors into DataFusionError that's very welcome
There was a problem hiding this comment.
Could we use DataFusionError::External(Box::new(error)) here? It preserves the original Iceberg error and its source chain before transport. With the kind tag in Context we’d retain the same information after transport.
There was a problem hiding this comment.
I agree keeping the original error might be nice
a35eb73 to
7804ffe
Compare
alexanderbianchi
left a comment
alexanderbianchi
left a comment
There was a problem hiding this comment.
LGTM, if you see other low hanging fruit idiomatic refactors to make this more DF focused we could probably have an epic to get folks involved.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks like an improvement to me (and removes just a little more friction from the API)
4 participants
The inherited code treated
iceberg::Resultas the first-class citizen regarding errors in function signatures. This PR changes it so that, by default, aResultis adatafusion::common::Resultinstead, making it more datafusion-centric, and reducing error conversions within the DataFusion code.It also improves the DataFusion->Iceberg and Iceberg->DataFusion error conversion, so that if these errors are sent serialized over the wire, they can be fully recovered.
The specific changes are:
datafusion::error::Result