manifest: simplify BulkVersionEdit.Apply

RaduBerinde · RaduBerinde · commit 27dd231cb2e1 · 2025-03-31T08:25:38.000-07:00
Require a non-nil version to simplify the interface and the code a
bit.

We also remove unnecessary nil checks for `Version.L0Sublevels`, which
is never the case.
diff --git a/compaction.go b/compaction.go
@@ -571,9 +571,7 @@ func newFlush(
 		}
 	}
 
-	if cur.L0Sublevels != nil {
-		c.l0Limits = cur.L0Sublevels.FlushSplitKeys()
-	}
+	c.l0Limits = cur.L0Sublevels.FlushSplitKeys()
 
 	smallestSet, largestSet := false, false
 	updatePointBounds := func(iter internalIterator) {
@@ -1038,7 +1036,7 @@ func (d *DB) addInProgressCompaction(c *compaction) {
 		}
 	}
 
-	if (isIntraL0 || isBase) && c.version.L0Sublevels != nil {
+	if isIntraL0 || isBase {
 		l0Inputs := []manifest.LevelSlice{c.startLevel.files}
 		if isIntraL0 {
 			l0Inputs = append(l0Inputs, c.outputLevel.files)
diff --git a/compaction_test.go b/compaction_test.go
@@ -41,7 +41,7 @@ import (
 )
 
 func newVersion(opts *Options, files [numLevels][]*tableMetadata) *version {
-	v := manifest.NewVersion(
+	v := manifest.NewVersionForTesting(
 		opts.Comparer,
 		opts.FlushSplitBytes,
 		files)
diff --git a/get_iter_test.go b/get_iter_test.go
@@ -429,7 +429,7 @@ func TestGetIter(t *testing.T) {
 
 			files[tt.level] = append(files[tt.level], meta)
 		}
-		v := manifest.NewVersion(cmp, 10<<20, files)
+		v := manifest.NewVersionForTesting(cmp, 10<<20, files)
 		err := v.CheckOrdering()
 		if tc.badOrdering && err == nil {
 			t.Errorf("desc=%q: want bad ordering, got nil error", desc)
diff --git a/internal/compact/splitting_test.go b/internal/compact/splitting_test.go
@@ -34,7 +34,7 @@ func TestOutputSplitter(t *testing.T) {
 					files[1] = append(files[1], f)
 				}
 			}
-			v := manifest.NewVersion(base.DefaultComparer, 64*1024, files)
+			v := manifest.NewVersionForTesting(base.DefaultComparer, 64*1024, files)
 			if err := v.CheckOrdering(); err != nil {
 				d.Fatalf(t, "%v", err)
 			}
diff --git a/internal/keyspan/keyspanimpl/level_iter_test.go b/internal/keyspan/keyspanimpl/level_iter_test.go
@@ -312,7 +312,7 @@ func TestLevelIterEquivalence(t *testing.T) {
 				amap[metas[i].FileNum] = metas[i]
 			}
 			b.AddedTables[6] = amap
-			v, err := b.Apply(nil, base.DefaultComparer, 0, 0)
+			v, err := b.Apply(manifest.NewVersion(base.DefaultComparer), 0, 0)
 			require.NoError(t, err)
 			levelIter.Init(
 				context.Background(),
diff --git a/internal/manifest/annotator_test.go b/internal/manifest/annotator_test.go
@@ -26,7 +26,7 @@ func makeTestVersion(numFiles int) (*Version, []*TableMetadata) {
 	var levelFiles [7][]*TableMetadata
 	levelFiles[6] = files
 
-	v := NewVersion(base.DefaultComparer, 0, levelFiles)
+	v := NewVersionForTesting(base.DefaultComparer, 0, levelFiles)
 	return v, files
 }
 
diff --git a/internal/manifest/l0_sublevels_test.go b/internal/manifest/l0_sublevels_test.go
@@ -32,7 +32,7 @@ func readManifest(filename string) (*Version, error) {
 	}
 	defer f.Close()
 	rr := record.NewReader(f, 0 /* logNum */)
-	var v *Version
+	v := NewVersion(base.DefaultComparer)
 	addedByFileNum := make(map[base.FileNum]*TableMetadata)
 	for {
 		r, err := rr.Next()
@@ -51,7 +51,7 @@ func readManifest(filename string) (*Version, error) {
 		if err := bve.Accumulate(&ve); err != nil {
 			return nil, err
 		}
-		if v, err = bve.Apply(v, base.DefaultComparer, 10<<20, 32000); err != nil {
+		if v, err = bve.Apply(v, 10<<20, 32000); err != nil {
 			return nil, err
 		}
 	}
diff --git a/internal/manifest/manifest_test.go b/internal/manifest/manifest_test.go
@@ -127,7 +127,7 @@ func replayManifest(t *testing.T, opts *pebble.Options, dirname string) *manifes
 		require.NoError(t, bve.Accumulate(&ve))
 	}
 	v, err := bve.Apply(
-		nil /* version */, cmp, opts.FlushSplitBytes,
+		manifest.NewVersion(cmp), opts.FlushSplitBytes,
 		opts.Experimental.ReadCompactionRate)
 	require.NoError(t, err)
 	return v
diff --git a/internal/manifest/version.go b/internal/manifest/version.go
@@ -1207,9 +1207,23 @@ func SortBySmallest(files []*TableMetadata, cmp Compare) {
 // NumLevels is the number of levels a Version contains.
 const NumLevels = 7
 
-// NewVersion constructs a new Version with the provided files. It requires
-// the provided files are already well-ordered. It's intended for testing.
-func NewVersion(
+// NewVersion creates a version with no files.
+func NewVersion(comparer *base.Comparer) *Version {
+	v := &Version{
+		cmp: comparer,
+	}
+	for level := range v.Levels {
+		v.Levels[level] = MakeLevelMetadata(comparer.Compare, level, nil /* files */)
+		v.RangeKeyLevels[level] = MakeLevelMetadata(comparer.Compare, level, nil /* files */)
+	}
+	v.L0Sublevels = &L0Sublevels{cmp: comparer.Compare, formatKey: comparer.FormatKey, levelMetadata: &v.Levels[0]}
+	return v
+}
+
+// NewVersionForTesting constructs a new Version with the provided files. It
+// requires the provided files are already well-ordered. It's intended for
+// testing.
+func NewVersionForTesting(
 	comparer *base.Comparer, flushSplitBytes int64, files [NumLevels][]*TableMetadata,
 ) *Version {
 	v := &Version{
@@ -1389,7 +1403,7 @@ func ParseVersionDebug(comparer *base.Comparer, flushSplitBytes int64, s string)
 	// partial order that represents newest to oldest. Reverse the order of L0
 	// files to ensure we construct the same sublevels.
 	slices.Reverse(files[0])
-	v := NewVersion(comparer, flushSplitBytes, files)
+	v := NewVersionForTesting(comparer, flushSplitBytes, files)
 	if err := v.CheckOrdering(); err != nil {
 		return nil, err
 	}
@@ -1476,7 +1490,7 @@ func (v *Version) Next() *Version {
 func (v *Version) InitL0Sublevels(flushSplitBytes int64) error {
 	var err error
 	v.L0Sublevels, err = NewL0Sublevels(&v.Levels[0], v.cmp.Compare, v.cmp.FormatKey, flushSplitBytes)
-	if err == nil && v.L0Sublevels != nil {
+	if err == nil {
 		v.L0SublevelFiles = v.L0Sublevels.Levels
 	}
 	return err
diff --git a/internal/manifest/version_edit.go b/internal/manifest/version_edit.go
@@ -1149,45 +1149,29 @@ func (b *BulkVersionEdit) Accumulate(ve *VersionEdit) error {
 //
 // curr may be nil, which is equivalent to a pointer to a zero version.
 func (b *BulkVersionEdit) Apply(
-	curr *Version, comparer *base.Comparer, flushSplitBytes int64, readCompactionRate int64,
+	curr *Version, flushSplitBytes int64, readCompactionRate int64,
 ) (*Version, error) {
+	comparer := curr.cmp
 	v := &Version{
 		cmp: comparer,
 	}
 
 	// Adjust the count of files marked for compaction.
-	if curr != nil {
-		v.Stats.MarkedForCompaction = curr.Stats.MarkedForCompaction
-	}
+	v.Stats.MarkedForCompaction = curr.Stats.MarkedForCompaction
 	v.Stats.MarkedForCompaction += b.MarkedForCompactionCountDiff
 	if v.Stats.MarkedForCompaction < 0 {
 		return nil, base.CorruptionErrorf("pebble: version marked for compaction count negative")
 	}
 
 	for level := range v.Levels {
-		if curr == nil || curr.Levels[level].tree.root == nil {
-			v.Levels[level] = MakeLevelMetadata(comparer.Compare, level, nil /* files */)
-		} else {
-			v.Levels[level] = curr.Levels[level].clone()
-		}
-		if curr == nil || curr.RangeKeyLevels[level].tree.root == nil {
-			v.RangeKeyLevels[level] = MakeLevelMetadata(comparer.Compare, level, nil /* files */)
-		} else {
-			v.RangeKeyLevels[level] = curr.RangeKeyLevels[level].clone()
-		}
+		v.Levels[level] = curr.Levels[level].clone()
+		v.RangeKeyLevels[level] = curr.RangeKeyLevels[level].clone()
 
 		if len(b.AddedTables[level]) == 0 && len(b.DeletedTables[level]) == 0 {
 			// There are no edits on this level.
 			if level == 0 {
-				// Initialize L0Sublevels.
-				if curr == nil || curr.L0Sublevels == nil {
-					if err := v.InitL0Sublevels(flushSplitBytes); err != nil {
-						return nil, errors.Wrap(err, "pebble: internal error")
-					}
-				} else {
-					v.L0Sublevels = curr.L0Sublevels
-					v.L0SublevelFiles = v.L0Sublevels.Levels
-				}
+				v.L0Sublevels = curr.L0Sublevels
+				v.L0SublevelFiles = curr.L0SublevelFiles
 			}
 			continue
 		}
@@ -1266,6 +1250,7 @@ func (b *BulkVersionEdit) Apply(
 			// Track the keys with the smallest and largest keys, so that we can
 			// check consistency of the modified span.
 			if sm == nil || base.InternalCompare(comparer.Compare, sm.Smallest, f.Smallest) > 0 {
+
 				sm = f
 			}
 			if la == nil || base.InternalCompare(comparer.Compare, la.Largest, f.Largest) < 0 {
@@ -1285,7 +1270,7 @@ func (b *BulkVersionEdit) Apply(
 		}
 
 		if level == 0 {
-			if curr != nil && curr.L0Sublevels != nil && len(deletedTablesMap) == 0 {
+			if len(deletedTablesMap) == 0 {
 				// Flushes and ingestions that do not delete any L0 files do not require
 				// a regeneration of L0Sublevels from scratch. We can instead generate
 				// it incrementally.
diff --git a/internal/manifest/version_edit_test.go b/internal/manifest/version_edit_test.go
@@ -443,7 +443,7 @@ func TestVersionEditApply(t *testing.T) {
 					}
 				}
 
-				newv, err := bve.Apply(v, base.DefaultComparer, flushSplitBytes, readCompactionRate)
+				newv, err := bve.Apply(v, flushSplitBytes, readCompactionRate)
 				if err != nil {
 					return err.Error()
 				}
diff --git a/internal/manifest/version_test.go b/internal/manifest/version_test.go
@@ -438,7 +438,7 @@ func TestTableMetadata_ParseRoundTrip(t *testing.T) {
 func TestCalculateInuseKeyRanges(t *testing.T) {
 	newVersion := func(files [NumLevels][]*TableMetadata) *Version {
 		t.Helper()
-		v := NewVersion(base.DefaultComparer, 64*1024, files)
+		v := NewVersionForTesting(base.DefaultComparer, 64*1024, files)
 		if err := v.CheckOrdering(); err != nil {
 			t.Fatal(err)
 		}
@@ -740,7 +740,7 @@ func TestCalculateInuseKeyRangesRandomized(t *testing.T) {
 				return cmp(a.Smallest.UserKey, b.Smallest.UserKey)
 			})
 		}
-		v := NewVersion(base.DefaultComparer, 64*1024, files)
+		v := NewVersionForTesting(base.DefaultComparer, 64*1024, files)
 		if err := v.CheckOrdering(); err != nil {
 			t.Fatal(err)
 		}
diff --git a/level_checker_test.go b/level_checker_test.go
@@ -279,7 +279,7 @@ func TestCheckLevelsCornerCases(t *testing.T) {
 				// Start from level 1 in this test.
 				files[i+1] = levels[i]
 			}
-			version := manifest.NewVersion(
+			version := manifest.NewVersionForTesting(
 				testkeys.Comparer,
 				0,
 				files)
diff --git a/open_test.go b/open_test.go
@@ -1457,7 +1457,7 @@ func TestCheckConsistency(t *testing.T) {
 					}
 				}
 
-				v := manifest.NewVersion(base.DefaultComparer, 0, filesByLevel)
+				v := manifest.NewVersionForTesting(base.DefaultComparer, 0, filesByLevel)
 				err := checkConsistency(v, provider)
 				if err != nil {
 					if redactErr {
diff --git a/replay/replay.go b/replay/replay.go
@@ -714,7 +714,7 @@ func (r *Runner) prepareWorkloadSteps(ctx context.Context) error {
 
 	var cumulativeWriteBytes uint64
 	var flushBufs flushBuffers
-	var v *manifest.Version
+	v := manifest.NewVersion(r.Opts.Comparer)
 	var previousVersion *manifest.Version
 	var bve manifest.BulkVersionEdit
 	bve.AddedTablesByFileNum = make(map[base.FileNum]*manifest.TableMetadata)
@@ -724,7 +724,6 @@ func (r *Runner) prepareWorkloadSteps(ctx context.Context) error {
 	currentVersion := func() (*manifest.Version, error) {
 		var err error
 		v, err = bve.Apply(v,
-			r.Opts.Comparer,
 			r.Opts.FlushSplitBytes,
 			r.Opts.Experimental.ReadCompactionRate)
 		bve = manifest.BulkVersionEdit{AddedTablesByFileNum: bve.AddedTablesByFileNum}
diff --git a/tool/db.go b/tool/db.go
@@ -716,7 +716,7 @@ func (d *dbT) runProperties(cmd *cobra.Command, args []string) {
 			}
 		}
 		v, err := bve.Apply(
-			nil /* version */, cmp, d.opts.FlushSplitBytes,
+			manifest.NewVersion(cmp), d.opts.FlushSplitBytes,
 			d.opts.Experimental.ReadCompactionRate,
 		)
 		if err != nil {
diff --git a/tool/lsm.go b/tool/lsm.go
@@ -322,7 +322,7 @@ func (l *lsmT) buildEdits(edits []*manifest.VersionEdit) error {
 			}
 		}
 
-		v := manifest.NewVersion(l.cmp, 0, currentFiles)
+		v := manifest.NewVersionForTesting(l.cmp, 0, currentFiles)
 		edit.Sublevels = make(map[base.FileNum]int)
 		for sublevel, files := range v.L0SublevelFiles {
 			iter := files.Iter()
diff --git a/tool/manifest.go b/tool/manifest.go
@@ -243,7 +243,8 @@ func (m *manifestT) runDump(cmd *cobra.Command, args []string) {
 
 			if comparer != nil {
 				v, err := bve.Apply(
-					nil /* version */, comparer, 0,
+					manifest.NewVersion(comparer),
+					0,
 					m.opts.Experimental.ReadCompactionRate,
 				)
 				if err != nil {
@@ -625,7 +626,10 @@ func (m *manifestT) runCheck(cmd *cobra.Command, args []string) {
 				}
 				// TODO(sbhola): add option to Apply that reports all errors instead of
 				// one error.
-				newv, err := bve.Apply(v, cmp, 0, m.opts.Experimental.ReadCompactionRate)
+				if v == nil {
+					v = manifest.NewVersion(cmp)
+				}
+				newv, err := bve.Apply(v, 0, m.opts.Experimental.ReadCompactionRate)
 				if err != nil {
 					fmt.Fprintf(stdout, "%s: offset: %d err: %s\n",
 						arg, offset, err)
diff --git a/version_set.go b/version_set.go
@@ -179,7 +179,8 @@ func (vs *versionSet) create(
 ) error {
 	vs.init(dirname, provider, opts, marker, getFormatMajorVersion, mu)
 	var bve bulkVersionEdit
-	newVersion, err := bve.Apply(nil /* curr */, opts.Comparer, opts.FlushSplitBytes, opts.Experimental.ReadCompactionRate)
+	emptyVersion := manifest.NewVersion(opts.Comparer)
+	newVersion, err := bve.Apply(emptyVersion, opts.FlushSplitBytes, opts.Experimental.ReadCompactionRate)
 	if err != nil {
 		return err
 	}
@@ -239,13 +240,13 @@ func (vs *versionSet) load(
 	// Read the versionEdits in the manifest file.
 	var bve bulkVersionEdit
 	bve.AddedTablesByFileNum = make(map[base.FileNum]*tableMetadata)
-	manifest, err := vs.fs.Open(manifestPath)
+	manifestFile, err := vs.fs.Open(manifestPath)
 	if err != nil {
 		return errors.Wrapf(err, "pebble: could not open manifest file %q for DB %q",
 			errors.Safe(manifestFilename), dirname)
 	}
-	defer manifest.Close()
-	rr := record.NewReader(manifest, 0 /* logNum */)
+	defer manifestFile.Close()
+	rr := record.NewReader(manifestFile, 0 /* logNum */)
 	for {
 		r, err := rr.Next()
 		if err == io.EOF || record.IsInvalidRecord(err) {
@@ -343,7 +344,8 @@ func (vs *versionSet) load(
 		}
 	}
 
-	newVersion, err := bve.Apply(nil, opts.Comparer, opts.FlushSplitBytes, opts.Experimental.ReadCompactionRate)
+	emptyVersion := manifest.NewVersion(opts.Comparer)
+	newVersion, err := bve.Apply(emptyVersion, opts.FlushSplitBytes, opts.Experimental.ReadCompactionRate)
 	if err != nil {
 		return err
 	}
@@ -576,7 +578,7 @@ func (vs *versionSet) logAndApply(
 			return errors.Wrap(err, "MANIFEST accumulate failed")
 		}
 		newVersion, err = b.Apply(
-			currentVersion, vs.cmp, vs.opts.FlushSplitBytes, vs.opts.Experimental.ReadCompactionRate,
+			currentVersion, vs.opts.FlushSplitBytes, vs.opts.Experimental.ReadCompactionRate,
 		)
 		if err != nil {
 			return errors.Wrap(err, "MANIFEST apply failed")

Original file line number	Diff line number	Diff line change
`@@ -571,9 +571,7 @@ func newFlush(`
`571`	`571`	`}`
`572`	`572`	`}`
`573`	`573`
`574`		`- if cur.L0Sublevels != nil {`
`575`		`- c.l0Limits = cur.L0Sublevels.FlushSplitKeys()`
`576`		`- }`
	`574`	`+ c.l0Limits = cur.L0Sublevels.FlushSplitKeys()`
`577`	`575`
`578`	`576`	`smallestSet, largestSet := false, false`
`579`	`577`	`updatePointBounds := func(iter internalIterator) {`
`@@ -1038,7 +1036,7 @@ func (d DB) addInProgressCompaction(c compaction) {`
`1038`	`1036`	`}`
`1039`	`1037`	`}`
`1040`	`1038`
`1041`		`- if (isIntraL0 \|\| isBase) && c.version.L0Sublevels != nil {`
	`1039`	`+ if isIntraL0 \|\| isBase {`
`1042`	`1040`	`l0Inputs := []manifest.LevelSlice{c.startLevel.files}`
`1043`	`1041`	`if isIntraL0 {`
`1044`	`1042`	`l0Inputs = append(l0Inputs, c.outputLevel.files)`
Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,7 @@ import (`
`41`	`41`	`)`
`42`	`42`
`43`	`43`	`func newVersion(opts Options, files [numLevels][]tableMetadata) *version {`
`44`		`- v := manifest.NewVersion(`
	`44`	`+ v := manifest.NewVersionForTesting(`
`45`	`45`	`opts.Comparer,`
`46`	`46`	`opts.FlushSplitBytes,`
`47`	`47`	`files)`
Original file line number	Diff line number	Diff line change
`@@ -429,7 +429,7 @@ func TestGetIter(t *testing.T) {`
`429`	`429`
`430`	`430`	`files[tt.level] = append(files[tt.level], meta)`
`431`	`431`	`}`
`432`		`- v := manifest.NewVersion(cmp, 10<<20, files)`
	`432`	`+ v := manifest.NewVersionForTesting(cmp, 10<<20, files)`
`433`	`433`	`err := v.CheckOrdering()`
`434`	`434`	`if tc.badOrdering && err == nil {`
`435`	`435`	`t.Errorf("desc=%q: want bad ordering, got nil error", desc)`
Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@ func TestOutputSplitter(t *testing.T) {`
`34`	`34`	`files[1] = append(files[1], f)`
`35`	`35`	`}`
`36`	`36`	`}`
`37`		`- v := manifest.NewVersion(base.DefaultComparer, 64*1024, files)`
	`37`	`+ v := manifest.NewVersionForTesting(base.DefaultComparer, 64*1024, files)`
`38`	`38`	`if err := v.CheckOrdering(); err != nil {`
`39`	`39`	`d.Fatalf(t, "%v", err)`
`40`	`40`	`}`
Original file line number	Diff line number	Diff line change
`@@ -312,7 +312,7 @@ func TestLevelIterEquivalence(t *testing.T) {`
`312`	`312`	`amap[metas[i].FileNum] = metas[i]`
`313`	`313`	`}`
`314`	`314`	`b.AddedTables[6] = amap`
`315`		`- v, err := b.Apply(nil, base.DefaultComparer, 0, 0)`
	`315`	`+ v, err := b.Apply(manifest.NewVersion(base.DefaultComparer), 0, 0)`
`316`	`316`	`require.NoError(t, err)`
`317`	`317`	`levelIter.Init(`
`318`	`318`	`context.Background(),`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ func makeTestVersion(numFiles int) (Version, []TableMetadata) {`
`26`	`26`	`var levelFiles [7][]*TableMetadata`
`27`	`27`	`levelFiles[6] = files`
`28`	`28`
`29`		`- v := NewVersion(base.DefaultComparer, 0, levelFiles)`
	`29`	`+ v := NewVersionForTesting(base.DefaultComparer, 0, levelFiles)`
`30`	`30`	`return v, files`
`31`	`31`	`}`
`32`	`32`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ func readManifest(filename string) (*Version, error) {`
`32`	`32`	`}`
`33`	`33`	`defer f.Close()`
`34`	`34`	`rr := record.NewReader(f, 0 /* logNum */)`
`35`		`- var v *Version`
	`35`	`+ v := NewVersion(base.DefaultComparer)`
`36`	`36`	`addedByFileNum := make(map[base.FileNum]*TableMetadata)`
`37`	`37`	`for {`
`38`	`38`	`r, err := rr.Next()`
`@@ -51,7 +51,7 @@ func readManifest(filename string) (*Version, error) {`
`51`	`51`	`if err := bve.Accumulate(&ve); err != nil {`
`52`	`52`	`return nil, err`
`53`	`53`	`}`
`54`		`- if v, err = bve.Apply(v, base.DefaultComparer, 10<<20, 32000); err != nil {`
	`54`	`+ if v, err = bve.Apply(v, 10<<20, 32000); err != nil {`
`55`	`55`	`return nil, err`
`56`	`56`	`}`
`57`	`57`	`}`
Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,7 @@ func replayManifest(t testing.T, opts pebble.Options, dirname string) *manifes`
`127`	`127`	`require.NoError(t, bve.Accumulate(&ve))`
`128`	`128`	`}`
`129`	`129`	`v, err := bve.Apply(`
`130`		`- nil /* version */, cmp, opts.FlushSplitBytes,`
	`130`	`+ manifest.NewVersion(cmp), opts.FlushSplitBytes,`
`131`	`131`	`opts.Experimental.ReadCompactionRate)`
`132`	`132`	`require.NoError(t, err)`
`133`	`133`	`return v`