Deterministic RNG test harness and address bug in BitVec
#72
Conversation
Fix issue where BitVec was required to be structurally identical rather than just semantically equivalent. This caused issues when bits in the LSB were toggled. This adds a test harness that provides seeded random number generators for tests, making failures reproducible via TEST_SEED env var.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses random test failures in the BitVec implementation by introducing a deterministic RNG test harness and fixing a semantic equality bug. The PR implements two main solutions: a test harness that captures and allows reproduction of random test failures using environment variables, and a fix to BitVec's equality comparison to check semantic equivalence rather than structural identity.
- Implements a deterministic RNG test harness for reproducible random test failures
- Fixes
BitVecequality to compare semantic equivalence instead of structural identity - Migrates all existing tests to use the new test harness
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/geo_filters/src/test_rng.rs |
New test harness module providing seeded RNG with failure reproduction capability |
crates/geo_filters/src/lib.rs |
Adds test_rng module import for test configuration |
crates/geo_filters/src/diff_count/bitvec.rs |
Implements custom PartialEq for BitVec using semantic comparison via bit_chunks |
crates/geo_filters/src/distinct_count.rs |
Migrates tests to use prng_test_harness |
crates/geo_filters/src/diff_count.rs |
Migrates tests to use prng_test_harness and removes old FIXME comment |
crates/geo_filters/src/config/lookup.rs |
Migrates tests to use prng_test_harness |
crates/geo_filters/src/config.rs |
Migrates test_estimate function to use prng_test_harness |
| fn eq(&self, other: &Self) -> bool { | ||
| self.bit_chunks().eq(other.bit_chunks()) | ||
| } |
There was a problem hiding this comment.
creative solution :)
The main issue I have is that we cannot rely on the BitVec being sufficiently sized and instead have to ensure its size all the time. That's rather ugly IMO
- Pass in RNG to helper functions - Add iterations to test harness
|
Following a discussion on slack I've |
| ) -> Self { | ||
| // if there are no chunks, we keep the size zero | ||
| let num_bits = chunks.peek().map(|_| num_bits).unwrap_or_default(); | ||
| pub fn from_bit_chunks<I: Iterator<Item = BitChunk>>(chunks: I, num_bits: usize) -> Self { |
There was a problem hiding this comment.
did you check whether any callsites are unnecessarily calling peekable?
There was a problem hiding this comment.
There weren't any for the BitVec::from_bit_chunks but I did find some for other functions 😅
There was a problem hiding this comment.
This is because BitChunksOnes iterator already has peekable iterators internally. They're required for next, but not required externally, so the Peekable wrapper here in the from_bit_chunks function is not needed.
Why?
There have been random test failures caused by some invariants not being maintained in the LSB
BitVec. This presents two problems.How?
To address this I've done two things.
Firstly, I implement a test harness for running tests which require a random number generator (RNG). This harness accepts a lambda function which has a single argument, a RNG. Under normal conditions the RNG is seeded randomly as a typical thread local RNG. The difference occurs when you get a test failure, at which point the harness captures the panic and prints out the RNG seed. You can then rerun your tests with
TEST_SEED=...set as an environment variable in order to start the test with the seed that failed. This allows developers to rerun the test and see the failure immediately, a generally useful thing to be able to do :)E.g. you run some tests with
cargo testand get the following error message from a flakey test:You can then re-run with
TEST_SEED=16501082297235867130 cargo testand the test will fail reliably.I rewrote all our
teststo use this since it's good to be able to reproduce issues. I did not roll it out to theevaluationcode.Secondly, I fix issue where
BitVecwas required to be structurally identical rather than just semantically equivalent to pass equality checks. Specifically, an emptyBitVecwith no blocks is semantically the same as aBitVecwith>=1block where all the bits are zeros, but thePartialEqfunction didn't consider them the same.This caused issues when bits in the LSB were toggled directly. An item is pushed, hashed and assigned a bucket. If the bucket is in the MSB range then the LSB is updated and potentially resized. If the bucket is in the LSB range the bit is toggled but the bucket is not resized. This is because we expect most items to toggle bits in the LSB range so not resizing the buffer every time this happen saves us on some performance.
This issue would only happen relatively rarely, given that it needed the last item inserted to toggle the last bit in the LSB before the equality check is performed. Usually there is more than 1 bit in the LSB set before the final item is inserted.
I addressed this issue by changing how
PartialEq::eqis implemented to check that the set bits are equivalent using the existingbit_chunksiterator.There may be some situations where it makes sense to manually trigger a resize (
shrink_to_fit?). For example, in the serialization code (PR not yet merged) it would make sense to resize the LSB before serialization to prevent writing empty bytes to disk.Alternative approaches
For the deterministic RNG I could have used something like
proptest, but it felt like too big/powerful a dependency for this limited use case.For the
BitVecstuff....Rather than make the
Eqfunction check the semantic equivalence we could spend more effort maintaining the structural equivalence of theBitVec.The first thing I tried was to make toggling the LSB check the size of the vectors and resize if needed - this fixed the issue, but was bad for performance.
I tried changing how the LSB is updated when the MSB has an insert, but this caused quite a few knock on effects due to the fact that the most-significant-bit in the LSB range is truncated when it's resized. Removing this resize code had a load of knock-on issues that, to me, seemed harder to solve.
If anyone feels strongly that the
BitVecshould do it's comparisons based on the specific state and that the semantic equivalence is not good enough then we can rework this PR.