db/commits: fix off-by-one in FindNearestToTs for "db vacate <ts>"#6842
Open
SAY-5 wants to merge 1 commit intobrimdata:mainfrom
Open
db/commits: fix off-by-one in FindNearestToTs for "db vacate <ts>"#6842SAY-5 wants to merge 1 commit intobrimdata:mainfrom
SAY-5 wants to merge 1 commit intobrimdata:mainfrom
Conversation
FindNearestToTs is supposed to return the oldest commit whose Date is still >= the vacate timestamp, so SetBase keeps that commit and drops everything strictly older. Today it walks from HEAD toward the root and returns on `ts >= commit.Date`, i.e. the newest commit older-than-or-equal-to ts. SetBase then keeps that too-old commit as the new base, leaving one extra commit in the chain. Concretely, in brimdata#6746 the user loads x:0 @ t=35s, x:1 @ 37s, x:2 @ 39s, x:3 @ 41s and runs `super db vacate -f 38s`. Expected: drop x:0 and x:1 (both strictly before 38s), keep x:2 and x:3. Actual: only x:0 is dropped because the walk stops at x:1 on the first iteration where 38s >= commit.Date. Flip the loop so we advance while Date >= ts and track the most recently visited ID in `prev`; return it when we hit a commit whose Date < ts. The `-f`-without-timestamp path (which passes HEAD.Date) still returns HEAD, matching current behaviour. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com> Fixes brimdata#6746
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
1 participant
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.
Summary
Fixes #6746.
FindNearestToTsis supposed to return the oldest commit whose Date is still>= ts, soSetBasekeeps that commit and drops everything strictly older. Today it walks from HEAD toward the root and returns onts >= commit.Date, i.e. the newest commit older-than-or-equal-tots.SetBasethen keeps that too-old commit as the new base, leaving one extra commit in the chain.Concretely, in #6746:
super db vacate -f 2026-03-18T20:48:38Z- expected to drop x:0 and x:1 (both strictly before 38s) and keep x:2/x:3. Actual: only x:0 is dropped, because the walk stops at x:1 on the first iteration where38s >= commit.Date.Fix
Flip the loop: advance while
commit.Date >= ts, record the most recently visited commit inprev, and returnprevwhen we hit a commit whoseDate < ts. The-f-without-timestamp path (which passesHEAD.Date) still returns HEAD on the first iteration (sinceHEAD.Date < HEAD.Dateis false, then parent's Date < HEAD.Date is true), matching the current behaviour.Test plan
go build ./...go build ./db/...db/ztests/vacate.yaml(the-f-without-timestamp scenario) is behaviourally unchanged under the new loop