refactor: improve error handling for datafusion engine #24097
Merged
+101
−27
Conversation
This file contains hidden or 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
type/refactor
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
8 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors error handling in the DataFusion integration by introducing a dedicated error module with proper backtrace support. The changes enable better debugging capabilities by capturing and formatting backtraces when converting RisingWave errors to DataFusion errors.
Key changes:
- Creates a new error module with
to_datafusion_errorfunction that captures backtraces - Enables the "backtrace" feature for DataFusion dependencies to support backtrace functionality
- Replaces the previous inline
boxedhelper function with the new centralized error conversion
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/frontend/src/datafusion/error.rs | New module containing error wrapper struct and to_datafusion_error function with backtrace capture logic |
| src/frontend/src/datafusion/mod.rs | Adds error module declaration and exports to_datafusion_error function, removes inline error conversion |
| src/frontend/src/datafusion/function.rs | Replaces all boxed calls with to_datafusion_error and removes old helper function |
| src/frontend/src/datafusion/iceberg_executor.rs | Updates to use exec_err! macro for more idiomatic DataFusion error handling |
| src/frontend/src/datafusion/iceberg_table_provider.rs | Simplifies return type using DFResult type alias for consistency |
| src/frontend/Cargo.toml | Enables "backtrace" features for datafusion dependencies and includes some unrelated formatting changes |
silver-ymz
force-pushed
the
refactor/datafusion-error-handling
branch
3 times, most recently
from
97eb01c to
dfffbcc
Compare
silver-ymz
force-pushed
the
refactor/datafusion-error-handling
branch
from
dfffbcc to
09e6d61
Compare
Signed-off-by: Mingzhuo Yin <[email protected]>
silver-ymz
force-pushed
the
refactor/datafusion-error-handling
branch
from
09e6d61 to
fe9e006
Compare
8 tasks
BugenZhao
reviewed
Dec 15, 2025
Member
BugenZhao
left a comment
BugenZhao
left a comment
There was a problem hiding this comment.
DataFusion's printing backtrace in Display impl is not a good practice in my own opinion. It's still okay if DataFusionError will be the topmost type. However, if we wrap it back into an RwError, it will confuse our Report and result in very ugly user-facing error message.
Would you add some e2e test case demonstrating the UI?
src/frontend/src/datafusion/error.rs
Outdated
Comment on lines
33
to
37
| if let Some(bt) = std::error::request_ref::<Backtrace>(self.0.as_ref()) { | ||
| write!(f, "{}{}", DataFusionError::BACK_TRACE_SEP, bt)?; | ||
| } else if let Some(bt) = &self.1 { | ||
| write!(f, "{}{}", DataFusionError::BACK_TRACE_SEP, bt)?; | ||
| } |
Member
There was a problem hiding this comment.
Shall we provide backtrace from either source, so that we can unify the fmt impl here?
Contributor
Author
There was a problem hiding this comment.
done in new commit
Contributor
Author
Below is the actual error users see today. It is admittedly ugly, but at the moment there is no clean way to log the detailed backtrace without also exposing it to the user-facing console. We may need to wait Detailed error info |
Member
Actually we would like to refrain from printing backtrace to users via SQL interface. 😕 If embedding a backtrace is mainly for debugging purposes at current stage, then we may temporarily tolerate such user interface. However, if the motivation is to expose the backtrace to users, I don't think it's a good idea. |
Contributor
Author
I agree with it. Ideally, we would only emit the backtrace to logs for debugging purposes and keep the SQL user-facing error concise and clean. Unfortunately, with the current technical constraints, we don’t have a reliable way to separate the two. Given that limitation, if we have to choose between losing the backtrace entirely or tolerating a somewhat ugly error message, I think keeping the backtrace is the lesser evil. It significantly improves debuggability, even though the user experience is not ideal. This is not meant to justify exposing backtraces to users long-term. Once we have a proper mechanism to decouple logging from user-facing errors, we should absolutely switch to that. |
Contributor
Author
Done in new commit |
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.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR improves error handling in the DataFusion integration.
RisingWaveErrortoDataFusionError, it will embed backtrace to formated string like other datafusion errors.Checklist
Documentation
Release note