(iceberg-go) branch main updated: fix: data races in exprEvaluator.Eval and manifestEvalVisitor.Eval (#954)

[ze...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git


The following commit(s) were added to refs/heads/main by this push:
     new e3dceccd fix: data races in exprEvaluator.Eval and manifestEvalVisitor.Eval (#954)
e3dceccd is described below

commit e3dceccd5c55cac1c91b9aec7586c395e9c930b9
Author: Varun Venkatesh <11...@users.noreply.github.com>
AuthorDate: Wed Apr 29 13:36:33 2026 -0700

    fix: data races in exprEvaluator.Eval and manifestEvalVisitor.Eval (#954)
    
    ## Problem
    
    Two evaluator types share a receiver across concurrent goroutine calls,
    causing data races detected by `-race`.
    
    **Race 1 — `exprEvaluator.Eval`** (`visitors.go`)
    
    `table.Scan.collectManifestEntries` fans out manifest opens via
    `errgroup` (`scanner.go`), and each goroutine calls the bound
    `*exprEvaluator.Eval` method value returned by `ExpressionEvaluator`.
    `Eval` writes the per-call `StructLike` to the receiver's shared `st`
    field (`e.st = st`). Because the `*exprEvaluator` is shared across the
    goroutine pool, parallel `Eval` calls race on `e.st`.
    
    **Race 2 — `manifestEvalVisitor.Eval`** (`table/evaluators.go`)
    
    `newManifestEvaluator` returns a closure that captures a shared
    `*manifestEvalVisitor`. When multiple goroutines call that closure
    concurrently (e.g. the `errgroup` in `classifyFilesForFilteredDeletions`
    and parallel manifest scans in `collectManifestEntries`), they race on
    the `partitionFields` write in `Eval` vs. the reads inside
    `VisitLess`/`VisitGreater`.
    
    ## Fix
    
    Both fixes mirror the approach used in #445 (closes #443) for
    `inclusiveMetricsEval`: instead of mutating the shared receiver, `Eval`
    builds a per-call stack-local copy of the visitor and passes `&ev` to
    `VisitExpr`. The captured receiver is read-only after construction; all
    per-call mutable state lives in the local copy.
    
    ```go
    // exprEvaluator
    func (e *exprEvaluator) Eval(st StructLike) (bool, error) {
        ev := exprEvaluator{bound: e.bound, st: st}
        return VisitExpr(ev.bound, &ev)
    }
    
    // manifestEvalVisitor
    func (m *manifestEvalVisitor) Eval(manifest iceberg.ManifestFile) (bool, error) {
        parts := manifest.Partitions()
        if len(parts) == 0 {
            return rowsMightMatch, nil
        }
        ev := manifestEvalVisitor{partitionFields: parts, partitionFilter: m.partitionFilter}
        return VisitExpr(ev.partitionFilter, &ev)
    }
    ```
    
    No API changes.
    
    ## Testing
    
    - Adds `TestManifestEvalVisitorEvalRace` in
    `table/evaluator_race_test.go`: 256 goroutines share one cached
    evaluator closure and call it concurrently. Confirmed to trip `-race`
    before the fix and pass after.
    - Existing `TestKeyDefaultMapRaceCondition` (the only prior race test)
    still passes.
    - Full `go test -race ./table/ ./...` passes (pre-existing races in
    miniohq-specific tests are unrelated).
    
    ## Related
    
    - Sibling precedent: #445 (closes #443) — same pattern for
    `inclusiveMetricsEval`.
    
    ---------
    
    Co-authored-by: Varun Venkatesh <va...@Varuns-MacBook-Pro.local>
---
 table/evaluator_race_test.go | 88 ++++++++++++++++++++++++++++++++++++++++++++
 table/evaluators.go          | 16 ++++++--
 visitors.go                  |  8 +++-
 3 files changed, 106 insertions(+), 6 deletions(-)

diff --git a/table/evaluator_race_test.go b/table/evaluator_race_test.go
new file mode 100644
index 00000000..5984ea01
--- /dev/null
+++ b/table/evaluator_race_test.go
@@ -0,0 +1,88 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+
+package table
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/apache/iceberg-go"
+)
+
+// TestManifestEvalVisitorEvalRace fans out 256 goroutines that all call the
+// same cached Eval closure concurrently. Under -race, any write to a shared
+// receiver field inside Eval (e.g. m.partitionFields = parts) plus a read
+// inside VisitGreater/VisitLess/etc. will trip the detector.
+//
+// This reproduces the same shared-receiver pattern fixed previously in
+// exprEvaluator: newManifestEvaluator returns (&manifestEvalVisitor{...}).Eval,
+// callers in transaction.go's classifyFilesForFilteredDeletions loop the
+// returned closure across an errgroup of goroutines, and every goroutine
+// races on the captured *manifestEvalVisitor.
+func TestManifestEvalVisitorEvalRace(t *testing.T) {
+	intMin := []byte{0x00, 0x00, 0x00, 0x00}
+	intMax := []byte{0x7f, 0x00, 0x00, 0x00}
+
+	schema := iceberg.NewSchema(0,
+		iceberg.NestedField{
+			ID: 1, Name: "id",
+			Type: iceberg.PrimitiveTypes.Int32, Required: false,
+		},
+	)
+
+	spec := iceberg.NewPartitionSpec(iceberg.PartitionField{
+		Name:      "id",
+		SourceIDs: []int{1},
+		FieldID:   1000,
+		Transform: iceberg.IdentityTransform{},
+	})
+
+	// Single shared evaluator — this is the analogue of
+	// manifestEvaluators.Get(specID) returning the same cached closure to
+	// every goroutine in the production callers.
+	eval, err := newManifestEvaluator(spec, schema,
+		iceberg.LessThan(iceberg.Reference("id"), int32(50)),
+		true)
+	if err != nil {
+		t.Fatalf("newManifestEvaluator: %v", err)
+	}
+
+	const goroutines = 256
+
+	manifests := make([]iceberg.ManifestFile, goroutines)
+	for i := range manifests {
+		manifests[i] = iceberg.NewManifestFile(1, "", 0, 0, int64(i)).Partitions(
+			[]iceberg.FieldSummary{
+				{
+					ContainsNull: false,
+					LowerBound:   &intMin,
+					UpperBound:   &intMax,
+				},
+			},
+		).Build()
+	}
+
+	start := make(chan struct{})
+	var wg sync.WaitGroup
+	wg.Add(goroutines)
+	for i := 0; i < goroutines; i++ {
+		i := i
+		go func() {
+			defer wg.Done()
+			<-start
+			if _, err := eval(manifests[i]); err != nil {
+				t.Errorf("eval[%d]: %v", i, err)
+			}
+		}()
+	}
+	close(start)
+	wg.Wait()
+}
diff --git a/table/evaluators.go b/table/evaluators.go
index 7dbfd030..40aaad84 100644
--- a/table/evaluators.go
+++ b/table/evaluators.go
@@ -60,13 +60,21 @@ type manifestEvalVisitor struct {
 }
 
 func (m *manifestEvalVisitor) Eval(manifest iceberg.ManifestFile) (bool, error) {
-	if parts := manifest.Partitions(); len(parts) > 0 {
-		m.partitionFields = parts
+	parts := manifest.Partitions()
+	if len(parts) == 0 {
+		return rowsMightMatch, nil
+	}
 
-		return iceberg.VisitExpr(m.partitionFilter, m)
+	// Use a per-call visitor so concurrent callers of the same cached Eval
+	// closure (e.g. the errgroup in classifyFilesForFilteredDeletions and
+	// the parallel manifest scans in scanner.collectManifestEntries) do
+	// not race on partitionFields. Mirrors the exprEvaluator fix above.
+	ev := manifestEvalVisitor{
+		partitionFields: parts,
+		partitionFilter: m.partitionFilter,
 	}
 
-	return rowsMightMatch, nil
+	return iceberg.VisitExpr(ev.partitionFilter, &ev)
 }
 
 func removeBoundCmp[T iceberg.LiteralType](bound iceberg.Literal, vals []iceberg.Literal, cmpToDelete int) []iceberg.Literal {
diff --git a/visitors.go b/visitors.go
index 9564a4ea..e0fd4dfd 100644
--- a/visitors.go
+++ b/visitors.go
@@ -201,9 +201,13 @@ type exprEvaluator struct {
 }
 
 func (e *exprEvaluator) Eval(st StructLike) (bool, error) {
-	e.st = st
+	// Use a per-call evaluator so concurrent callers (e.g. parallel
+	// manifest scans in table.Scan.collectManifestEntries) do not race
+	// on the shared `st` field. Mirrors the fix in inclusiveMetricsEval
+	// from #445.
+	ev := exprEvaluator{bound: e.bound, st: st}
 
-	return VisitExpr(e.bound, e)
+	return VisitExpr(ev.bound, &ev)
 }
 
 func (e *exprEvaluator) VisitUnbound(UnboundPredicate) bool {