-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Refactor error handling to use boxed errors for DataFusionError variants #16672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cc @crepererum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing -- thank you @kosiew
I recommend:
- Adding a test to ensure the size of DataFusionError does not get larger (similar to the one added by @crepererum in #16653 )
- Add a note in the upgrade guide (in #16673)
Added test and upgrade note. |
…nts (apache#16672) - Refactored the `DataFusionError` enum to use `Box<T>` for: - `ArrowError` - `ParquetError` - `AvroError` - `object_store::Error` - `ParserError` - `SchemaError` - `JoinError` - Updated all relevant match arms and constructors to handle boxed errors. - Refactored error-related macros (`arrow_datafusion_err!`, `sql_datafusion_err!`, etc.) to use `Box<T>`. - Adjusted test cases and error assertions for boxed variants. - Documentation update to the upgrade guide to explain the required changes and rationale.
Which issue does this PR close?
Rationale for this change
This change standardizes the internal representation of several
DataFusionError
variants by wrapping them inBox<T>
. This:DataFusionError
enum.What changes are included in this PR?
DataFusionError
enum to useBox<T>
for:ArrowError
ParquetError
AvroError
object_store::Error
ParserError
SchemaError
JoinError
arrow_datafusion_err!
,sql_datafusion_err!
, etc.) to useBox<T>
.Are these changes tested?
Yes. These changes are covered by existing unit and integration tests, which validate correct error handling and propagation. Additional assertions were added where necessary to handle boxed variants.
Are there any user-facing changes?
No. This refactor maintains API compatibility and does not introduce breaking changes for users. Error messages and types remain consistent from the user's perspective.