varun0630 opened a new pull request, #954:
URL: https://github.com/apache/iceberg-go/pull/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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
