You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Omega359 (via GitHub)" <gi...@apache.org> on 2026/08/22 20:15:01 UTC
[PR] Sql benchmark sync updated queries [datafusion]
Omega359 opened a new pull request, #24583:
URL: https://github.com/apache/datafusion/pull/24583
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Part of #21706
## Rationale for this change
Updated sql queries to match existing benchmark queries, mostly #22885 and #24050
## What changes are included in this PR?
sql benchmark query updates.
## Are these changes tested?
Yes
## Are there any user-facing changes?
No
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #24583:
URL: https://github.com/apache/datafusion/pull/24583#discussion_r3837685822
##########
benchmarks/sql_benchmarks/h2o/benchmarks/window/q14.benchmark:
##########
@@ -0,0 +1,26 @@
+subgroup window
+
+name Q14
+group h2o
+
+echo Loading ${H2O_BENCH_SIZE:-small} window ${H2O_FILE_TYPE:-csv} h2o data
+
+load sql_benchmarks/h2o/init/load_window_${H2O_BENCH_SIZE:-small}_${H2O_FILE_TYPE:-csv}.sql
+
+assert I
+SELECT COUNT(*) > 0 FROM x
+----
+true
+
+run
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
Review Comment:
Since none of q13–q29 set it in `init`, I think these all run the `SortExec + FilterExec` baseline rather than the `PartitionedTopKExec` path?
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #24583:
URL: https://github.com/apache/datafusion/pull/24583
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "SubhamSinghal (via GitHub)" <gi...@apache.org>.
SubhamSinghal commented on code in PR #24583:
URL: https://github.com/apache/datafusion/pull/24583#discussion_r3840907167
##########
benchmarks/sql_benchmarks/h2o/benchmarks/window/q14.benchmark:
##########
@@ -0,0 +1,26 @@
+subgroup window
+
+name Q14
+group h2o
+
+echo Loading ${H2O_BENCH_SIZE:-small} window ${H2O_FILE_TYPE:-csv} h2o data
+
+load sql_benchmarks/h2o/init/load_window_${H2O_BENCH_SIZE:-small}_${H2O_FILE_TYPE:-csv}.sql
+
+assert I
+SELECT COUNT(*) > 0 FROM x
+----
+true
+
+run
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
Review Comment:
@kumarUjjawal is right, `enable_window_topn` defaults to `false` and the rule hard-exits on it before anything else:
```rust
// physical-optimizer/src/window_topn.rs
if !config.optimizer.enable_window_topn {
return Ok(plan);
}
```
q13–q29 have been timing the SortExec + FilterExec baseline all along.
Just adding SET ... = true isn't right either: the comment's whole claim is that these stay competitive with the baseline, so the sweep needs both arms, not a flip to the other one.
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #24583:
URL: https://github.com/apache/datafusion/pull/24583#discussion_r3839819476
##########
benchmarks/sql_benchmarks/h2o/benchmarks/window/q14.benchmark:
##########
@@ -0,0 +1,26 @@
+subgroup window
+
+name Q14
+group h2o
+
+echo Loading ${H2O_BENCH_SIZE:-small} window ${H2O_FILE_TYPE:-csv} h2o data
+
+load sql_benchmarks/h2o/init/load_window_${H2O_BENCH_SIZE:-small}_${H2O_FILE_TYPE:-csv}.sql
+
+assert I
+SELECT COUNT(*) > 0 FROM x
+----
+true
+
+run
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
Review Comment:
Quite possible @kumarUjjawal. I went through the h2o.rs and the window.sql that these queries came from and I don't see anything that would change that, at least from a config perspective.
The only thing I noted is that the defaults for almost everything related to topk in the config is true except for enable_window_topn. All of enable_topk_aggregation, enable_topk_repartition, enable_topk_dynamic_filter_pushdown, enable_dynamic_filter_pushdown, and enable_sort_pushdown default to true. So perhaps that covers it?
@SubhamSinghal - do you have an answer for this?
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24583:
URL: https://github.com/apache/datafusion/pull/24583#issuecomment-5382523562
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24583?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.38%. Comparing base ([`5134a1a`](https://app.codecov.io/gh/apache/datafusion/commit/5134a1adff1498e18cc3af34a79aaa62ccc4efb8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f934bed`](https://app.codecov.io/gh/apache/datafusion/commit/f934bed9d4712b0f89016cbf556182914ffbc5a1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24583 +/- ##
==========================================
- Coverage 81.38% 81.38% -0.01%
==========================================
Files 1116 1116
Lines 397960 397960
Branches 397960 397960
==========================================
- Hits 323880 323875 -5
- Misses 55120 55124 +4
- Partials 18960 18961 +1
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24583?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24583:
URL: https://github.com/apache/datafusion/pull/24583#discussion_r3846478425
##########
benchmarks/sql_benchmarks/h2o/benchmarks/window/q14.benchmark:
##########
@@ -0,0 +1,26 @@
+subgroup window
+
+name Q14
+group h2o
+
+echo Loading ${H2O_BENCH_SIZE:-small} window ${H2O_FILE_TYPE:-csv} h2o data
+
+load sql_benchmarks/h2o/init/load_window_${H2O_BENCH_SIZE:-small}_${H2O_FILE_TYPE:-csv}.sql
+
+assert I
+SELECT COUNT(*) > 0 FROM x
+----
+true
+
+run
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
Review Comment:
I am having a hard time following this -- so what changes are needed in this PR?
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #24583:
URL: https://github.com/apache/datafusion/pull/24583#discussion_r3847374592
##########
benchmarks/sql_benchmarks/h2o/benchmarks/window/q14.benchmark:
##########
@@ -0,0 +1,26 @@
+subgroup window
+
+name Q14
+group h2o
+
+echo Loading ${H2O_BENCH_SIZE:-small} window ${H2O_FILE_TYPE:-csv} h2o data
+
+load sql_benchmarks/h2o/init/load_window_${H2O_BENCH_SIZE:-small}_${H2O_FILE_TYPE:-csv}.sql
+
+assert I
+SELECT COUNT(*) > 0 FROM x
+----
+true
+
+run
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
Review Comment:
@alamb Currently this PR mirrors the setup for the rust (h2o.rs) version, whatever the fix is I think either is outside of the benchmarks or applies to both types (till the rust-based benchmarks are removed).
--
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] Sql benchmark sync updated queries [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24583:
URL: https://github.com/apache/datafusion/pull/24583#discussion_r3847661201
##########
benchmarks/sql_benchmarks/h2o/benchmarks/window/q14.benchmark:
##########
@@ -0,0 +1,26 @@
+subgroup window
+
+name Q14
+group h2o
+
+echo Loading ${H2O_BENCH_SIZE:-small} window ${H2O_FILE_TYPE:-csv} h2o data
+
+load sql_benchmarks/h2o/init/load_window_${H2O_BENCH_SIZE:-small}_${H2O_FILE_TYPE:-csv}.sql
+
+assert I
+SELECT COUNT(*) > 0 FROM x
+----
+true
+
+run
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
Review Comment:
ok, so I'll merge this PR in and then we can adjust as part of the removal of h20
##########
benchmarks/sql_benchmarks/h2o/benchmarks/window/q14.benchmark:
##########
@@ -0,0 +1,26 @@
+subgroup window
+
+name Q14
+group h2o
+
+echo Loading ${H2O_BENCH_SIZE:-small} window ${H2O_FILE_TYPE:-csv} h2o data
+
+load sql_benchmarks/h2o/init/load_window_${H2O_BENCH_SIZE:-small}_${H2O_FILE_TYPE:-csv}.sql
+
+assert I
+SELECT COUNT(*) > 0 FROM x
+----
+true
+
+run
+-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
+-- These exercise PartitionedTopKExec across cardinalities to validate it stays
Review Comment:
ok, so I'll merge this PR in and then we can adjust as part of the removal of h20 rust runner
--
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