Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removed last usages of scalar_inputs, scalar_input_types and inputs2 to use arrow unary/binary for performance #12972

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Removed last usages of scalar_inputs, scalar_input_types and inputs2 to use arrow unary/binary for performance #12972

wants to merge 5 commits into from
+144 −214

Conversation

buraksenn
Copy link

@buraksenn buraksenn commented Oct 16, 2024 •
edited
Loading

Which issue does this PR close?

Closes #12923 .

Rationale for this change

Copy-pasta'ing the issue description here:
In #12881, #12890 it turned out that make_function_scalar_inputs_return_type may lead to less performant code.
In #12909 it turned out that make_function_inputs2 may lead to less performant code.

That's why this PR refactores their usages.

What changes are included in this PR?

  • Remove make_function_scalar_inputs macro
  • Remove make_function_scalar_inputs_return_type
  • Remove make_function_inputs2
  • Refactor their usages throughout the app

Are these changes tested?

Existing testcases

I can add benchmarks if requested

@buraksenn buraksenn changed the title removed last usages of make_function_scalar_inputs to make it more performant Removed last usages of make_function_scalar_inputs to make it more performant Oct 16, 2024
berkaysynnada
berkaysynnada approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@berkaysynnada berkaysynnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @buraksenn. I wonder how CI passed before the last commit 🤔

@buraksenn
Copy link
Author

buraksenn commented Oct 18, 2024 •
edited by berkaysynnada
Loading

LGTM, thank you @buraksenn. I wonder how CI passed before the last commit 🤔

Thanks for reviewing the PR.

The reason was that in a few lines above:

let mut x = &args[0];
Thus using args[0] did not cause any errors but of course not a best practice afais.

@berkaysynnada
Copy link
Contributor

let mut x = &args[0]; Thus using args[0] did not cause any errors but of course not a best practice afais.

x is re-set to args[1] if log has two parameters, so using args[0] should have caused inconsistent behavior (sometimes pointing to the base of log, sometimes to the actual log value) in that commit. This might have uncovered a bug. I am planning to investigate it further.

@buraksenn
Copy link
Author

let mut x = &args[0]; Thus using args[0] did not cause any errors but of course not a best practice afais.

x is re-set to args[1] if log has two parameters, so using args[0] should have caused inconsistent behavior (sometimes pointing to the base of log, sometimes to the actual log value) in that commit. This might have uncovered a bug. I am planning to investigate it further.

I saw that now. Then probably test case miss this. I can add further tests on this

@buraksenn buraksenn changed the title Removed last usages of make_function_scalar_inputs to make it more performant Removed last usages of scalar_inputs, scalar_input_types and inputs2 to use arrow unary/binary for performance Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkaysynnada berkaysynnada berkaysynnada approved these changes

Assignees
No one assigned
Labels
functions
Projects
None yet
Milestone
No milestone
2 participants
@buraksenn @berkaysynnada