datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Move `abs` to `datafusion_functions`

Open alamb opened this issue 1 year ago • 2 comments

Is your feature request related to a problem or challenge?

Part of https://github.com/apache/arrow-datafusion/issues/9285 and https://github.com/apache/arrow-datafusion/issues/8045

The goal is to extract function definitions out of the datafusion core.

Describe the solution you'd like

Move the abs function to datafusion-functions

Target location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/math

Here is an example function: isnan: https://github.com/apache/arrow-datafusion/blob/e1f7b245168c5762135abc4e594bd81c508d7186/datafusion/functions/src/math/nans.rs#L31-L48

There is already coverage in https://github.com/apache/arrow-datafusion/blob/main/datafusion/sqllogictest/test_files/scalar.slt

Here are the steps I followed when porting isnan:

  1. Create a ScalarUDFImpl in the functions crate following an existing example, and stub out the invoke() function with todo!().
  2. Make sure cargo check -p datafusion-functions --all-features compiles successfully
  3. Remove the enum in BuiltInScalarFunctions (source link)
  4. Try and build with cargo check -p datafusion and the compiler will point out all the places in the code that has logic for this function
  5. While removing old code, copy the relevant parts back into the UDFs (like signature, and return_type and implementation for invoke()
  6. Verify that the sqllogictests pass: `

Describe alternatives you've considered

No response

Additional context

No response

alamb avatar Feb 20 '24 07:02 alamb

I have tried to make this as well specified as possible and thus I think it would be a good first issue. However, we don't have a huge list of these functions yet so if you hit a snag let us know

alamb avatar Feb 20 '24 07:02 alamb

take

yyy1000 avatar Feb 20 '24 19:02 yyy1000

❤️

alamb avatar Feb 27 '24 13:02 alamb