internal/manifest: fix Overlaps L0 expansion with exclusive end#1463
internal/manifest: fix Overlaps L0 expansion with exclusive end#1463jbowens merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
nicktrav
left a comment
There was a problem hiding this comment.
Nice! Thanks for tracking this down, and adding the regression test case too.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @sumeerbhola)
sumeerbhola
left a comment
There was a problem hiding this comment.
Glad to see the new datadriven test.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jbowens)
internal/manifest/version.go, line 463 at r1 (raw file):
// 000971:[acutc@6#4227,SET-zzhra@12#72057594037927935,RANGEDEL] delim := map[rune]bool{':': true, '[': true, '-': true, ']': true} fields := strings.FieldsFunc(l, func(c rune) bool { return delim[c] })
didn't know about FieldsFunc -- super convenient for tests.
internal/manifest/version.go, line 476 at r1 (raw file):
} // Reverse the order of L0 files. This ensures we construct the same // sublevels.
consider adding that they are printed from higher sublevel to lower, which means in a partial order that represents newest to oldest.
btw, why does this matter given that the input btree that is used for constructing sublevels is sorted by seqnum?
internal/manifest/version.go, line 611 at r1 (raw file):
restart = true } if v := cmp(largest, end); v > 0 {
amusing that 4 engineers looking at this code didn't catch this bug during review.
the metamorphic test saves us from ourselves again!
internal/manifest/testdata/overlaps, line 95 at r1 (raw file):
000700:[b#7008,SET-e#7009,SET] 000701:[c#7018,SET-f#7019,SET] 000702:[f#7028,SET-g#7029,SET]
000702 would not have been picked with the bug?
Consider adding a short comment like the following.
// Case that relies on exclusive-end changing to false after picking some file.
When Version.Overlaps is called for L0, the overlap window is iteratively expanded until stable. In cockroachdb#1432, the Overlaps function was adjusted to allow specifying that the end bound should be considered exclusive. However, cockroachdb#1432 failed to update the exclusivity of the end bound when the range widened. This improperly excluded files with largest keys that exactly equaled the new widened end bound. This commit also transforms the TestOverlaps test into a datadriven test, introducing a few helpers for parsing the DebugString output of a Version. Fix cockroachdb#1459.
jbowens
left a comment
There was a problem hiding this comment.
TFTRs!
Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @nicktrav and @sumeerbhola)
internal/manifest/version.go, line 476 at r1 (raw file):
Done.
btw, why does this matter given that the input btree that is used for constructing sublevels is sorted by seqnum?
in order to accommodate tests that require constructing inconsistent versions, NewVersion intentionally constructs the b-tree using the order the files are provided (and from then on uses the correct comparator). Added a TODO there, because we should definitely remove that.
internal/manifest/version.go, line 611 at r1 (raw file):
Previously, sumeerbhola wrote…
amusing that 4 engineers looking at this code didn't catch this bug during review.
the metamorphic test saves us from ourselves again!
I truly love the metamorphic tests. ❤️
internal/manifest/testdata/overlaps, line 95 at r1 (raw file):
Previously, sumeerbhola wrote…
000702 would not have been picked with the bug?
Consider adding a short comment like the following.
// Case that relies on exclusive-end changing to false after picking some file.
Done.
When Version.Overlaps is called for L0, the overlap window is
iteratively expanded until stable. In #1432, the Overlaps function was
adjusted to allow specifying that the end bound should be considered
exclusive. However, #1432 failed to update the exclusivity of the end
bound when the range widened. This improperly excluded files with
largest keys that exactly equaled the new widened end bound.
This commit also transforms the TestOverlaps test into a datadriven
test, introducing a few helpers for parsing the DebugString output of a
Version.
Fix #1459.