Skip to content

Conversation

@petern48
Copy link
Collaborator

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:

> select st_m(st_geomfromtext('point (1 1)'));
Internal error: unexpected dimension.
This issue was likely caused by a bug in DataFusion's code. Please help us to resolve this by filing a bug report in our issue tracker: https://github.com/apache/datafusion/issues

Now:

> select st_m(st_geomfromtext('point (1 1)'));
External error: SedonaDB internal error: unexpected dimension.
This issue was likely caused by a bug in SedonaDB's code. Please help us to resolve this by filing a bug report in our issue tracker: https://github.com/apache/sedona-db/issues

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.

@paleolimbot
Copy link
Member

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).

Copy link
Member

@paleolimbot paleolimbot left a 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(),
Copy link
Member

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)

Copy link
Collaborator Author

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it passed !

@petern48 petern48 marked this pull request as ready for review August 29, 2025 17:35
@petern48 petern48 requested a review from paleolimbot August 29, 2025 17:36
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

@jiayuasu jiayuasu merged commit fbf1642 into apache:main Aug 29, 2025
3 checks passed
@petern48 petern48 deleted the sedona_internal_err branch September 2, 2025 04:36
@petern48 petern48 mentioned this pull request Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants