-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 |
alamb
left a comment
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
DataFusionErrorvariants by wrapping them inBox<T>. This:DataFusionErrorenum.What changes are included in this PR?
DataFusionErrorenum to useBox<T>for:ArrowErrorParquetErrorAvroErrorobject_store::ErrorParserErrorSchemaErrorJoinErrorarrow_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.