perf(table): skip manifests with no live entries - #1964
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
673ba7a to
ec1d33d
Compare
zeroshade
left a comment
There was a problem hiding this comment.
The new pre-open guard is conservative for both V2 counts and the V1 decoder sentinel: known-empty manifests are skipped only after partition pruning, while unknown or negative counts stay eligible for entry reads. Approving, with one test to add.
Skipping a manifest that did contain live entries would silently drop data, so the question is entirely how "no live entries" is decided. table/scanner.go:927 skips only when HasAddedFiles() and HasExistingFiles() are both known-zero, and those are != 0 checks. The V1 manifest-list decoder maps absent nullable counts to -1, so absent-count manifests are retained and read normally rather than being treated as empty. That's the right handling of the V1 nuance and it's consistent with the existing precedent in table/rewrite_manifests.go:316-320.
I also checked: the guard runs after partition evaluation, so partition-pruned manifests still skip via the evaluator first; delete manifests use the same live-count guard and a live delete manifest is retained (the updated TestFetchManifestCounters case sets AddedFiles(1) on a delete manifest and confirms it's scanned); and snapshot summaries are untouched. The scan-metrics docs are deliberately updated to define scanned/skipped as pre-open selection including known-empty manifests, which is the honest thing to do given the counters now mean something slightly different.
Minor — add a serialized V1 scanner regression (table/scanner.go:927)
The new scanner tests synthesize a V2 ManifestFile with -1 counts, but never write and read an actual format-v1 manifest-list record with absent count fields. The absent-counts path is precisely the silent-data-loss guard here, and it's currently only covered indirectly (TestInspectManifestsV1UnknownCounts is inspect-only). A V1 scan test proving a live entry is still planned would pin it. Not blocking — the implementation is already conservative — but worth having.
Evidence: committed benchmark, 256 manifests × 64 entries. All-empty planning 65.0 → 0.31 ms/op with manifest opens 256 → 0; the 10%-live case 65.6 → 10.7 ms/op, opens 256 → 26. Avoiding the opens entirely is the win.
CI green (15/15), both commits signed off.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer, who has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how to contribute to Apache Iceberg Go: CONTRIBUTING.md
Summary
added_files_countandexisting_files_countare known to be zero.Benchmark
Command:
go test -run '^$' -bench '^BenchmarkPlanFilesSkipsKnownEmptyManifests$' -benchtime=2s ./tableApple M1 Pro, 256 manifests, 64 entries per manifest.
Allocations also dropped from 81.0 MB to 0.32 MB per operation for the all-empty case, and from 81.6 MB to 9.1 MB for the mixed case.
Checks
go test ./table -count=1go test -race ./table -count=1golangci-lint run --timeout=10m