Skip to content

Commit d7f926e

Browse files
RaduBerindeRaduBerinde
committed
manifest: remove SumAggregator
We also move the test-only NumFilesAnnotator to the test package. This move triggered some false positives for `gcassert` which the rest of the commit addresses (by including the `.` package).
1 parent 3e3c617 commit d7f926e

File tree

5 files changed

+123
-126
lines changed

5 files changed

+123
-126
lines changed

‎db.go‎

Lines changed: 0 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -2695,105 +2695,6 @@ func (d *DB) ObjProvider() objstorage.Provider {
26952695
return d.objProvider
26962696
}
26972697

2698-
func (d *DB) checkVirtualBounds(m *manifest.TableMetadata) {
2699-
if !invariants.Enabled {
2700-
return
2701-
}
2702-
2703-
objMeta, err := d.objProvider.Lookup(base.FileTypeTable, m.TableBacking.DiskFileNum)
2704-
if err != nil {
2705-
panic(err)
2706-
}
2707-
if objMeta.IsExternal() {
2708-
// Nothing to do; bounds are expected to be loose.
2709-
return
2710-
}
2711-
2712-
iters, err := d.newIters(context.TODO(), m, nil, internalIterOpts{}, iterPointKeys|iterRangeDeletions|iterRangeKeys)
2713-
if err != nil {
2714-
panic(errors.Wrap(err, "pebble: error creating iterators"))
2715-
}
2716-
defer func() { _ = iters.CloseAll() }()
2717-
2718-
if m.HasPointKeys {
2719-
pointIter := iters.Point()
2720-
rangeDelIter := iters.RangeDeletion()
2721-
2722-
// Check that the lower bound is tight.
2723-
pointKV := pointIter.First()
2724-
rangeDel, err := rangeDelIter.First()
2725-
if err != nil {
2726-
panic(err)
2727-
}
2728-
if (rangeDel == nil || d.cmp(rangeDel.SmallestKey().UserKey, m.PointKeyBounds.Smallest().UserKey) != 0) &&
2729-
(pointKV == nil || d.cmp(pointKV.K.UserKey, m.PointKeyBounds.Smallest().UserKey) != 0) {
2730-
panic(errors.Newf("pebble: virtual sstable %s lower point key bound is not tight", m.TableNum))
2731-
}
2732-
2733-
// Check that the upper bound is tight.
2734-
pointKV = pointIter.Last()
2735-
rangeDel, err = rangeDelIter.Last()
2736-
if err != nil {
2737-
panic(err)
2738-
}
2739-
if (rangeDel == nil || d.cmp(rangeDel.LargestKey().UserKey, m.PointKeyBounds.LargestUserKey()) != 0) &&
2740-
(pointKV == nil || d.cmp(pointKV.K.UserKey, m.PointKeyBounds.Largest().UserKey) != 0) {
2741-
panic(errors.Newf("pebble: virtual sstable %s upper point key bound is not tight", m.TableNum))
2742-
}
2743-
2744-
// Check that iterator keys are within bounds.
2745-
for kv := pointIter.First(); kv != nil; kv = pointIter.Next() {
2746-
if d.cmp(kv.K.UserKey, m.PointKeyBounds.Smallest().UserKey) < 0 || d.cmp(kv.K.UserKey, m.PointKeyBounds.LargestUserKey()) > 0 {
2747-
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, kv.K.UserKey))
2748-
}
2749-
}
2750-
s, err := rangeDelIter.First()
2751-
for ; s != nil; s, err = rangeDelIter.Next() {
2752-
if d.cmp(s.SmallestKey().UserKey, m.PointKeyBounds.Smallest().UserKey) < 0 {
2753-
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.SmallestKey().UserKey))
2754-
}
2755-
if d.cmp(s.LargestKey().UserKey, m.PointKeyBounds.Largest().UserKey) > 0 {
2756-
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.LargestKey().UserKey))
2757-
}
2758-
}
2759-
if err != nil {
2760-
panic(err)
2761-
}
2762-
}
2763-
2764-
if !m.HasRangeKeys {
2765-
return
2766-
}
2767-
rangeKeyIter := iters.RangeKey()
2768-
2769-
// Check that the lower bound is tight.
2770-
if s, err := rangeKeyIter.First(); err != nil {
2771-
panic(err)
2772-
} else if m.HasRangeKeys && d.cmp(s.SmallestKey().UserKey, m.RangeKeyBounds.SmallestUserKey()) != 0 {
2773-
panic(errors.Newf("pebble: virtual sstable %s lower range key bound is not tight", m.TableNum))
2774-
}
2775-
2776-
// Check that upper bound is tight.
2777-
if s, err := rangeKeyIter.Last(); err != nil {
2778-
panic(err)
2779-
} else if d.cmp(s.LargestKey().UserKey, m.RangeKeyBounds.LargestUserKey()) != 0 {
2780-
panic(errors.Newf("pebble: virtual sstable %s upper range key bound is not tight", m.TableNum))
2781-
}
2782-
2783-
s, err := rangeKeyIter.First()
2784-
for ; s != nil; s, err = rangeKeyIter.Next() {
2785-
if d.cmp(s.SmallestKey().UserKey, m.RangeKeyBounds.SmallestUserKey()) < 0 {
2786-
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.SmallestKey().UserKey))
2787-
}
2788-
if d.cmp(s.LargestKey().UserKey, m.RangeKeyBounds.LargestUserKey()) > 0 {
2789-
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.LargestKey().UserKey))
2790-
}
2791-
}
2792-
if err != nil {
2793-
panic(err)
2794-
}
2795-
}
2796-
27972698
// DebugString returns a debugging string describing the LSM.
27982699
func (d *DB) DebugString() string {
27992700
return d.DebugCurrentVersion().DebugString()

‎file_cache_test.go‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/cockroachdb/errors"
2222
"github.com/cockroachdb/pebble/internal/base"
2323
"github.com/cockroachdb/pebble/internal/cache"
24+
"github.com/cockroachdb/pebble/internal/invariants"
2425
"github.com/cockroachdb/pebble/internal/manifest"
2526
"github.com/cockroachdb/pebble/internal/testkeys"
2627
"github.com/cockroachdb/pebble/objstorage"
@@ -1381,3 +1382,102 @@ func (tl *catchFatalLogger) Errorf(format string, args ...interface{}) {}
13811382
func (tl *catchFatalLogger) Fatalf(format string, args ...interface{}) {
13821383
tl.fatalMsgs = append(tl.fatalMsgs, fmt.Sprintf(format, args...))
13831384
}
1385+
1386+
func (d *DB) checkVirtualBounds(m *manifest.TableMetadata) {
1387+
if !invariants.Enabled {
1388+
return
1389+
}
1390+
1391+
objMeta, err := d.objProvider.Lookup(base.FileTypeTable, m.TableBacking.DiskFileNum)
1392+
if err != nil {
1393+
panic(err)
1394+
}
1395+
if objMeta.IsExternal() {
1396+
// Nothing to do; bounds are expected to be loose.
1397+
return
1398+
}
1399+
1400+
iters, err := d.newIters(context.TODO(), m, nil, internalIterOpts{}, iterPointKeys|iterRangeDeletions|iterRangeKeys)
1401+
if err != nil {
1402+
panic(errors.Wrap(err, "pebble: error creating iterators"))
1403+
}
1404+
defer func() { _ = iters.CloseAll() }()
1405+
1406+
if m.HasPointKeys {
1407+
pointIter := iters.Point()
1408+
rangeDelIter := iters.RangeDeletion()
1409+
1410+
// Check that the lower bound is tight.
1411+
pointKV := pointIter.First()
1412+
rangeDel, err := rangeDelIter.First()
1413+
if err != nil {
1414+
panic(err)
1415+
}
1416+
if (rangeDel == nil || d.cmp(rangeDel.SmallestKey().UserKey, m.PointKeyBounds.Smallest().UserKey) != 0) &&
1417+
(pointKV == nil || d.cmp(pointKV.K.UserKey, m.PointKeyBounds.Smallest().UserKey) != 0) {
1418+
panic(errors.Newf("pebble: virtual sstable %s lower point key bound is not tight", m.TableNum))
1419+
}
1420+
1421+
// Check that the upper bound is tight.
1422+
pointKV = pointIter.Last()
1423+
rangeDel, err = rangeDelIter.Last()
1424+
if err != nil {
1425+
panic(err)
1426+
}
1427+
if (rangeDel == nil || d.cmp(rangeDel.LargestKey().UserKey, m.PointKeyBounds.LargestUserKey()) != 0) &&
1428+
(pointKV == nil || d.cmp(pointKV.K.UserKey, m.PointKeyBounds.Largest().UserKey) != 0) {
1429+
panic(errors.Newf("pebble: virtual sstable %s upper point key bound is not tight", m.TableNum))
1430+
}
1431+
1432+
// Check that iterator keys are within bounds.
1433+
for kv := pointIter.First(); kv != nil; kv = pointIter.Next() {
1434+
if d.cmp(kv.K.UserKey, m.PointKeyBounds.Smallest().UserKey) < 0 || d.cmp(kv.K.UserKey, m.PointKeyBounds.LargestUserKey()) > 0 {
1435+
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, kv.K.UserKey))
1436+
}
1437+
}
1438+
s, err := rangeDelIter.First()
1439+
for ; s != nil; s, err = rangeDelIter.Next() {
1440+
if d.cmp(s.SmallestKey().UserKey, m.PointKeyBounds.Smallest().UserKey) < 0 {
1441+
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.SmallestKey().UserKey))
1442+
}
1443+
if d.cmp(s.LargestKey().UserKey, m.PointKeyBounds.Largest().UserKey) > 0 {
1444+
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.LargestKey().UserKey))
1445+
}
1446+
}
1447+
if err != nil {
1448+
panic(err)
1449+
}
1450+
}
1451+
1452+
if !m.HasRangeKeys {
1453+
return
1454+
}
1455+
rangeKeyIter := iters.RangeKey()
1456+
1457+
// Check that the lower bound is tight.
1458+
if s, err := rangeKeyIter.First(); err != nil {
1459+
panic(err)
1460+
} else if m.HasRangeKeys && d.cmp(s.SmallestKey().UserKey, m.RangeKeyBounds.SmallestUserKey()) != 0 {
1461+
panic(errors.Newf("pebble: virtual sstable %s lower range key bound is not tight", m.TableNum))
1462+
}
1463+
1464+
// Check that upper bound is tight.
1465+
if s, err := rangeKeyIter.Last(); err != nil {
1466+
panic(err)
1467+
} else if d.cmp(s.LargestKey().UserKey, m.RangeKeyBounds.LargestUserKey()) != 0 {
1468+
panic(errors.Newf("pebble: virtual sstable %s upper range key bound is not tight", m.TableNum))
1469+
}
1470+
1471+
s, err := rangeKeyIter.First()
1472+
for ; s != nil; s, err = rangeKeyIter.Next() {
1473+
if d.cmp(s.SmallestKey().UserKey, m.RangeKeyBounds.SmallestUserKey()) < 0 {
1474+
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.SmallestKey().UserKey))
1475+
}
1476+
if d.cmp(s.LargestKey().UserKey, m.RangeKeyBounds.LargestUserKey()) > 0 {
1477+
panic(errors.Newf("pebble: virtual sstable %s point key %s is not within bounds", m.TableNum, s.LargestKey().UserKey))
1478+
}
1479+
}
1480+
if err != nil {
1481+
panic(err)
1482+
}
1483+
}

