feat: add macros for DataFusionError variants

[Chen-Yuan-Lai]

Which issue does this PR close?

• Closes Add missing error macro #15491.

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?

[comphead]

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

[Chen-Yuan-Lai]

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?

[comphead]

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)
    }
    };
}

[Chen-Yuan-Lai]

@comphead Thank you for your review. You're right - my current implementation doesn't preserve backtraces.

To preserve backtraces, I think there are two options:

1. Add Context wrapping: We could add backtrace by using Context

   let err = DataFusionError::External(Box::new($ERR))
       .context(format!("backtrace: {}", DataFusionError::get_back_trace()));

   However, this would create double wrapping and might affect error handling logic.

2. Modify DataFusionError enum definitions: Change error variants to include backtrace fields

   External(GenericError, Option<String>),
   ExecutionJoin(JoinError, Option<String>),

   But this would need to change the usage of this error type in the codebase

Do you think which approach is better, or would you recommend a different solution?

[comphead]

Thanks @Chen-Yuan-Lai I think second option is good way to go, more over we already have similar invariants

    /// Error returned by arrow.
    ///
    /// 2nd argument is for optional backtrace
    ArrowError(ArrowError, Option<String>),

[Chen-Yuan-Lai]

Summary of this change

1. Enhancement of DataFusionError Enum Variants

   • Updated the ExecutionJoin and External variants to maintain backtrace

2. Addition of Utility Method for Error Processing

   • Implemented a new method DataFusionError::box_error_if_needed() that handles:
     • Automatic boxing for regular error types
     • Prevention of double-boxing for already boxed errors
     • Support for string-based error messages

3. Improvement of Error-related Macros

   • Capture and include backtraces
   • Utilize the new box_error_if_needed() method
   • Replace all the usage with new macros

[comphead]

tbh I'm not sure about if this method is needed, can we just inline it into the macros where it is being used?

[Chen-Yuan-Lai]

Agree, but to prevent the compiler from resolving the target type for e.into(), I use GenericError::from(e) to explicitly provide the target type

datafusion/datafusion/common/src/error.rs

Lines 926 to 930 in e030422

	let err = $crate::error::DataFusionError::External(
	// covert input to GenericError
	$crate::error::GenericError::from($CONVERTIBLE_TO_ERR),
	Some($crate::error::DataFusionError::get_back_trace())
	);

[comphead]

thanks @Chen-Yuan-Lai I think we really close to it

[alamb]

I have noticed the lack of this macro recently too -- thank you for adding it 🙏

[alamb]

Thank you for this PR @Chen-Yuan-Lai and @comphead -- very nice

I am a little worried about changing the DataFusionError variant as it which will be a disruptive downstream change (users have to update all match statements that currently match on DataFusionError and they won't really see any new functionality

Rather than change the DataFusionError variant, maybe we could model adding a backtrace using DataFusionError::Context

So keep DataFusionError::External and potentially add a backtrace like

DataFusionError::External(..)
  .context(add_backtrace())

Or something like that

[github-actions]

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.