perf(table): retain only required delete stats

[fallintoplace]

Summary

• Keep only the stats needed by each delete index.
• Positional deletes retain file_path metrics for partition-scoped pruning.
• Equality deletes retain metrics for EqualityFieldIDs().
• Deletion vectors retain no stats, but keep the metadata needed to read the Puffin range.
• Store only the compact delete file and sequence number in the indexes.
• Release the original delete manifest entries before data task planning continues.
• Keep the existing fallback for external or malformed DataFile implementations.

Benchmark

Command:
go test ./table -run "^$" -bench "^BenchmarkDeleteIndexRetainsOnlyRequiredStats$" -benchmem -benchtime=1s

Apple M1 Pro, 100 delete files, 512 stat fields:

index	ns/op	B/op	allocs/op	source stats/file	retained stats/file
positional	227,435	346,987	4,211	2,565	5
equality	185,194	238,189	3,731	2,560	10

The source and retained columns count entries across value, null, NaN, lower-bound, and upper-bound maps.

Tests

• go test ./...
• go test -race ./table -run "TestCompactDeleteFileForIndex|TestDeleteIndexesRetainOnlyRequiredStats|Test(PositionalDeleteIndex|EqualityDeleteIndex|BuildDVIndex|MatchDV|PlanFiles)" -count=1
• go vet ./table

[zeroshade]

The compact delete-file representation keeps what the delete readers need, but it drops the metadata compaction relies on to recognise a file-scoped positional delete. That plus a conflicted branch is what's holding this up.

Major — inferred path scope is lost before compaction sees it (table/positional_delete_index.go:41)

The path-scoped branch calls compactDeleteFileForIndex(deleteFile, partition, nil). For a v2 position delete with no ReferencedDataFile, the target is inferred from equal file_path lower/upper bounds — and this call drops those bounds from the compact copy.

Downstream, table/compaction.isFileScoped can no longer infer the reference, referencedDataFilePath returns "", and fileScopedDeletedRows therefore omits that delete's Count. Concretely: 40 deleted rows out of 100 on a right-sized file can fall below DeleteFileThreshold and miss the configured DeleteRatioThreshold, so compaction that should run is skipped.

Impact is maintenance, not data loss — scans still return the correct rows. What's lost is space reclamation, silently.

Why green CI doesn't contradict this, since it otherwise looks like it should: TestPlanCompaction_BoundsScopedPositionalDeleteRatioForcesCompaction does document the intended behaviour, but it lives in package compaction_test, constructs testDataFile and its tasks directly with bounds set, and calls cfg.PlanCompaction without ever passing through scanner index construction. It cannot observe what the scanner now hands to compaction, so the regression sits in a seam no test crosses.

Fix either way works: preserve the file_path bounds when the reference is inferred, or populate ReferencedDataFile explicitly on the compacted copy. I'd lean toward the explicit reference — it removes the dependency on bounds-inference entirely rather than keeping a second thing that must stay in sync.

Minor — add a test across that seam (table/delete_file_index_test.go:227-254)

The current path-scoped test uses an explicit ReferencedDataFile and deliberately expects stats to be nil. Please add a bounds-only case that routes a positional delete through scanner index construction into compaction.PlanCompaction and asserts the ratio-triggered group, so this metadata contract is actually covered rather than assumed.

Please rebase

mergeStateStatus is DIRTY. Twelve PRs from this stack merged in the last couple of hours, and the duplicated schema.go MarshalJSON hunk this branch carries has already landed on main — that's the likely conflict, and it should simply vanish on rebase.

What I checked on the other consumers

This was the interaction I was most concerned about, and it's handled: equality deletes retain all five selected metrics for EqualityFieldIDs including NaN counts, which is what keeps the just-merged #1960 pruning working correctly. Equality readers use equality IDs and path; position readers use path and file metadata; DV readers use the preserved Puffin reference plus offset and size. All intact.

