refactor: make BloomFilterProperties fpp/ndv private with accessors - #9969
Merged
alamb merged 1 commit intoMay 14, 2026
Merged
Conversation
- Replace public `fpp` / `ndv` fields on `BloomFilterProperties` with `pub fn fpp()` and `pub fn ndv()` accessors
- Move the original per-field documentation verbatim onto the accessor methods so rustdoc renders it
- BREAKING CHANGE: external struct-literal construction (`BloomFilterProperties { fpp, ndv }`) and direct field reads no longer compile.
etseidl
approved these changes
May 13, 2026
etseidl
left a comment
etseidl
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me. This is a breaking change, but luckily 'tis the season. 😄
Contributor
🎄 |
Contributor
|
Thank you @CuteChuanChuan @adriangb and @etseidl |
Rich-T-kid
pushed a commit
to Rich-T-kid/arrow-rs
that referenced
this pull request
Jun 2, 2026
…apache#9969) # Which issue does this PR close? - Follow-up to apache#9877; completes the remaining scope of apache#9667 (making `BloomFilterProperties` fields non-public and exposing accessors). # Rationale for this change apache#9877 introduced `BloomFilterPropertiesBuilder`. With the builder in place, the public `fpp` / `ndv` fields constrain future evolution. Locking them down behind accessors makes the public API smaller and more evolvable, matching the original direction outlined in apache#9667. # What changes are included in this PR? - Make `BloomFilterProperties::fpp` and `BloomFilterProperties::ndv` private fields - Add `pub fn fpp(&self) -> f64` and `pub fn ndv(&self) -> u64` accessor methods - Move the per-field documentation verbatim onto the accessor methods so it remains rendered by rustdoc (private fields are not rendered) # Are these changes tested? Yes — the existing test suite covers the affected paths # Are there any user-facing changes? **Yes — this is a breaking change to the public API.** - External struct-literal construction no longer compiles: ```rust // Before — no longer works BloomFilterProperties { fpp: 0.01, ndv: 10_000 } // Use the builder added in apache#9877 instead: BloomFilterProperties::builder() .with_fpp(0.01) .with_max_ndv(10_000) .build() ``` - Direct field reads (props.fpp, props.ndv) no longer compile; use props.fpp() / props.ndv() instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
BloomFilterPropertiesfields non-public and exposing accessors).Rationale for this change
#9877 introduced
BloomFilterPropertiesBuilder. With the builder in place, the publicfpp/ndvfields constrain future evolution. Locking them down behind accessors makes the public API smaller and more evolvable, matching the original direction outlined in #9667.What changes are included in this PR?
BloomFilterProperties::fppandBloomFilterProperties::ndvprivate fieldspub fn fpp(&self) -> f64andpub fn ndv(&self) -> u64accessor methodsAre these changes tested?
Yes — the existing test suite covers the affected paths
Are there any user-facing changes?
Yes — this is a breaking change to the public API.