Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add plan_err! error macro #7115

Merged
merged 9 commits into from
Aug 3, 2023
Merged

Add plan_err! error macro #7115

merged 9 commits into from
Aug 3, 2023

Conversation

comphead
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Related to #6740.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate labels Jul 27, 2023
@comphead
Copy link
Contributor Author

@alamb I went through some use cases for replacing plan errors with macro. Going fwd we can add to the macro file!, line! macros to let user know the exact cause of the error. Please let me know your thoughts on this direction

@comphead comphead changed the title Dev RFC: add error macros Jul 27, 2023
alamb
alamb reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR looks really cool @comphead - the new macro seems to save a lot of boiler plate and is quite clear in my mind

@comphead
Copy link
Contributor Author

I think this PR looks really cool @comphead - the new macro seems to save a lot of boiler plate and is quite clear in my mind

Cool, thanks for the feedback, will be covering all other plan references

@github-actions github-actions bot added sql SQL Planner physical-expr Physical Expressions substrait labels Aug 1, 2023
@comphead comphead marked this pull request as ready for review August 1, 2023 05:34
@comphead comphead requested a review from alamb August 1, 2023 05:34
@comphead
Copy link
Contributor Author

comphead commented Aug 1, 2023

@alamb went through all Err(DatafusionError::Plan) entries. There are still few entries like DatafusionError::Plan in ok_or_else or map_err, I will prob generate a separate macros for such case.

Thanks for checking the PR before the code rots

@alamb alamb changed the title RFC: add error macros Add error macros Aug 1, 2023
@alamb
Copy link
Contributor

alamb commented Aug 1, 2023

Screenshot 2023-08-01 at 3 55 59 PM

very nice @comphead 👍

@alamb alamb changed the title Add error macros Add plan_err! error macro Aug 1, 2023
alamb
alamb approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is epic work @comphead -- thank you so much. I'll plan to merge tomorrow unless anyone else wants more time to review

datafusion/common/src/error.rs
@@ -411,6 +399,56 @@ impl DataFusionError {
}
}

/// Unwrap an `Option` if possible. Otherwise return an `DataFusionError::Internal`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 very nice

datafusion/core/tests/sql/mod.rs Show resolved Hide resolved
@alamb alamb merged commit e695de0 into apache:main Aug 3, 2023
@alamb
Copy link
Contributor

alamb commented Aug 3, 2023

Thanks again @comphead

This was referenced Aug 15, 2023
Merged
Merged
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viirya viirya viirya left review comments

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner substrait
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@comphead @alamb @viirya