Skip to content

Review the need of make_scalar_function for functions #14835

Open

Description

@jayzhan211

Is your feature request related to a problem or challenge?

The current code converts scalars to arrays and then back after the function calculation. However, this conversion is unnecessary and can be optimized, especially in cases involving scalars. The conversion process duplicates values in the array, which doesn't add any value.

This approach was likely implemented when Scalar was not yet introduced in arrow-rs, but that’s no longer the case. With Scalar available, the conversion to arrays is redundant.

For example, the gcd function does not require an array and can instead operate directly on an i64 value #14834

Describe the solution you'd like

Revisit the functions that use make_scalar_function and identify those where it is no longer necessary. If there are functions that no longer require it, remove the usage entirely to streamline the code.

Describe alternatives you've considered

No response

Additional context

No response

Activity

  1. alamb commented on Feb 23, 2025

    @alamb
    Contributor

    Revisit the functions that use make_scalar_function and identify those where it is no longer necessary. If there are functions that no longer require it, remove the usage entirely to streamline the code.

    • I think trying out @findepi 's Simple Functions  #12635 would be an excellent candidate for these functions. The ones that use make_scalar_function are all fairly simple and would benefit from having a special case for constant.

    Ensuring #12635 can handle the basic math functions would be a good validation of the API in general

  2. jayzhan211 commented on Feb 24, 2025

    @jayzhan211
    ContributorAuthor

    I think trying out @findepi 's Simple Functions #12635 would be an excellent candidate for these functions. The ones that use make_scalar_function are all fairly simple and would benefit from having a special case for constant.

    I agree, but it is still far from ready.

  3. jayzhan211 commented on Feb 24, 2025

    @jayzhan211
    ContributorAuthor

    it seems this issue is accidentally added in blog sub-issue

  4. alamb commented on Feb 24, 2025

    @alamb
    Contributor

    it seems this issue is accidentally added in blog sub-issue

    Fixed -- sorry about that -- I am not sure how I did that but will be more careful

  5. findepi commented on Feb 24, 2025

    @findepi
    Member

    I think trying out @findepi 's Simple Functions #12635 would be an excellent candidate for these functions. The ones that use make_scalar_function are all fairly simple and would benefit from having a special case for constant.

    I agree, but it is still far from ready.

    i am not questioning it (i know very well how much more I'd like to do there), but I am curious about outsider's perspective.
    in your eyes, what makes you say "far from ready", @jayzhan211 ?

  6. jayzhan211 commented on Feb 24, 2025

    @jayzhan211
    ContributorAuthor

    To replace the existing function without needing to maintain it, we must ensure that all its current functionality is fully supported. Among everything we have so far, extensibility seems to be the biggest challenge. The more extensible we make the function, the less simple it becomes. Since I haven't seen a viable solution for this in #14668, I believe it is still far from being ready.

  7. alamb commented on Feb 24, 2025

    @alamb
    Contributor

    I think trying out @findepi 's Simple Functions #12635 would be an excellent candidate for these functions. The ones that use make_scalar_function are all fairly simple and would benefit from having a special case for constant.

    I agree, but it is still far from ready.

    i am not questioning it (i know very well how much more I'd like to do there), but I am curious about outsider's perspective. in your eyes, what makes you say "far from ready", @jayzhan211 ?

    FWIW in my opinion, getting to the level of replacing make_scalar_function (and improving the resulting implementations) seems like a good first step towards a more general solution for different types of functions

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