Skip to content

feat: add macros for DataFusionError variants #15946

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Chen-Yuan-Lai
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

As #15491 says, add two macros for

  • External
  • ExecutionJoin

Moreover, the macro for ResourcesExhausted already existed, so we don't need to add it in the PR

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the common Related to common crate label May 5, 2025
@@ -808,12 +808,18 @@ make_error!(plan_err, plan_datafusion_err, Plan);
// Exposes a macro to create `DataFusionError::Internal` with optional backtrace
make_error!(internal_err, internal_datafusion_err, Internal);

// Exposes a macro to create `DataFusionError::IoError` with optional backtrace
make_error!(external_err, external_datafusion_err, External);
Copy link
Contributor

Choose a reason for hiding this comment

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

wasn't there any current usage of this error types it to replace with macros?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @comphead, I noticed that using make_error! isn't suitable for these error variants because External and ExecutionJoin are designed to wrap original error objects, not just error messages.

Instead, I've implemented custom macros for External and ExecutionJoin errors, similar to #15796, and replaced a few use cases as examples. Should we replace all the cases?

@Chen-Yuan-Lai Chen-Yuan-Lai marked this pull request as draft May 6, 2025 17:56
@github-actions github-actions bot added core Core DataFusion crate datasource Changes to the datasource crate labels May 7, 2025
@Chen-Yuan-Lai Chen-Yuan-Lai marked this pull request as ready for review May 7, 2025 09:41
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

The key point of this macros is preserving backtrace which makes debugging easier if you got an error from datafusion among multiple layers of processing.

The variant below is for non string errors preserving the backtrace

#[macro_export]
macro_rules! schema_err {
    ($ERR:expr $(; diagnostic = $DIAG:expr)?) => {{
        let err = $crate::error::DataFusionError::SchemaError(
            $ERR,
            Box::new(Some($crate::error::DataFusionError::get_back_trace())),
        );
        $(
            let err = err.with_diagnostic($DIAG);
        )?
        Err(err)
    }
    };
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate datasource Changes to the datasource crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing error macro
2 participants