Skip to content

Commit

Permalink
*: return *InternalKV from internal iterators
Browse files Browse the repository at this point in the history
Previously, internal iterators' positioning methods returned two return values,
a pointer to an internal key and a LazyValue struct. This commit collapses
these two return values into a single return value: a pointer to an InternalKV
struct. The key and value buffers were already owned by the underlying
iterator, so passing the base.LazyValue as a value did not decouple the two.
Propagating a single *InternalKV pointer can be cheaper in some instances.

More importantly, it allows us to propagate additional information without
increasing the amount of copying up and down the iterator stack. I anticipate
this being useful in the implementation of cockroachdb/cockroach#111001,
allowing us to propagate checksums alongside keys and values. Future work may
add an explicit error return value.
  • Loading branch information
jbowens committed Apr 10, 2024
1 parent 6cdb88d commit af7e51a
Show file tree
Hide file tree
Showing 60 changed files with 2,047 additions and 2,161 deletions.
206 changes: 113 additions & 93 deletions batch.go

Large diffs are not rendered by default.

22 changes: 11 additions & 11 deletions batch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1069,9 +1069,9 @@ func TestBatchRangeOps(t *testing.T) {
return err.Error()
}
} else {
for k, v := internalIter.First(); k != nil; k, v = internalIter.Next() {
k.SetSeqNum(k.SeqNum() &^ InternalKeySeqNumBatch)
fmt.Fprintf(&buf, "%s:%s\n", k, v.InPlaceValue())
for kv := internalIter.First(); kv != nil; kv = internalIter.Next() {
kv.K.SetSeqNum(kv.K.SeqNum() &^ InternalKeySeqNumBatch)
fmt.Fprintf(&buf, "%s:%s\n", kv.K, kv.InPlaceValue())
}
}
return buf.String()
Expand Down Expand Up @@ -1193,9 +1193,9 @@ func TestFlushableBatch(t *testing.T) {

var buf bytes.Buffer

iter := newInternalIterAdapter(b.newIter(nil))
for valid := iter.First(); valid; valid = iter.Next() {
fmt.Fprintf(&buf, "%s:%s\n", iter.Key(), iter.Value())
iter := b.newIter(nil)
for kv := iter.First(); kv != nil; kv = iter.Next() {
fmt.Fprintf(&buf, "%s:%s\n", kv.K, kv.InPlaceValue())
}
iter.Close()

Expand Down Expand Up @@ -1261,8 +1261,8 @@ func TestFlushableBatchDeleteRange(t *testing.T) {
}

func scanInternalIter(w io.Writer, ii internalIterator) {
for k, v := ii.First(); k != nil; k, v = ii.Next() {
fmt.Fprintf(w, "%s:%s\n", k, v.InPlaceValue())
for kv := ii.First(); kv != nil; kv = ii.Next() {
fmt.Fprintf(w, "%s:%s\n", kv.K, kv.InPlaceValue())
}
}

Expand Down Expand Up @@ -1290,7 +1290,7 @@ func TestFlushableBatchBytesIterated(t *testing.T) {
it := fb.newFlushIter(nil, &bytesIterated)

var prevIterated uint64
for key, _ := it.First(); key != nil; key, _ = it.Next() {
for kv := it.First(); kv != nil; kv = it.Next() {
if bytesIterated < prevIterated {
t.Fatalf("bytesIterated moved backward: %d < %d", bytesIterated, prevIterated)
}
Expand All @@ -1308,8 +1308,8 @@ func TestEmptyFlushableBatch(t *testing.T) {
// Verify that we can create a flushable batch on an empty batch.
fb, err := newFlushableBatch(newBatch(nil), DefaultComparer)
require.NoError(t, err)
it := newInternalIterAdapter(fb.newIter(nil))
require.False(t, it.First())
it := fb.newIter(nil)
require.Nil(t, it.First())
}

func TestBatchCommitStats(t *testing.T) {
Expand Down
12 changes: 6 additions & 6 deletions compaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -607,18 +607,18 @@ func newFlush(

smallestSet, largestSet := false, false
updatePointBounds := func(iter internalIterator) {
if key, _ := iter.First(); key != nil {
if kv := iter.First(); kv != nil {
if !smallestSet ||
base.InternalCompare(c.cmp, c.smallest, *key) > 0 {
base.InternalCompare(c.cmp, c.smallest, kv.K) > 0 {
smallestSet = true
c.smallest = key.Clone()
c.smallest = kv.K.Clone()
}
}
if key, _ := iter.Last(); key != nil {
if kv := iter.Last(); kv != nil {
if !largestSet ||
base.InternalCompare(c.cmp, c.largest, *key) < 0 {
base.InternalCompare(c.cmp, c.largest, kv.K) < 0 {
largestSet = true
c.largest = key.Clone()
c.largest = kv.K.Clone()
}
}
}
Expand Down
Loading

0 comments on commit af7e51a

Please sign in to comment.