Skip to content
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

Remove "Execution error: " prefix from error messages from Rust #293

Closed
andygrove opened this issue Apr 19, 2024 · 6 comments · Fixed by #544
Closed

Remove "Execution error: " prefix from error messages from Rust #293

andygrove opened this issue Apr 19, 2024 · 6 comments · Fixed by #544
Assignees
Labels
enhancement New feature or request

Comments

@andygrove
Copy link
Member

What is the problem the feature request solves?

We cannot exactly match Spark error messages in some cases because DataFusion is prefixing errors from Rust with "Execution error: ".

Because we are ultimately implementing traits from DataFusion, any CometErrors we create get converted into DataFusion errors:

impl From<CometError> for DataFusionError {
    fn from(value: CometError) -> Self {
        match value {
            CometError::DataFusion { source } => source,
            _ => DataFusionError::Execution(value.to_string()),
        }
    }
}

In errors.rs we use thiserror for formatting errors and we delegate formatting to DataFusion for DataFusionError:

#[error(transparent)]
DataFusion {
    #[from]
    source: DataFusionError,
},

DataFusion adds an "Execution error: " prefix in its formatting:

DataFusionError::Execution(ref desc) => {
    write!(f, "Execution error: {desc}")
}

Describe the potential solution

There seem to be two options:

  1. Add a new variant to DataFusionError which does not do any additional formatting and just returns the underlying error string
  2. Find a work around in the mapping using thiserror

Additional context

No response

@andygrove andygrove added the enhancement New feature or request label Apr 19, 2024
@comphead
Copy link
Contributor

DF still returns formatted messages, physical_plan.execute(...)? will produce a formatted error message so we still have to live with that.

If we looking for sending the Comet message to the user without additional wrapping into Execution error:, then thiserror can help. Another couple of options is hacky one to strip Execution error: , or make a separate method in DF for errors returning direct error messages. The last one imho better option. if you agree I'll create the PR to DF and see where it can bring us

@andygrove
Copy link
Member Author

Thanks @comphead. I think it would be good to add a method in DF that lets us get the underlying error string without a prefix.

@comphead
Copy link
Contributor

My naive approach was to fix the formatter and for some alignment case the prefix won't be added

println!("{}", err) => Execution error: err
println!("{:>}", err) => err

But this solution looks incomplete to me as to_string() for example will format with default format....

@comphead
Copy link
Contributor

Filed apache/datafusion#10186

@comphead
Copy link
Contributor

@andygrove we the proposed by @alamb approach we can do something like coded in https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4d0e0462ab6db7fa63e7791e941eea36

Notice Exec err: is gone.
Btw this is another question on why we do chain CometError -> DataFusionError -> ExecutionError

@comphead comphead self-assigned this May 10, 2024
@comphead
Copy link
Contributor

Depends on DF 38.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants