-
Notifications
You must be signed in to change notification settings - Fork 30
Use new sedona_internal_err to avoid misleading datafusion internal err #2
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
|
I can't make the CI go on this repo because I don't have write access, but I think @jiayuasu can (and I think this problem will go away once your first PR merges). |
paleolimbot
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.
Thank you!
We'll need to get CI running to make sure, but this is great in principle!
| Please help us to resolve this by filing a bug report in our issue tracker: \ | ||
| https://github.com/apache/sedona-db/issues", | ||
| std::format!($($args),*), | ||
| datafusion_common::DataFusionError::get_back_trace(), |
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.
If we get the same linking errors in CI that we got before, you could try removing the back trace feature (we can always add it back in later if it turns out that we're having trouble debugging user errors)
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.
Ah, that's what you meant
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.
it passed !
paleolimbot
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.
Thank you!
I've found the use of Datafusion's
internal_err!to be very misleading during development because it suggests the error is DataFusion's and not Sedona's (see the example texts). This PR adds a new macro to use instead.Example (code was modified to intentionally trigger this internal error):
Before:
Now:
It would be nice to avoid the "External error:" prepending the message, but that seems to be the most reasonable of the available options without making an upstream change.