-
Notifications
You must be signed in to change notification settings - Fork 1k
Deprecate set_bits in favor of apply_bitwise_binary_op
#8833
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
base: main
Are you sure you want to change the base?
Conversation
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
Thank you. Looking forward to the perf tests. +1 for the rational. |
WIP: I am verifying this PR is faster via benchmarks and if so I will polish is up and mark it ready for review
Which issue does this PR close?
Rationale for this change
apply_unary_opandapply_binary_opbitwise operations #8619, it appears thatapply_bitwise_binary_opis faster and more general thanset_bits, so let's deprecate set_bits in favor of it.This is part of a broader goal to consolidate the bitwise operations in DataFusion so that we can focus additional optimization energy on them (aka bithacks for the win)
set_bitswas introduced by @kazuyukitanimura inWhat changes are included in this PR?
set_bitswith a note to use apply_bitwise_binary_op instead.set_bitsto useapply_bitwise_binary_opinternallyAre these changes tested?
Covered by existing tests
I will also performance test
Are there any user-facing changes?
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.