Skip to content

docs: clarify BloomFilter contract after invert - #202

Merged
tisonkun merged 2 commits into
apache:mainfrom
Renkai:docs/bloom-invert-contract
Aug 16, 2026
Merged

docs: clarify BloomFilter contract after invert#202
tisonkun merged 2 commits into
apache:mainfrom
Renkai:docs/bloom-invert-contract

Conversation

@Renkai

@Renkai Renkai commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

This closes #194

Summary

Documentation-only clarification of the BloomFilter contract around invert(), addressing #194:

  • qualify the module-level guarantees as holding before invert() only (mod.rs#L31-L33)
  • same qualification at the type level (sketch.rs#L41-L42)
  • state in invert()'s documentation that after inversion neither the no-false-negative nor the false-positive guarantee holds, and that is_empty()/bits_used()/load_factor() then describe the raw bit state rather than insertion history (sketch.rs#L241-L245)
  • document is_empty() in terms of bit state (num_bits_set == 0), which is what the implementation actually observes (sketch.rs#L265-L268)
  • remove the unreachable BloomFilterBuilder::build panic documentation: the builder has no public default constructor, and both public entry points always produce a valid configuration (builder.rs#L157)

Design questions from #194

This PR makes the current semantics honest without committing to the open API decisions:

  1. invert placement: unchanged (stays on the public API, aligned with the C++/Java implementations, where it serves as an intermediate in boolean composition with union()/intersect()). Whether to move it to a raw/diagnostic API remains open in Clarify BloomFilter semantics after invert and correct builder documentation #194; the documentation added here is accurate under either outcome.
  2. is_empty semantics: documented as bit state, with the normal-operation meaning noted. Renaming would be a breaking change and is not proposed here.
  3. post-invert qualification of guarantees: done at module, type, and method level, which is the substance of this patch.
  4. build panic documentation: removed as directed. No further code change is needed for "encode the configured state more directly": the builder fields are already non-optional, and both constructors always produce a valid configuration.

Validation

  • cargo x check
  • cargo x test
  • cargo x lint

- qualify the module- and type-level no-false-negative and false-positive
  guarantees as applying before invert() only
- state in invert()'s documentation that after inversion both guarantees
  lapse and is_empty/bits_used/load_factor report raw bit state
- document is_empty() in terms of bit state rather than insertion history
- remove the unreachable BloomFilterBuilder::build panic documentation;
  both public constructors always produce a configured builder
@Renkai
Renkai marked this pull request as ready for review August 16, 2026 03:40
Signed-off-by: tison <wander4096@gmail.com>

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@tisonkun

Copy link
Copy Markdown
Member

cc @leerho I wonder if a BloomFilter gets invert-ed, is its state still valid? Not sure if we should call other methods of an inverted BloomFilter and what is the proper semantic there.

@tisonkun
tisonkun merged commit 21f1d9d into apache:main Aug 16, 2026
10 checks passed
@tisonkun

Copy link
Copy Markdown
Member

Thanks for your contribution @Renkai!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify BloomFilter semantics after invert and correct builder documentation

2 participants