manifest: fix incorrect Annotator pointer aggregation

#3760 contained a bug which causes Annotator
values to be incorrectly aggregated when pointer values
should be overwritten. This is because the `vtyped`
variable was not being modified due to being on the
stack.

This change fixes this and adds a unit test for
`PickFileAggregator` to catch this issue in the future.

#3759 should already not be affected by
this due to the different way it handles aggregation.

internal/manifest/annotator.go

@@ -84,33 +84,36 @@ func (a *Annotator[T]) findAnnotation(n *node) *annotation {
 // nodeAnnotation computes this annotator's annotation of this node across all
 // files in the node's subtree. The second return value indicates whether the
 // annotation is stable and thus cacheable.
-func (a *Annotator[T]) nodeAnnotation(n *node) (v *T, cacheOK bool) {
+func (a *Annotator[T]) nodeAnnotation(n *node) (_ *T, cacheOK bool) {
  annot := a.findAnnotation(n)
- vtyped := annot.v.(*T)
+ t := annot.v.(*T)
  // If the annotation is already marked as valid, we can return it without
  // recomputing anything.
  if annot.valid {
- return vtyped, true
+ return t, true
  }
 
- annot.v = a.Aggregator.Zero(vtyped)
- annot.valid = true
+ t = a.Aggregator.Zero(t)
+ valid := true
 
  for i := int16(0); i <= n.count; i++ {
  if !n.leaf {
  v, ok := a.nodeAnnotation(n.children[i])
- annot.v = a.Aggregator.Merge(v, vtyped)
- annot.valid = annot.valid && ok
+ t = a.Aggregator.Merge(v, t)
+ valid = valid && ok
  }
 
  if i < n.count {
- v, ok := a.Aggregator.Accumulate(n.items[i], vtyped)
- annot.v = v
- annot.valid = annot.valid && ok
+ var ok bool
+ t, ok = a.Aggregator.Accumulate(n.items[i], t)
+ valid = valid && ok
  }
  }
 
- return annot.v.(*T), annot.valid
+ annot.v = t
+ annot.valid = valid
+
+ return t, annot.valid
 }
 
 // InvalidateAnnotation removes any existing cached annotations from this

internal/manifest/annotator_test.go

@@ -56,3 +56,26 @@ func BenchmarkNumFilesAnnotator(b *testing.B) {
  require.EqualValues(b, uint64(i), numFiles)
  }
 }
+
+func TestPickFileAggregator(t *testing.T) {
+ const count = 1000
+ a := Annotator[FileMetadata]{
+ Aggregator: PickFileAggregator{
+ Filter: func(f *FileMetadata) (eligible bool, cacheOK bool) {
+ return true, true
+ },
+ Compare: func(f1 *FileMetadata, f2 *FileMetadata) bool {
+ return base.DefaultComparer.Compare(f1.Smallest.UserKey, f2.Smallest.UserKey) < 0
+ },
+ },
+ }
+
+ lm, files := makeTestLevelMetadata(1)
+
+ for i := 1; i <= count; i++ {
+ lm.tree.Insert(newItem(key(i)))
+ pickedFile := a.LevelAnnotation(lm)
+ // The picked file should always be the one with the smallest key.
+ require.Same(t, files[0], pickedFile)
+ }
+}