apache / datafusion-comet

Apache DataFusion Comet Spark Accelerator
https://datafusion.apache.org/comet
Apache License 2.0
653 stars 119 forks source link

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

Closed andygrove closed 3 weeks ago

andygrove commented 2 months ago

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

comphead commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Filed https://github.com/apache/datafusion/pull/10186

comphead commented 2 months ago

@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 commented 1 month ago

Depends on DF 38.0.0