Skip to content

Make TopKDynamicFilters public - #25429

Open
masonh22 wants to merge 3 commits into
apache:mainfrom
masonh22:pub-topk-dyn-filters
Open

masonh22 wants to merge 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.

pub use crate::statistics::{ChildStats, StatisticsArgs, StatisticsContext};
pub use crate::stream::EmptyRecordBatchStream;
pub use crate::topk::TopK;
pub use crate::topk::{TopK, TopKDynamicFilters};

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.

Could you add a compiling rustdoc example or external integration test that imports both types from the crate root, constructs TopKDynamicFilters, and passes it to TopK::try_new? Tests inside the private topk module would still pass without this re-export, so a public-path compilation test would catch this visibility issue if it regresses.

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.

Yes, this is a good idea. I'll try to get around to this later today or tomorrow

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.

I had claude add a trivial doc example and validated that reverting my fix causes the doc test to fail. Let me know what you think, I'm happy to make changes.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants