Move abs to datafusion_functions #9313
Conversation
datafusion/functions/src/math/abs.rs
Outdated
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| //! Encoding expressions |
There was a problem hiding this comment.
Seems that it should be something like:
//! Math function: abs()
| // #[tokio::test] | ||
| // async fn simple_scalar_function_abs() -> Result<()> { | ||
| // roundtrip("SELECT ABS(a) FROM data").await | ||
| // } |
There was a problem hiding this comment.
I disabled this test case because now it seems impossible for substrait to get a ScalarFunction from name. See: https://github.com/apache/arrow-datafusion/blob/91f3eb2e5430d23e2b551e66732bec1a3a575971/datafusion/substrait/src/logical_plan/consumer.rs#L116-L130 and https://github.com/apache/arrow-datafusion/blob/91f3eb2e5430d23e2b551e66732bec1a3a575971/datafusion/substrait/src/logical_plan/consumer.rs#L77-L82
I can try to implement this function in this PR or as a follow-up.
There was a problem hiding this comment.
Also @SteveLauC, I think you also may experience this question also. Cause no test case for acos so CI could work now.
There was a problem hiding this comment.
I think we may need to fix the issue beforehand to avoid regression on this test
There was a problem hiding this comment.
Thanks! @jayzhan211
I applied the changes and this PR should work now.
datafusion/functions/src/macros.rs
Outdated
| @@ -160,3 +160,43 @@ macro_rules! downcast_arg { | |||
| })? | |||
| }}; | |||
| } | |||
|
|
|||
| macro_rules! make_abs_function { | |||
There was a problem hiding this comment.
do we really need to introduce this new macros? would be that possible to reuse existing macros like it was before?
There was a problem hiding this comment.
Yeah, these are not new macros, but from datafusion-physical_expr like https://github.com/apache/arrow-datafusion/pull/9216/files#r1487960276 said.
I think it's also OK to import and reuse the existing macros, but I think in the future if all functions are moved, macros should also move.🤔
There was a problem hiding this comment.
These are macros for abs, I think they should be moved to math.rs. Also, you need to delete those macros in the old file math_expressions.rs
| Acos = 1; | ||
| // 0 was Abs | ||
| // The first enum value must be zero for open enums | ||
| Acos = 0; |
There was a problem hiding this comment.
This is not good that old code that has 0 as Abs will accidentally map to Acos, and 1 is missing. I think you can keep it as it is for this PR
After moving them all to UDF, this enum can be deprecated.
I think we need to add unknown = 0 in the future for enum.
There was a problem hiding this comment.
Good idea!
Applied the suggestions now.
Which issue does this PR close?
Closes #9286
What changes are included in this PR?
Port logic describe in the issue, also I'm wondering whether I can remove related logic in math_expressions.rs, but I'd like to invest more.
Are these changes tested?
Yes
Are there any user-facing changes?
No