You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/06/27 14:59:55 UTC
[PR] fix(table): return scanner memoization errors [iceberg-go]
fallintoplace opened a new pull request, #1333:
URL: https://github.com/apache/iceberg-go/pull/1333
Summary
replace the panic-wrapping memoized factory helper with an error-carrying cache
propagate cached factory errors through scanner planning, filtered overwrite classification, and conflict validation
add regression coverage for cached factory failures and an invalid-spec manifest path that used to panic
Why
The scanner code memoized several per-spec builders using `newKeyDefaultMapWrapErr`, which converted any factory error into a panic. That meant ordinary failures like unknown partition spec ids or projection/evaluator build errors could crash the process instead of being returned to the caller.
The shared helper is also reused in transaction-side manifest classification and conflict validation, so the same panic behavior leaked there too.
Testing
go test ./table -run 'Test(KeyDefaultMapWrapErrCachesError|FetchPartitionSpecFilteredManifests_InvalidSpecIDDoesNotPanic|BuildPartitionProjectionWithInvalidSpecID|BuildManifestEvaluatorWithInvalidSpecID|BuildPartitionEvaluatorWithInvalidSpecID|FetchPartitionSpecFilteredManifests_PropagatesEvalError)$' -count=1
go test ./table -count=1
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./table/...
git diff --check
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): return scanner memoization errors [iceberg-go]
Posted by "fallintoplace (via GitHub)" <gi...@apache.org>.
fallintoplace commented on PR #1333:
URL: https://github.com/apache/iceberg-go/pull/1333#issuecomment-4833683127
@tanmayrauth @laskoviymishka Thank you both for the reviews!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): return scanner memoization errors [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1333:
URL: https://github.com/apache/iceberg-go/pull/1333#discussion_r3486649584
##########
table/transaction.go:
##########
@@ -1542,7 +1542,10 @@ func (t *Transaction) classifyFilesForFilteredDeletions(ctx context.Context, fs
for _, manifest := range manifests {
manifest := manifest // capture loop variable
g.Go(func() error {
- manifestEval := manifestEvaluators.Get(int(manifest.PartitionSpecID()))
+ manifestEval, err := manifestEvaluators.Get(int(manifest.PartitionSpecID()))
+ if err != nil {
+ return fmt.Errorf("failed to build manifest evaluator for spec %d: %w", manifest.PartitionSpecID(), err)
+ }
if manifestEval != nil {
Review Comment:
The `if manifestEval != nil` guard is dead now that the `err != nil` return runs first, `buildManifestEvaluator` only ever returns a nil func alongside a non-nil error, so past line 1548 this is always true. Harmless, but could be dropped to make the intent clearer.
##########
table/scanner.go:
##########
@@ -73,17 +85,33 @@ func newKeyDefaultMap[K comparable, V any](factory func(K) V) *keyDefaultMap[K,
}
}
-func newKeyDefaultMapWrapErr[K comparable, V any](factory func(K) (V, error)) *keyDefaultMap[K, V] {
- return &keyDefaultMap[K, V]{
- data: make(map[K]V),
- defaultFactory: func(k K) V {
- v, err := factory(k)
- if err != nil {
- panic(err)
- }
+func (k *keyDefaultMapErr[K, V]) Get(key K) (V, error) {
+ k.mx.RLock()
+ if v, ok := k.data[key]; ok {
+ k.mx.RUnlock()
+
+ return v.value, v.err
+ }
+
+ k.mx.RUnlock()
+ k.mx.Lock()
Review Comment:
Just confirming the intent: caching the error means a failing factory is never retried for that spec id (the new TestKeyDefaultMapWrapErrCachesError pins this). That's fine for these deterministic errors, unknown spec id, projection build failure but wanted to flag it explicitly since it's a behavior the helper now guarantees, not just an implementation detail.
##########
table/conflict_validation.go:
##########
@@ -376,7 +376,10 @@ func validateAddedDataFilesMatchingFilter(ctx *conflictContext, filter iceberg.B
continue
}
- mEval := manifestEvals.Get(int(mf.PartitionSpecID()))
+ mEval, err := manifestEvals.Get(int(mf.PartitionSpecID()))
+ if err != nil {
+ return fmt.Errorf("building manifest evaluator for spec %d: %w", mf.PartitionSpecID(), err)
Review Comment:
Tiny consistency nit: here the wrap is "building manifest evaluator for spec %d" (and "building partition evaluator..." on 393), but the equivalent wraps in scanner.go:626 and transaction.go:1547 say "failed to build ...". Worth aligning the prefix across the three so the messages read uniformly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): return scanner memoization errors [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1333:
URL: https://github.com/apache/iceberg-go/pull/1333
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org