Skip to content

Fix topk emitted batches metric - #24481

Merged
kumarUjjawal merged 2 commits into
apache:mainfrom
pydantic:fix_topk_emitted_batches
Aug 26, 2026
Merged

Fix topk emitted batches metric#24481
kumarUjjawal merged 2 commits into
apache:mainfrom
pydantic:fix_topk_emitted_batches

Conversation

@cetra3

@cetra3 cetra3 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Fixes some issues around metrics and TopK execution nodes

What changes are included in this PR?

Adjusts to include metrics in PartitionedTopKExec and also fixes the batch count in TopK

Are these changes tested?

Yes new tests added.

Are there any user-facing changes?

No new traits, etc.. just fixes some bugs on the physical plan side

@github-actions github-actions Bot added core Core DataFusion crate physical-plan Changes to the physical-plan crate labels Aug 19, 2026
@codecov-commenter

codecov-commenter commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.17949% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.45%. Comparing base (1f5be0e) to head (021451f).

Files with missing lines Patch % Lines
datafusion/physical-plan/src/topk/mod.rs 86.66% 5 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24481      +/-   ##
==========================================
- Coverage   81.45%   81.45%   -0.01%     
==========================================
  Files        1118     1118              
  Lines      399555   399627      +72     
  Branches   399555   399627      +72     
==========================================
+ Hits       325462   325516      +54     
- Misses      55097    55107      +10     
- Partials    18996    19004       +8     

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

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

Thanks @cetra3

@alamb alamb changed the title Fix topk emitted batches Fix topk emitted batches metric Aug 20, 2026

@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 @cetra3 and @jayzhan211

@Dandandan
Dandandan enabled auto-merge August 20, 2026 20:54
@kumarUjjawal
kumarUjjawal disabled auto-merge August 22, 2026 07:43
@kumarUjjawal
kumarUjjawal enabled auto-merge August 22, 2026 07:43
@kumarUjjawal

kumarUjjawal commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

hmm! there seems to be some issue with spark actions. I will manually trigger the re-run see if that fixes it.

@kumarUjjawal
kumarUjjawal disabled auto-merge August 22, 2026 07:52
@kumarUjjawal

Copy link
Copy Markdown
Contributor

@cetra3 looks like you will have to rebase with main once.

auto-merge was automatically disabled August 26, 2026 01:21

Head branch was pushed to by a user without write access

@cetra3
cetra3 force-pushed the fix_topk_emitted_batches branch from e88598c to 021451f Compare August 26, 2026 01:21

@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 @cetra3 for resolving the issue.

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

Labels

core Core DataFusion crate physical-plan Changes to the physical-plan crate

Projects

None yet

5 participants