‎internal/lint/lint_test.go‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"fmt"
1010
"go/build"
1111
"os/exec"
12+
"path/filepath"
1213
"regexp"
1314
"runtime"
1415
"slices"
@@ -108,20 +109,24 @@ func TestLint(t *testing.T) {
108109
installTool(t, gcassert)
109110
t.Parallel()
110111

111-
// Build a list of all packages that contain a gcassert directive.
112-
var packages []string
112+
// Build a list of all packages that contain a gcassert directive. Always
113+
// include the top-level package to reduce pruning of code that isn't used
114+
// within `packages`.
115+
packages := []string{"."}
113116
if err := stream.ForEach(
114117
dirCmd(
115118
t, pkg.Dir, "git", "grep", "-nE", `// ?gcassert`,
116119
), func(s string) {
117120
// s here is of the form
118121
// some/package/file.go:123:// gcassert:inline
119122
// and we want to extract the package path.
120-
filePath := s[:strings.Index(s, ":")] // up to the line number
121-
pkgPath := filePath[:strings.LastIndex(filePath, "/")] // up to the file name
122-
path := fmt.Sprintf("./%s", pkgPath)
123-
if !slices.Contains(packages, path) {
124-
packages = append(packages, path)
123+
filePath := s[:strings.Index(s, ":")] // up to the line number
124+
pkgPath := "."
125+
if dir := filepath.Dir(filePath); dir != "." {
126+
pkgPath = "./" + dir
127+
}
128+
if !slices.Contains(packages, pkgPath) {
129+
packages = append(packages, pkgPath)
125130
}
126131
}); err != nil {
127132
t.Error(err)

‎internal/manifest/annotator.go‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -376,26 +376,6 @@ func (sa SumAggregator[T]) Merge(src *T, dst *T) *T {
376376
return dst
377377
}
378378

379-
// SumAnnotator takes a function that computes a uint64 value from a single
380-
// TableMetadata and returns an TableAnnotator that sums together the values across
381-
// files.
382-
func SumAnnotator(
383-
accumulate func(f *TableMetadata) (v uint64, cacheOK bool),
384-
) *TableAnnotator[uint64] {
385-
return NewTableAnnotator[uint64](SumAggregator[uint64]{
386-
AddFunc: func(src, dst *uint64) { *dst += *src },
387-
AccumulateFunc: accumulate,
388-
})
389-
}
390-
391-
// NumFilesAnnotator is an TableAnnotator which computes an annotation value
392-
// equal to the number of files included in the annotation. Particularly, it
393-
// can be used to efficiently calculate the number of files in a given key
394-
// range using range annotations.
395-
var NumFilesAnnotator = SumAnnotator(func(f *TableMetadata) (uint64, bool) {
396-
return 1, true
397-
})
398-
399379
// PickFileAggregator implements the AnnotationAggregator interface. It defines
400380
// an aggregator that picks a single file from a set of eligible files.
401381
type PickFileAggregator struct {

‎internal/manifest/annotator_test.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,17 @@ import (
1212
"github.com/stretchr/testify/require"
1313
)
1414

15+
// NumFilesAnnotator is an TableAnnotator which computes an annotation value
16+
// equal to the number of files included in the annotation. Particularly, it
17+
// can be used to efficiently calculate the number of files in a given key
18+
// range using range annotations.
19+
var NumFilesAnnotator = NewTableAnnotator[uint64](SumAggregator[uint64]{
20+
AddFunc: func(src, dst *uint64) { *dst += *src },
21+
AccumulateFunc: func(f *TableMetadata) (uint64, bool) {
22+
return 1, true
23+
},
24+
})
25+
1526
// Creates a version with numFiles files in level 6.
1627
func makeTestVersion(numFiles int) (*Version, []*TableMetadata) {
1728
files := make([]*TableMetadata, numFiles)

0 commit comments

Comments
 (0)