One note on the benchmark: it reports absolute figures (positional 227,435 ns/op / 346,987 B/op with 2,565 source stats reduced to 5; equality 185,194 ns/op / 238,189 B/op with 2,560 reduced to 10) but no before/after baseline. The retained-stat counts make the intent clear, so this isn't a finding — a baseline comparison would just make the win easier to see.

CI green, all 3 commits signed off.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.

More on how to contribute to Apache Iceberg Go: CONTRIBUTING.md

[laskoviymishka]

Catch-up pass on the three commits now on the branch (c504683, 31c21fe, 11d180e). I'm holding this rather than approving, and the one gating item is a merge/rebase problem, not the code itself.

The branch is based one commit short of the streaming scanner (#1974, now main's head). Both sides rewrote planFilesLocal, and this PR changes the DV index type, so a 3-way merge into current main conflicts in table/scanner.go and, once resolved, collides on the DV type (buildDVIndex now returns map[string]deleteFileIndexEntry while main's planDataManifestTasks / fileScanTaskForDataEntry still take map[string]iceberg.ManifestEntry). I confirmed the conflict with a merge-tree; the branch checks stay green because they don't cover the merge result. A rebase onto current main, threading the new type through the streaming path, is what unblocks this.

The substantive fix this round (11d180e) is good: preserving the inferred positional-delete scope so the compact copy still carries its referenced data file into compaction, with a regression test. The equality-pruning and schema concerns from my earlier look are resolved now that main moved forward. What's left in the inline notes is cleanup: an always-nil error return that's now spread across two functions, a synthetic partition spec that sets source IDs to partition-field IDs, and a gap in the trimming-equivalence tests. None of those block on their own.

Leaving this pending so it can be finalized.

[laskoviymishka]

This is the same base-staleness issue from last round, and I was able to pin it down concretely this time. The branch sits on 8778910, one commit before the streaming scanner (#1974) that's now main's head, and both sides rewrote planFilesLocal.

buildDVIndex here returns map[string]deleteFileIndexEntry and matchDVToData takes it, but on current main planDataManifestTasks and fileScanTaskForDataEntry still thread dvIndex map[string]iceberg.ManifestEntry into matchDVToData. A merge-tree of this branch into origin/main conflicts in scanner.go, and once that's resolved the DV type won't line up with the streaming path.

I'd rebase onto current main, thread deleteFileIndexEntry through planDataManifestTasks and fileScanTaskForDataEntry, and land the entries.*Entries = nil release just before the planDataManifestTasks call. Worth building the merge result locally, since the branch checks stay green regardless.

[laskoviymishka]

Still the always-nil error return, and the split into compactDeleteFileForIndex / compactDeleteFileForIndexWithReference doubled it: both declare (iceberg.DataFile, error), the NewDataFileBuilder failure path returns (file, nil) as the intended fallback, and builder.Build() has no error. So the dead if err != nil branch is now in four call sites (equality_delete_index.go:481, positional_delete_index.go:62 and its by-path sibling, scanner.go:883).

I'd drop the error from both signatures and simplify the callers, keeping the fallback comment so the "hold onto the original file for external/malformed metadata" intent stays documented. wdyt?

[laskoviymishka]

Unchanged from last round. partition is keyed by partition-field IDs, so SourceIDs: []int{fieldID} and FieldID: fieldID end up equal, which claims the source column is field 1000 rather than the real schema column.

No impact on the current lookups, since they take (specID, partition map) directly and never interpret this synthetic spec. But the compacted DataFile rides along in the FileScanTask, and now that 11d180e is deliberately making these compact copies survive into compaction planning, a consumer that resolves compacted.SpecID() to reason about partitioning would read a source-id that doesn't match the schema. I'd rather preserve the original spec than hand out one with wrong source IDs, or at least drop a comment saying the source IDs are synthetic.

[laskoviymishka]

This new test is the right shape, it proves a bounds-scoped positional delete keeps its ReferencedDataFile all the way through scan and into compaction grouping, which is exactly the kind of "compaction dropped a needed field" regression I was worried about last round.

The gap that's left is the general one: nothing asserts a compact index returns the same delete-file matches as a full-stat index across the equality and DV paths too. This test covers the positional referenced-file case specifically. A small case that builds both a full-stat and a compact index from the same entries and asserts forDataFile returns the same set would lock the whole trimming contract, not just this slice. Not blocking, and I'd be fine taking it as a follow-up given the merge rework this needs anyway.

[zeroshade]

The trimming contract itself is safe — I verified it end to end, including through a real Puffin DV — and all four of my prior items plus three of @laskoviymishka's four are fixed. But the parity test added in the head commit specifically to close the remaining gap cannot fail, and the DV path pays the full compaction cost for a measured zero benefit.

Blocking — table/delete_file_index_test.go:309: TestDeleteIndexesRetainMatchingParity is a tautology

This test was added in aa230a5c in response to @laskoviymishka's "nothing asserts a compact index returns the same delete-file matches as a full-stat index". It does not close that gap. I built four separate mutations that each break the trimming contract and ran only this test against each:

mutation	breaks the contract?	this test
dataFileStatsForFields → all-nil (drop every stat)	yes — 6 other tests fail	PASS
byPath compaction drops the explicit &path ref (reverts 5fafb17f)	yes — TestPlanCompaction_BoundsScopedPositionalDeleteRatioFromScan fails	PASS
drop EqualityFieldIDs from the compact copy	yes — 24 tests fail	PASS
drop content_offset/content_size_in_bytes	yes — 1 test fails	PASS

Per subtest:

• :310 positional — the delete file has equal file_path bounds, so it routes to byPath, and appendPositionalDeletesFromSequence (positional_delete_index.go:165) does no stat-based filtering at all. fullIndex is hand-built in byPath too. The assertion reduces to ["position-delete.parquet"] == ["position-delete.parquet"].
• :354 equality — delete bounds [10,20], data bounds [0,30]: overlapping. Both indexes return the file whether the bounds are present, absent, or garbage, because absent bounds are conservative.
• :399 deletion vector — matchDVToData uses only path + sequence number. No stats involved by construction.

Fix: pick scenarios where the two indexes must diverge if trimming is wrong.

• Equality: use a disjoint delete range (delete [100,200], data [0,30]) and assert the compact index still prunes it — that fails the moment equality-field bounds stop being retained.
• Positional: use unequal file_path bounds so the file lands in byPartition, and assert the compact index still prunes a data path outside [lower, upper].
• DV: drop the subtest, or assert the Puffin range end-to-end (ref/offset/size non-nil after PlanFiles) — as written it asserts nothing.

Supporting datapoint: dropping the []int{filePathFieldID} retention at positional_delete_index.go:59 fails only TestDeleteIndexesRetainOnlyRequiredStats, a white-box assertion that the map keys exist. Nothing anywhere exercises filePathMayMatch on the retained bounds behaviourally. My probe does; it should live in the tree.

Major — table/scanner.go:906: the DV compact copy is pure overhead

indexedFile := compactDeleteFileForIndex(deleteFile, dataFilePartition(deleteFile), nil)

A Puffin DV manifest entry carries no column statistics, so there is nothing to trim. Measured on 100 DVs:

• retained heap: 53,464 B → 53,256 B — 208 bytes total, 2 B per DV, 0.39%
• construction: 7.16 µs → 67.6 µs (+845%, p=0.008), 5.3 KiB → 54.6 KiB, 4 → 804 allocations

Every byte the PR set out to reclaim here is already reclaimed by the deleteFileIndexEntry struct itself, which drops the ManifestEntry. Replacing the call with file: deleteFile keeps that benefit, removes 800 allocations per 100 DVs, and removes the synthetic partition spec from the DV path entirely. The statFieldIDs == nil argument is itself the signal that there's nothing to select.

The same reasoning partly applies to the byPath positional branch (positional_delete_index.go:44, also statFieldIDs = nil), but there the explicit &path materialisation is load-bearing for compaction.isFileScoped, so that copy has to stay.

What checks out — the trimming is safe

End-to-end probe, v2 table, two data files, four delete files covering every path the trimmed index touches (partition-scoped positional with unequal bounds → byPartition; bounds-inferred file-scoped → byPath; overlapping equality; disjoint equality). Rows [2 4 8], disjoint equality correctly pruned. Identical output against a build with compaction physically disabled — same rows, same delete→task attachment.

Separately drove a real Puffin DV (dv.NewDVWriter, positions {1,3}) through actual scan planning on a v3 table: PlanFiles attached it, the compact copy carried ref/offset=4/size=44/count=2, rows came back [1 3]. Identical on both builds. Worth knowing: the existing TestDVScanEndToEnd hand-builds []FileScanTask and never crosses buildDVIndex, so it does not cover this.

Retained-heap A/B, 100 delete files — the memory win is real and holds at narrow widths too:

index	stat fields	base	HEAD	delta
positional	8	510,856 B	151,640 B	−70.3%
positional	512	22,070,840 B	146,064 B	−99.34%
equality	8	308,128 B	96,960 B	−68.5%
equality	512	21,616,160 B	96,960 B	−99.55%
DV	0	53,464 B	53,256 B	−0.39%

selectByteStats' slices.Clone genuinely severs the source backing arrays — the 22 MB really is released, not aliased.

Consumer census. I checked every reader of delete-file metadata downstream of planning against the retained set — filePathMayMatch, equality field metrics, compaction.referencedDataFilePath, readAllDeleteFiles, DV read + sameDVBlob, equality_delete_reader.go:520, scan_metrics.go:96, codec/file_scan_task.go:71. All satisfied. Notably the delete file's pos-column bounds are read by nothing (arrow_scanner.go:705 reads the pos column data, not its statistics), so dropping them is safe.

Every failure mode is conservative, not optimistic. Forcing dataFileStatsForFields to return all-nil changed the probe from eq=1 to eq=2 — more delete files attached, never fewer — with byte-identical rows. equalityDeleteCanContainData continues on absent bounds; filePathMayMatch skips its checks on absent counts. No silent-data-corruption path exists in this PR.

golangci-lint 0 issues; -race -count=5 clean; CI 15/15; git merge-tree upstream/main aa230a5c clean.

Minor

• delete_file_index.go:213 — selectByteStats does slices.Clone(value), then the only consumer (builder.LowerBoundValues/UpperBoundValues) calls mapToAvroColMapClonedBytes, which clones again. Drop the first.
• delete_file_index.go:99-101 + equality_delete_index.go:481 — equality field IDs are cloned three times (call site, :101, then builder.EqualityFieldIDs); same double-clone for KeyMetadata and SplitOffsets. ~6 allocations per delete file, material against the measured 42/file.
• delete_file_index.go:59 — the synthetic spec's only product is dead weight. fieldNameToID is read only by initPartitionData, which is a guaranteed no-op here (the builder pre-sets fieldIDToPartitionData at the same length), and PartitionData is unconditionally overwritten by MarshalAvroEntry from DataFilePartitionRef + the caller's real spec. So every compact copy retains two maps keyed by partition_<id> names that nothing reads: 1,516 B/file partitioned vs 970 B/file unpartitioned, and roughly two-thirds of that 546 B delta is provably dead. This is a memory PR and the fix is in scope.
• scanner.go:900-902 — buildDVIndex tests ref != nil but not *ref != "", so a DV with an empty-string referenced_data_file is indexed under "" and compaction.referencedDataFilePath (stricter) can no longer fall back to file_path bounds. Robustness only — catalog metadata is trusted per the threat model — but worth aligning the two predicates.
• delete_file_index.go:42-44 — the doc says positional indexes select filePathFieldID; true only for byPartition. byPath selects nothing and relies on the materialised reference. That exact distinction is what broke last round, so it's worth a sentence.
• Intermediate commit 5fafb17f doesn't build its test binary (type errors in scanner_plan_tasks_bench_test.go / scanner_internal_test.go); aa230a5c fixes it. Harmless under squash-merge, noted for git bisect.
• scanner.go:1551-1553 — the release is genuinely effective, not cosmetic: newManifestEntries() returns a *manifestEntries, so nilling the fields mutates the shared heap object and nothing else retains it. Good change, accurate comment.

Prior items

Mine (CHANGES_REQUESTED 2026-09-01):

1. Inferred path scope lost before compaction sees it → Fixed. 5fafb17f passes &path into compactDeleteFileForIndexWithReference. Mutation-verified: reverting that single argument makes TestPlanCompaction_BoundsScopedPositionalDeleteRatioFromScan fail and nothing else. Probe confirms ReferencedDataFile() is now data-1.parquet where the manifest had nil. You took the option I recommended.
2. Test across that seam → Fixed, and confirmed a live guard rather than decoration.
3. Rebase → Fixed.
4. Benchmark reports absolutes with no baseline → Still open. I measured it: construction is +1295% to +2876% slower and +1158% to +3225% more bytes, in exchange for 99.3–99.6% less retained statistics (wide) / 50–70% (narrow). See below.
5. Equality NaN counts for #1960 → re-confirmed; dropping them fails TestEqualityDeleteIndexPrunesNullOnlyMetricRanges and TestEqualityDeleteIndexDoesNotUseUncertainFloatBounds.

@laskoviymishka: base one commit short of #1974 → Fixed. Always-nil error return → Fixed, four dead branches gone. Synthetic spec source IDs → Partially fixed (documentation option taken). I verified why that's defensible: SpecID() returns the real ID, Partition() returns real id-keyed values, the synthetic names are unreachable, and a full EncodeFileScanTask→DecodeFileScanTask round-trip of a compacted partitioned positional delete preserves partition values, path, count and the retained file_path lower bound. The leftover is wasted retention (Minor), not incorrect source IDs escaping. Compact-vs-full parity across equality and DV → Still open — present in form, inert in substance (Blocking).

Description

• Omits a public-surface behaviour change. The compact copy now materialises an inferred referenced_data_file on path-scoped positional deletes: FileScanTask.DeleteFiles[i].ReferencedDataFile() was nil before and is now the inferred data path, while the file_path bounds that used to be there are gone. That's the whole point of 5fafb17f and it belongs in the summary — a downstream consumer round-tripping a scan-planned delete file into a manifest will now write a field the source manifest didn't carry.
• Omits table/compaction/analyze_test.go and any mention of compaction. A reader can't tell the trimming interacts with compaction.isFileScoped at all — which is precisely where it broke.
• Benchmark table has no baseline and reads as a win table. The two deterministic columns reproduce exactly, and B/op and allocs/op are within run-to-run variance of your M1 figures, so it's not stale. But these absolutes are ~13× the pre-change cost; the win lives entirely in the two custom metrics. Please add the base column and label ns/op and B/op as the cost side.
• Test list is stale — omits both tests added in the last two commits.
• BenchmarkDeleteIndexRetainsOnlyRequiredStats passes a nil schema to buildEqualityDeleteIndex (delete_file_index_bench_test.go:54), short-circuiting newEqualityDeleteIndexEntry before any field-metric decoding — so the equality row measures stat selection only, not the metric path #1960 added. Worth a footnote.
• source_stats_fields/file is asserted, not measured (:37-40 computes 5*fieldCount [+5] as a constant). It matches the fixture today but will silently drift; countDataFileStats on a source file would make it self-checking, as the retained column already is.

---

This review was drafted by an AI-assisted tool and confirmed by an Iceberg Go maintainer. The findings cite the project's review criteria; if you think one is mis-applied, please reply and a maintainer will weigh in.