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/08/12 11:44:09 UTC
[PR] perf(table): reuse FileIO during local scan planning [iceberg-go]
fallintoplace opened a new pull request, #1781:
URL: https://github.com/apache/iceberg-go/pull/1781
## Summary
- load one `FileIO` for each local planning operation
- reuse it for the manifest list and concurrent manifest entry reads
- keep empty-table planning lazy so it still skips FileIO construction
- leave remote planning and its plan-scoped IO lifecycle unchanged
## Benchmark
`BenchmarkPlanFilesFileIOReuse` plans real Avro manifests from an in-memory filesystem and reports factory loads directly.
| Manifests | Before | After |
| ---: | ---: | ---: |
| 1 | 2 loads/op | 1 load/op |
| 10 | 11 loads/op | 1 load/op |
| 100 | 101 loads/op | 1 load/op |
The in-memory wall time stays close to noise. The useful part here is removing repeated backend setup from cloud FileIO factories.
## Tests
- `go test ./table -count=1`
- repository packages excluding the unchanged `io/gocloud` package
- `go test -race ./table -run ^TestPlanFilesReusesFileIO$ -count=1`
- `go vet ./table/...`
- `golangci-lint run --timeout=10m ./table/...`
--
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] perf(table): reuse FileIO during local scan planning [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1781:
URL: https://github.com/apache/iceberg-go/pull/1781#discussion_r3832668932
##########
table/scanner.go:
##########
@@ -900,14 +924,23 @@ func (scan *Scan) planFilesLocal(ctx context.Context, acc *scanMetricsAccumulato
}
}()
+ snap, err := scan.ResolveSnapshot()
+ if err != nil || snap == nil {
+ return nil, err
+ }
+ fs, err := scan.ioF(ctx)
Review Comment:
Retaining this concrete FileIO for the entire plan bypasses later factory calls that may refresh credentials. REST tables wire `vendedCredentialRefresher.loadFS` as `ioF`, and each invocation checks `needsRenewal()` before returning an IO. A deterministic expiring-credential factory probe passes on `main`—later manifest workers reacquire FileIO—but fails on this branch with `credentials expired before open`. Long-running plans that cross the renewal window can therefore fail with storage authorization errors. Please preserve credential-renewal checkpoints while avoiding repeated backend construction, and add coverage for expiry during planning.
--
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] perf(table): reuse FileIO during local scan planning [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1781:
URL: https://github.com/apache/iceberg-go/pull/1781
--
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] perf(table): reuse FileIO during local scan planning [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1781:
URL: https://github.com/apache/iceberg-go/pull/1781#discussion_r3864468507
##########
table/scanner.go:
##########
@@ -789,6 +785,9 @@ func (scan *Scan) collectManifestEntriesWithSchema(
}
g.Go(func() error {
+ // FileIO factories may renew credentials between manifest reads. Keep
+ // this load at the worker boundary instead of retaining the IO used for
+ // the manifest list across the whole plan.
fs, err := scan.ioF(ctx)
Review Comment:
Each manifest worker now calls `scan.ioF(ctx)` again, so current head measures 2, 11, and 101 factory calls/op for 1, 10, and 100 manifests—the same values documented as “Before,” not the claimed one call/op. This fixes credential renewal by restoring the original repeated factory behavior, but removes the performance benefit that is the purpose of this PR. Please preserve renewal checkpoints while retaining a measurable reuse benefit, or revise the PR scope if this is intentionally only a refactor. The comment below saying one FileIO is shared with all workers should also be updated.
--
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] perf(table): reuse FileIO during local scan planning [iceberg-go]
Posted by "fallintoplace (via GitHub)" <gi...@apache.org>.
fallintoplace closed pull request #1781: perf(table): reuse FileIO during local scan planning
URL: https://github.com/apache/iceberg-go/pull/1781
--
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] perf(table): reuse FileIO during local scan planning [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1781:
URL: https://github.com/apache/iceberg-go/pull/1781#discussion_r3865722475
##########
table/scanner.go:
##########
@@ -742,6 +738,42 @@ func (scan *Scan) filterManifestsWithSchema(
return filtered, nil
}
+// manifestIOBatch shares a FileIO within a concurrency-sized batch. A new
+// batch loads through the factory again so factories that renew credentials
+// still get regular checkpoints without rebuilding the backend for every
+// manifest.
+type manifestIOBatch struct {
+ factory FSysF
+ limit int
+
+ mu sync.Mutex
+ fs io.IO
+ remaining int
+}
+
+func newManifestIOBatch(factory FSysF, limit int) *manifestIOBatch {
+ return &manifestIOBatch{factory: factory, limit: max(limit, 1)}
+}
+
+func (b *manifestIOBatch) acquire(ctx context.Context) (io.IO, error) {
Review Comment:
`acquire` relies on the factory itself to reject a canceled context. `FSysF` receives a context but is not required to inspect it, so after one worker fails, a queued worker can still invoke a context-agnostic factory and open another manifest.
I reproduced this 10/10 times with concurrency 1: after manifest 0 fails, factory calls increase from the expected 2 to 3 and manifest 1 is opened once. The new cancellation test masks this because its fake factory explicitly returns `ctx.Err()`.
Please have `manifestIOBatch.acquire` check cancellation itself after acquiring the mutex and before returning cached IO or invoking the factory. The regression should use a factory that ignores context so the collector—not the test double—enforces the invariant.
--
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