interval/generic: improve randomized testing, fix upper bound bug #51939
Conversation
|
This change is |
There was a problem hiding this comment.
LGTM, I thought this was a bigger diff than it was. Now that we're sharing the same code for the benchmarks+tests, would it make sense to skip the verification code when running the benchmarks?
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/util/interval/generic/internal/interval_btree_tmpl.go, line 627 at r1 (raw file):
// adjustUpperBoundOnRemoval adjusts the upper key bound for this node // given a item and an optional child node that was removed. Returns
s/given a/given an
In an effort to track down the bug that triggered cockroachdb#51913, this commit ports the randomized interval btree benchmarks to also be unit tests. This allows us to run invariant checks (see `btree.Verify`) on randomized tree configurations. Doing so revealed a violation of the `isUpperBoundCorrect` invariant. This was determined to be a bug in `node.removeMax`. When removing an item from a grandchild node, we were failing to adjust the upper bound of the child node. It doesn't look like this could cause user-visible effects because the upper bound of a subtree is only ever decreased on removal, so at worst, this caused searches in the tree to do more work than strictly necessary. Still, this is a good bug to fix and it's encouraging that the new randomized testing using the existing invariant validation caught it.
This was fallout from a previous `s/latch/item/g`.
nvanbenschoten
force-pushed
the
nvanbenschoten/btreeRandomTest
branch
from
6aac001
to
33f86ff
Compare
There was a problem hiding this comment.
TFTR!
Now that we're sharing the same code for the benchmarks+tests, would it make sense to skip the verification code when running the benchmarks?
Yep, we are, see the type switch in verify.
bors r+
Reviewable status:
pkg/util/interval/generic/internal/interval_btree_tmpl.go, line 627 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
s/given a/given an
Good catch. I think I s/latch/item/g at one point, which is why we have this issue in a number of places. Fixed.
|
Build succeeded: |
In an effort to track down the bug that triggered #51913, this commit
ports the randomized interval btree benchmarks to also be unit tests.
This allows us to run invariant checks (see
btree.Verify) on randomizedtree configurations.
Doing so revealed a violation of the
isUpperBoundCorrectinvariant.This was determined to be a bug in
node.removeMax. When removing anitem from a grandchild node, we were failing to adjust the upper bound
of the child node. It doesn't look like this could cause user-visible
effects because the upper bound of a subtree is only ever decreased on
removal, so at worst, this caused searches in the tree to do more work
than strictly necessary. Still, this is a good bug to fix and it's
encouraging that the new randomized testing using the existing invariant
validation caught it.