Skip to content

Commit

Permalink
[BUG] Log purge off by one (chroma-core#2436)
Browse files Browse the repository at this point in the history
## Description of changes

*Summarize the changes made by this PR.*
 - Improvements & Bug fixes
- When revamping the prop tests in chroma-core#1969 I noticed that the purge code
was off by one - this remedies that.
 - New functionality
	 - None

## Test plan
*How are these changes tested?*
The tests TODO is updated with the correct behavior
- [x] Tests pass locally with `pytest` for python, `yarn test` for js,
`cargo test` for rust

## Documentation Changes
None
  • Loading branch information
HammadB authored Jul 3, 2024
1 parent 6087aba commit 28e6f5f
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 10 deletions.
2 changes: 1 addition & 1 deletion go/database/log/db/queries.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion go/database/log/queries/queries.sql
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ UPDATE collection set record_enumeration_offset_position = $2 where id = $1;
INSERT INTO collection (id, record_enumeration_offset_position, record_compaction_offset_position) values($1, $2, $3) returning *;

-- name: PurgeRecords :exec
DELETE FROM record_log r using collection c where r.collection_id = c.id and r.offset < c.record_compaction_offset_position;
DELETE FROM record_log r using collection c where r.collection_id = c.id and r.offset <= c.record_compaction_offset_position;
2 changes: 1 addition & 1 deletion go/database/log/schema/collection.sql
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ CREATE TABLE collection (
record_enumeration_offset_position bigint NOT NULL
);

-- The `record_compaction_offset_position` column indicates the offset position of the latest compaction.
-- The `record_compaction_offset_position` column indicates the offset position of the latest compaction, offsets are 1-indexed.
-- The `record_enenumeration_offset_position` column denotes the incremental offset for the most recent record in a collection.
12 changes: 5 additions & 7 deletions go/pkg/log/server/property_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,10 @@ func (suite *LogServerTestSuite) modelPullLogs(ctx context.Context, t *rapid.T,

// Pull logs from the model
modelLogs := suite.model.CollectionData[c]
// Find start offset in the model
// Find start offset in the model, which is the first offset that is greater than or equal to the start offset
startIndex := -1
for i, record := range modelLogs {
if record.offset == startOffset {
if record.offset >= startOffset {
startIndex = i
break
}
Expand All @@ -243,10 +243,8 @@ func (suite *LogServerTestSuite) modelPurgeLogs(ctx context.Context, t *rapid.T)

new_log := []ModelLogRecord{}
for _, record := range log {
// TODO: It is odd that the SUT purge behavior keeps the record
// with the compaction offset. Shouldn't we be able to purge this
// record?
if record.offset >= compactionOffset {
// Purge by adding everything after the compaction offset
if record.offset > compactionOffset {
new_log = append(new_log, record)
}
}
Expand Down Expand Up @@ -449,7 +447,7 @@ func (suite *LogServerTestSuite) TestRecordLogDb_PushLogs() {
records, err = suite.lr.PullRecords(ctx, id.String(), 0, 1, time.Now().UnixNano())
suite.NoError(err)
if len(records) > 0 {
suite.Equal(int64(offset), records[0].Offset)
suite.Equal(int64(offset)+1, records[0].Offset)
}
}
}
Expand Down

0 comments on commit 28e6f5f

Please sign in to comment.