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.

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

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

Merged
merged 10 commits into from
Oct 21, 2024

Conversation

buraksenn
Copy link
Contributor

@buraksenn buraksenn commented Oct 16, 2024

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
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
Contributor Author

buraksenn commented Oct 18, 2024

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
Contributor 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
@berkaysynnada berkaysynnada merged commit edeca39 into apache:main Oct 21, 2024
24 checks passed
@berkaysynnada
Copy link
Contributor

Thanks again @buraksenn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants