Skip to content

Sql benchmark sync updated queries - #24583

Merged
alamb merged 4 commits into
apache:mainfrom
Omega359:sql_benchmark_sync_updated_queries
Aug 24, 2026
Merged

Sql benchmark sync updated queries#24583
alamb merged 4 commits into
apache:mainfrom
Omega359:sql_benchmark_sync_updated_queries

Conversation

@Omega359

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.38%. Comparing base (5134a1a) to head (f934bed).

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @Omega359

Looks good!


run
-- Window Top-N partition cardinality sweep (id3 % N gives N distinct partitions).
-- These exercise PartitionedTopKExec across cardinalities to validate it stays

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since none of q13–q29 set it in init, I think these all run the SortExec + FilterExec baseline rather than the PartitionedTopKExec path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kumarUjjawal is right, enable_window_topn defaults to false and the rule hard-exits on it before anything else:

  // 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am having a hard time following this -- so what changes are needed in this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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).

@alamb alamb Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, so I'll merge this PR in and then we can adjust as part of the removal of h20 rust runner

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @Omega359

@alamb
alamb added this pull request to the merge queue Aug 24, 2026
Merged via the queue into apache:main with commit c699996 Aug 24, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants