Repository navigation
Conversation
This is needed as an argument to `TopK::try_new()`, so without it it's impossible to construct a `TopK`.
Upstream PR: apache#25429
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
Upstream PR: apache#25429
| pub use crate::statistics::{ChildStats, StatisticsArgs, StatisticsContext}; | ||
| pub use crate::stream::EmptyRecordBatchStream; | ||
| pub use crate::topk::TopK; | ||
| pub use crate::topk::{TopK, TopKDynamicFilters}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, this is a good idea. I'll try to get around to this later today or tomorrow
There was a problem hiding this comment.
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.
Which issue does this PR close?
Rationale for this change
TopKDynamicFiltersis needed as an argument toTopK::try_new(), so without it it's impossible to construct aTopK.What changes are included in this PR?
What is the testing strategy for this PR?
Are there any user-facing changes?