You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/04/08 11:10:13 UTC
[PR] Remove uneeded binary_op benchmarks [datafusion]
alamb opened a new pull request, #15632:
URL: https://github.com/apache/datafusion/pull/15632
## Which issue does this PR close?
- part of https://github.com/apache/datafusion/issues/15631
- follow up to https://github.com/apache/datafusion/pull/15462
## Rationale for this change
Some of these benchmarks were written for the wonderful analysis by @acking-you in https://github.com/apache/datafusion/pull/15462 of bool_or vs count_ones. While they were helpful for that analysis, they really are micro benchmarks for arrow kernels, not DataFusion so I think they don't belong in this repo.
Since most of the binary.rs operations call into arrow kernels, I didn't think adding more was useful at this time.
## What changes are included in this PR?
1. Remove arrow kernel benchmarks
2. rename the short circuit benchmarks
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## 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 add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove uneeded binary_op benchmarks [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #15632:
URL: https://github.com/apache/datafusion/pull/15632#discussion_r2032958303
##########
datafusion/physical-expr/benches/binary_op.rs:
##########
@@ -130,75 +129,6 @@ fn generate_boolean_cases<const TEST_ALL_FALSE: bool>(
cases
}
-/// Benchmarks boolean operations `false_count/bool_or` and `true_count/bool_and` on [`BooleanArray`]
-/// You can run this benchmark with:
-/// ```sh
-/// # test true_count/false_count
-/// TEST_BOOL_COUNT=1 cargo bench --bench binary_op -- boolean_ops
-/// # test bool_or/bool_and
-/// cargo bench --bench binary_op -- boolean_ops
-/// ```
-fn benchmark_boolean_ops(c: &mut Criterion) {
- let len = 1_000_000; // Use one million elements for clear performance differentiation
- static TEST_BOOL_COUNT: LazyLock<bool> =
- LazyLock::new(|| match std::env::var("TEST_BOOL_COUNT") {
- Ok(_) => {
- println!("TEST_BOOL_COUNT=ON");
- true
- }
- Err(_) => {
- println!("TEST_BOOL_COUNT=OFF");
- false
- }
- });
-
- // Determine the test function to be executed based on the ENV `TEST_BOOL_COUNT`
- fn test_func<const TEST_ALL_FALSE: bool>(array: &BooleanArray) -> bool {
- // Use false_count for all false and true_count for all true
- if *TEST_BOOL_COUNT {
- if TEST_ALL_FALSE {
- array.false_count() == array.len()
- } else {
- array.true_count() == array.len()
- }
- }
- // Use bool_or for all false and bool_and for all true
- else if TEST_ALL_FALSE {
- match bool_or(array) {
- Some(v) => !v,
- None => false,
- }
- } else {
- bool_and(array).unwrap_or(false)
Review Comment:
these are benchmarks for bool_and and false_count / true_count
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove uneeded binary_op benchmarks [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #15632:
URL: https://github.com/apache/datafusion/pull/15632#discussion_r2032958303
##########
datafusion/physical-expr/benches/binary_op.rs:
##########
@@ -130,75 +129,6 @@ fn generate_boolean_cases<const TEST_ALL_FALSE: bool>(
cases
}
-/// Benchmarks boolean operations `false_count/bool_or` and `true_count/bool_and` on [`BooleanArray`]
-/// You can run this benchmark with:
-/// ```sh
-/// # test true_count/false_count
-/// TEST_BOOL_COUNT=1 cargo bench --bench binary_op -- boolean_ops
-/// # test bool_or/bool_and
-/// cargo bench --bench binary_op -- boolean_ops
-/// ```
-fn benchmark_boolean_ops(c: &mut Criterion) {
- let len = 1_000_000; // Use one million elements for clear performance differentiation
- static TEST_BOOL_COUNT: LazyLock<bool> =
- LazyLock::new(|| match std::env::var("TEST_BOOL_COUNT") {
- Ok(_) => {
- println!("TEST_BOOL_COUNT=ON");
- true
- }
- Err(_) => {
- println!("TEST_BOOL_COUNT=OFF");
- false
- }
- });
-
- // Determine the test function to be executed based on the ENV `TEST_BOOL_COUNT`
- fn test_func<const TEST_ALL_FALSE: bool>(array: &BooleanArray) -> bool {
- // Use false_count for all false and true_count for all true
- if *TEST_BOOL_COUNT {
- if TEST_ALL_FALSE {
- array.false_count() == array.len()
- } else {
- array.true_count() == array.len()
- }
- }
- // Use bool_or for all false and bool_and for all true
- else if TEST_ALL_FALSE {
- match bool_or(array) {
- Some(v) => !v,
- None => false,
- }
- } else {
- bool_and(array).unwrap_or(false)
Review Comment:
these are benchmarks for bool_and and false_count / true_count so I don't think we need them in DataFusion
##########
datafusion/physical-expr/benches/binary_op.rs:
##########
@@ -130,75 +129,6 @@ fn generate_boolean_cases<const TEST_ALL_FALSE: bool>(
cases
}
-/// Benchmarks boolean operations `false_count/bool_or` and `true_count/bool_and` on [`BooleanArray`]
-/// You can run this benchmark with:
-/// ```sh
-/// # test true_count/false_count
-/// TEST_BOOL_COUNT=1 cargo bench --bench binary_op -- boolean_ops
-/// # test bool_or/bool_and
-/// cargo bench --bench binary_op -- boolean_ops
-/// ```
-fn benchmark_boolean_ops(c: &mut Criterion) {
- let len = 1_000_000; // Use one million elements for clear performance differentiation
- static TEST_BOOL_COUNT: LazyLock<bool> =
- LazyLock::new(|| match std::env::var("TEST_BOOL_COUNT") {
- Ok(_) => {
- println!("TEST_BOOL_COUNT=ON");
- true
- }
- Err(_) => {
- println!("TEST_BOOL_COUNT=OFF");
- false
- }
- });
-
- // Determine the test function to be executed based on the ENV `TEST_BOOL_COUNT`
- fn test_func<const TEST_ALL_FALSE: bool>(array: &BooleanArray) -> bool {
- // Use false_count for all false and true_count for all true
- if *TEST_BOOL_COUNT {
- if TEST_ALL_FALSE {
- array.false_count() == array.len()
- } else {
- array.true_count() == array.len()
- }
- }
- // Use bool_or for all false and bool_and for all true
- else if TEST_ALL_FALSE {
- match bool_or(array) {
- Some(v) => !v,
- None => false,
- }
- } else {
- bool_and(array).unwrap_or(false)
Review Comment:
these are benchmarks for arrow functions bool_and and false_count / true_count so I don't think we need them in DataFusion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove uneeded binary_op benchmarks [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15632:
URL: https://github.com/apache/datafusion/pull/15632#issuecomment-2789215249
Thank you @Dandandan and @xudong963
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove uneeded binary_op benchmarks [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #15632:
URL: https://github.com/apache/datafusion/pull/15632
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org