search

apache / datafusion-comet

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

abs implementation seems incorrect #642

Closed andygrove closed 1 week ago

andygrove commented 3 weeks ago

Describe the bug

Describe the bug

I was reviewing the abs logic as part of https://github.com/apache/datafusion-comet/pull/638 and noticed that it seems incorrect.

In the following code, when running in Legacy mode, it looks like we return the original array if any of the values cause an overflow. What we should be doing instead is returning the original value for the items in the input array that would cause overflow but return the abs of the other values.

Tests are passing, so I may just be misunderstanding something here.

        match self.inner_abs_func.invoke(args) {
            Err(DataFusionError::ArrowError(ArrowError::ComputeError(msg), _trace))
                if msg.contains("overflow") =>
            {
                if self.eval_mode == EvalMode::Legacy {
                    Ok(args[0].clone())
andygrove commented 3 weeks ago

I added tests in https://github.com/apache/datafusion-comet/pull/638 that confirm the bug

vaibhawvipul commented 3 weeks ago

I will work on this after #638 is merged.

andygrove commented 1 week ago

this is a duplicate of https://github.com/apache/datafusion-comet/issues/666