Extend backtrace coverage for DatafusionError::Plan errors errors
#7803
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
sql
comphead
commented
Oct 12, 2023
datafusion/common/src/error.rs
Outdated
| @@ -493,16 +500,16 @@ macro_rules! make_error { | |||
| } | |||
|
|
|||
| // Exposes a macro to create `DataFusionError::Plan` | |||
| make_error!(plan_err, Plan); | |||
| make_error!(plan_err, plan_err_raw, Plan); | |||
There was a problem hiding this comment.
I was not able to invent better naming, appreciate if anyone helps
There was a problem hiding this comment.
Maybe plan_err_obj or plan_datafusion_error?
waynexia
approved these changes
Oct 14, 2023
alamb
approved these changes
Oct 14, 2023
datafusion/common/src/error.rs
Outdated
| @@ -478,9 +478,16 @@ macro_rules! with_dollar_sign { | |||
| /// plan_err!("Error {val}") | |||
| /// plan_err!("Error {val:?}") | |||
| macro_rules! make_error { | |||
| ($NAME:ident, $ERR:ident) => { | |||
| ($NAME:ident, $NAME_RAW: ident, $ERR:ident) => { | |||
There was a problem hiding this comment.
Can you please document in comments what $NAME and $NAME_RAW are and how they are different?
my reading is that the raw is the name of an internal macro that gets generated. I think it would help to document more clearly what the difference in plan_err! and plan_err_raw! did (specifically that plan_err returns an Err(plan_err_raw)
datafusion/common/src/error.rs
Outdated
| @@ -493,16 +500,16 @@ macro_rules! make_error { | |||
| } | |||
|
|
|||
| // Exposes a macro to create `DataFusionError::Plan` | |||
| make_error!(plan_err, Plan); | |||
| make_error!(plan_err, plan_err_raw, Plan); | |||
There was a problem hiding this comment.
Maybe plan_err_obj or plan_datafusion_error?
jackwener
approved these changes
Oct 16, 2023
alamb
reviewed
Oct 16, 2023
| @@ -477,12 +477,25 @@ macro_rules! with_dollar_sign { | |||
| /// plan_err!("Error {:?}", val) | |||
| /// plan_err!("Error {val}") | |||
| /// plan_err!("Error {val:?}") | |||
| /// | |||
| /// `NAME_ERR` - macro name for wrapping Err(DataFusionError::*) | |||
| /// `NAME_DF_ERR` - macro name for wrapping DataFusionError::*. Needed to keep backtrace opportunity | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #7818
The PR improves backtrace coverage for
DatafusionError::Planerrors, specifically for errors called inok_or_elseormap_errconstructionsRationale for this change
Currently constructions
ok_or_elseormap_errusing direct reference toDatafusionError::Planwhich makes impossible to backtrace them in case of tracing error for theResult<T, E>. To improve the coverage one more error macros introducedplan_err_raw(name discussible)What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?