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: [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]
