Skip to content

Make TopKDynamicFilters public - #25429

Merged
kosiew merged 3 commits into
apache:mainfrom
masonh22:pub-topk-dyn-filters
Oct 9, 2026
Merged

kosiew merged 3 commits into
apache:mainfrom
masonh22:pub-topk-dyn-filters

Conversation

@masonh22

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

TopKDynamicFilters is needed as an argument to TopK::try_new(), so without it it's impossible to construct a TopK.

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

This is needed as an argument to `TopK::try_new()`, so without it it's
impossible to construct a `TopK`.
@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 17, 2026
masonh22 added a commit to coralogix/arrow-datafusion that referenced this pull request Sep 17, 2026
@codecov-commenter

codecov-commenter commented Sep 17, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.73%. Comparing base (264ee3d) to head (f2b0922).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25429      +/-   ##
==========================================
+ Coverage   82.72%   82.73%   +0.01%     
==========================================
  Files        1147     1147              
  Lines      448013   449540    +1527     
  Branches   448013   449540    +1527     
==========================================
+ Hits       370602   371916    +1314     
- Misses      54920    54947      +27     
- Partials    22491    22677     +186     

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

@masonh22
masonh22 marked this pull request as ready for review September 21, 2026 17:05
masonh22 added a commit to coralogix/arrow-datafusion that referenced this pull request Sep 21, 2026

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

@masonh22,

Thanks for working on this. The re-export fixes the API visibility issue cleanly without changing runtime behavior. I just have one non-blocking suggestion to make sure this public API path stays covered.

Comment thread datafusion/physical-plan/src/lib.rs

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

@masonh22,

Thanks for addressing the earlier feedback. The crate-root re-export makes TopKDynamicFilters accessible to downstream consumers, and the new rustdoc example provides regression coverage for the public import path and TopK::try_new construction.

All previous comments have been addressed, and I found no new issues in the follow-up changes. LGTM!

@kosiew

kosiew commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🚀
@masonh22
Thank you for your contribution.

@kosiew
kosiew added this pull request to the merge queue Oct 9, 2026
Merged via the queue into apache:main with commit bc693cc Oct 9, 2026
42 checks passed
kosiew

This comment was marked as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants