Skip to content

[Epic] Port BuiltInFunctons to datafusion-functions-* crates #9285

New issue
New issue
Closed
#10098
Closed
[Epic] Port BuiltInFunctons to datafusion-functions-* crates#9285
#10098
Labels
enhancementNew feature or request

Description

@alamb
alamb
opened on Feb 20, 2024

Is your feature request related to a problem or challenge?

As part of making DataFusion even more customizable (#8045), it is valuable to let system designers mix and match different packages of functions to get the precise behavior they want (e.g. postgres style to_date or spark style to_date).

To support this functionality as well as to ensure the ScalarUDF API exposes the full power of DataFusion, we are in the process of extracting the "built in" functions out of the core and into separate crates.

This epic tracks the work to actually move the functions out of the core datafusion crate (spread through datafusion_expr and datafusion-physical-expr and into the new datafusion-functions / datafusion-functions-array crates

Tasks:

Here is list of many of the items necessary to complete this transition. Eventually there should be tickets for all tasks, and there are tickets for some already, but I don't want to make 100s of tickets all at once. I plan to make more as we make it through more of this project.

Anyone should feel free to make other tickets if they want to help with items below.

math_expressions

These should be located in the datafusion-functions crate (source link)
Code location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/math/mod.rs

array_expressions

Note that given the size and specialization of these functions are put in their own subcrate, datafusion-functions-array

Core functions

These should be located in the datafusion-functions crate (source link)
Code location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/core/mod.rs

crypto_expressions

These should be located in the datafusion-functions crate (source link)
Code location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/crypto/mod.rs

  • Create crypto module in datafusion/functions/src/crypto and crypto_expressions feature flag, move digest function
  • Digest, MD5, SHA224, SHA256, SHA384, SHA512

string_expressions

These should be located in the datafusion-functions crate (source link)
Code location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/string/mod.rs

  • concat, concat_ws, ends_with, initcap Move take concat, concat_ws, ends_with, initcap, to datafusion-functions #9540
  • Create string module in datafusion/functions/src/string and string_expressions feature flag, move ascii function
  • ascii, bit_length, btrim, chr,
  • instr, lower, ltrim, octet_length,
  • repeat, replace, rtrim, split_part,
  • starts_with, to_hex, trim, upper,
  • levenshtein, uuid, overlay

unicode_expressions

These should be located in the datafusion-functions crate (source link)
Code location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/unicode/mod.rs

  • Create unicode module in datafusion/functions/src/unicode and unicode_expressions feature flag, move charlength function
  • CharLength,
  • Left, Lpad, Reverse, Right, Rpad,
  • Strpos, Substr,
  • Translate, SubstrIndex, FindInSet

regex_expressions

These should be located in the datafusion-functions crate (source link)
Code location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/regexp/mod.rs

datetime_expressions

These should be located in the datafusion-functions crate (source link)
Code location: https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/datetime/mod.rs

  • Create datetime module in datafusion/functions/src/datetime and datetime_expressions feature flag, move date_part
  • Move the to_timestamp* functions to datafusion-functions #9291
  • port benchmarks to datafusion-functions crate
  • date_part, date_trunc, date_bin,
  • to_timestamp, to_timestamp_millis, to_timestamp_micros, to_timestamp_nanos, to_timestamp_seconds,
  • from_unixtime, now, current_date, current_time

Infrastructure

Describe alternatives you've considered

No response

Additional context

The organization was discussed in #9100

Activity

  1. alamb
    added
    enhancementNew feature or request
    on Feb 20, 2024

  2. SteveLauC commented on Feb 21, 2024

    @SteveLauC
    SteveLauC
    on Feb 21, 2024
    Contributor

    Let me give this a try:

    math_expressions

    [ ] Abs, Acos, Asin,

    Update: Abs has been taken in #9286, I will handle the remaining 2 fns

  3. Lordworms commented on Feb 21, 2024

    @Lordworms
    Lordworms
    on Feb 21, 2024
    Contributor

    I'll try to do regex_expressions parts.

  4. guojidan commented on Feb 23, 2024

    @guojidan
    guojidan
    on Feb 23, 2024
    Contributor

    I'll try to do array_expressions parts.

  5. yyy1000 commented on Feb 24, 2024

    @yyy1000
    yyy1000
    on Feb 24, 2024
    Contributor

    I filed #9336 which I think may be necessary before we port functions.

  6. jayzhan211 commented on Feb 27, 2024

    @jayzhan211
    jayzhan211
    on Feb 27, 2024
    Contributor

    work on array_concat

  7. guojidan commented on Feb 27, 2024

    @guojidan
    guojidan
    on Feb 27, 2024
    Contributor

    work on array_concat

    hi @jayzhan211 I already work on array_concat #9343

  8. jayzhan211 commented on Feb 27, 2024

    @jayzhan211
    jayzhan211
    on Feb 27, 2024
    Contributor

    Oh, sorry. I will work on other

  9. jayzhan211 commented on Feb 27, 2024

    @jayzhan211
    jayzhan211
    on Feb 27, 2024
    Contributor

    Take ArrayHas, ArrayHasAll, ArrayHasAny

  10. SteveLauC commented on Feb 28, 2024

    @SteveLauC
    SteveLauC
    on Feb 28, 2024
    Contributor

    Take Atan, Atan2, Acosh.

  11. Weijun-H commented on Feb 29, 2024

    @Weijun-H
    Weijun-H
    on Feb 29, 2024
    Member

    Take ArrayPopFront, ArrayPopBack, ArrayDistinct, ArrayElement

  12. Weijun-H commented on Mar 2, 2024

    @Weijun-H
    Weijun-H
    on Mar 2, 2024
    Member

    Take ArrayDims, ArrayNdims, Cardinality, ArrayNdims
    #9425

  13. jayzhan211 commented on Mar 4, 2024

    @jayzhan211
    jayzhan211
    on Mar 4, 2024
    Contributor

    Take ArrayHas, ArrayHasAll, ArrayHasAny

    I think this needs to wait until make_array is done and also move to rewrite to the optimizer.

    Take ArrayIntersect, ArrayUnion, ArrayExcept

    Edit: general_set_op needs make_array too

    Take: ArraySlice + ArrayElement

  14. jayzhan211 commented on Mar 4, 2024

    @jayzhan211
    jayzhan211
    on Mar 4, 2024
    Contributor

    While I worked on ArrayElement, I found that GetFieldAccessExpr::ListIndex uses array_element in physical-expr https://github.com/apache/arrow-datafusion/blob/581fd98270e69221689ddd4c37566ef620a67167/datafusion/physical-expr/src/expressions/get_indexed_field.rs#L252-L257 ListRange likewise, which is kernel function in function-arrays crate, instead of converting ArrayRef to Expr, I'm thinking of making array function kernel public. @alamb Do you think we can make those array functions public? And, is importing function-arrays to crate physical-expr a concern like crate optimize is?

    If neither is a good idea, we may need to rewrite ListIndex and ListRange to array function early.

  15. alamb commented on Mar 4, 2024

    @alamb
    alamb
    on Mar 4, 2024
    ContributorAuthor

    ListRange likewise, which is kernel function in function-arrays crate, instead of converting ArrayRef to Expr, I'm thinking of making array function kernel public. @alamb Do you think we can make those array functions public? And, is importing function-arrays to crate physical-expr a concern like crate optimize is?

    I think making physical-exprs depend on datafusion-functions would be unfortunate (as it would still mean there could be dependencies / special cases from built in functions to "built in" user defined functions)

    If neither is a good idea, we may need to rewrite ListIndex and ListRange to array function early.

    I agree if this is possible it would be the best approach from my perspective

    I also though about about potentially adding the common code to the new datafusion_common_runtime crate (that @mustafasrepo added recently)

    https://github.com/apache/arrow-datafusion/tree/main/datafusion/common_runtime

    But I think that would result in a tokio dependency on datafusion-functions which would be unwise.

  16. 15 remaining items

  17. jayzhan211 commented on Mar 15, 2024

    @jayzhan211
    jayzhan211
    on Mar 15, 2024
    Contributor

    Take pop_front and pop_back, they need to be moved together with slice

  18. Weijun-H commented on Mar 16, 2024

    @Weijun-H
    Weijun-H
    on Mar 16, 2024
    Member

    Take ArrayReverse

  19. erenavsarogullari commented on Mar 16, 2024

    @erenavsarogullari
    erenavsarogullari
    on Mar 16, 2024
    Member

    ArrayExcept: PR(#9634) has been submitted
    ArrayRemove, ArrayRemoveN, ArrayRemoveAll: Work in Progress(#9635)

  20. Weijun-H commented on Mar 17, 2024

    @Weijun-H
    Weijun-H
    on Mar 17, 2024
    Member

    take array_replace family

  21. erenavsarogullari commented on Mar 18, 2024

    @erenavsarogullari
    erenavsarogullari
    on Mar 18, 2024
    Member

    @jayzhan211 Just follow-up to our previous discussion (single func per file), we almost complete functions-array migration. Its kernels and udf source files include more functions by our new pattern. Does it make sense to break them as well by aligning with the new pattern for the consistency?

  22. jayzhan211 commented on Mar 18, 2024

    @jayzhan211
    jayzhan211
    on Mar 18, 2024 via email
    Contributor

  23. jayzhan211 commented on Mar 18, 2024

    @jayzhan211
    jayzhan211
    on Mar 18, 2024
    Contributor

    @erenavsarogullari
    For consistency, I think we can also change the invoke with make_scalar_function (probably have a better name). It returns ColumnValue::Scalar if length of list is 1. It has no difference currently, since we have no optimization based on Scalar.

  24. erenavsarogullari commented on Mar 18, 2024

    @erenavsarogullari
    erenavsarogullari
    on Mar 18, 2024
    Member

    @jayzhan211 Yes, makes sense. I have created #9660 for this refactoring effort. I will also include to be used make_scalar_function for invoke calls.

  25. tinfoil-knight commented on Mar 19, 2024

    @tinfoil-knight
    tinfoil-knight
    on Mar 19, 2024
    Contributor

    Take Ln, Log, Log10, Log2

    Edit: Take Power since it's related to Log

  26. Omega359 commented on Mar 20, 2024

    @Omega359
    Omega359
    on Mar 20, 2024
    Contributor

    Take lower, ltrim, octet_length. Edit: all the trim functions use a common general function so I'll migrate all the trim functions first - rtrim, ltrim and btrim.

  27. Omega359 commented on Mar 23, 2024

    @Omega359
    Omega359
    on Mar 23, 2024
    Contributor

    take levenshtein, uuid, overlay

  28. Weijun-H commented on Mar 30, 2024

    @Weijun-H
    Weijun-H
    on Mar 30, 2024
    Member

    take Atan, Acosh, Asinh, Atanh, #9872

  29. alamb
    closed this as completedin #10098on Apr 22, 2024
  30. Michael-J-Ward
    added a commit that references this issue on May 8, 2024

    feat: migrate functions.rs

    7ad526c
  31. andygrove
    added a commit that references this issue on May 8, 2024

    Upgrade Datafusion to v37.1.0 (#669)

    ee93cdd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions