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

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-actionsgithub-actionsBot 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 linesPatch %Lines
datafusion/physical-plan/src/topk/mod.rs86.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.

@jayzhan211jayzhan211 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

@alambalamb changed the title Fix topk emitted batchesFix topk emitted batches metricAug 20, 2026

@alambalamb 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

@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

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
cetra3force-pushed the fix_topk_emitted_batches branch from e88598c to 021451fCompareAugust 26, 2026 01:21

@kumarUjjawalkumarUjjawal 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 queueAug 26, 2026
Merged via the queue into apache:main with commit 54557c3Aug 26, 2026
41 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion cratephysical-planChanges to the physical-plan crate

Projects

None yet

5 participants

@cetra3@codecov-commenter@kumarUjjawal@alamb@jayzhan211