perf(table): filter position-delete reads by file path

[fallintoplace]

What changed 🔥

• Collect target data-file paths per position-delete file from the scan tasks.
• Reuse the existing file_path and pos projection when reading position-delete Parquet files.
• Push a bounded file-path predicate into row-group stats and Bloom-filter pruning. A single target uses equality; 2 to 200 targets use IN (...).
• Keep exact row-level path filtering while accumulating batches, so unrelated paths do not create position builders.
• Keep the whole-file fallback when any task has no usable data-file path.
• Enable stats/Bloom pushdown only when the physical Parquet field IDs match the canonical position-delete IDs.
• Fall back to name-based reads without pushdown when those IDs are missing or non-canonical. Reserved-ID collisions still return ErrInvalidSchema.
• Keep the existing result shape and validation for dictionary-encoded and reordered delete columns.

Benchmark 📊

Apple M1 Pro, Go 1.26.3, -benchtime=100ms -count=3. The file has 1,000 sorted data-file paths and 1,024 delete rows per path, with one row group per path.

Target set	Time	Memory	Allocs
all paths (nil target set)	~51.5 ms	~172.0 MB	~201k
1 path	~4.7 ms	~17.9 MB	~81.5k
10 paths	~5.7 ms	~19.8 MB	~83.2k
100 paths	~13.4 ms	~37.4 MB	~99.5k
200 paths	~21.6 ms	~55.8 MB	~117.8k
201 paths	~39.3 ms	~156.5 MB	~187.5k
1,000 paths	~61.8 ms	~172.5 MB	~201.1k

The row-group predicate cap is inclusive at 200 targets. At 201 and above, stats/Bloom pushdown is disabled and exact row-level filtering remains responsible for correctness. This can be slower than an unfiltered read because it still performs target-set lookups.

Checks ✅

• go test ./table -count=1
• go vet ./table
• go test -race ./table -run Test(ReadDeletesForPaths|PositionDeleteRowGroupTester|ReadAllDeleteFiles) -count=1

[zeroshade]

The target-path collection, row-level filtering, fallback behavior, Arrow ownership, and performance improvement otherwise look sound; focused race tests, full table suites, vet, diagnostics, benchmarks, CI, and synthetic-main tests passed. The new stats/Bloom pushdown can still silently omit applicable deletes when physical field IDs are inconsistent.

[zeroshade]

[P1] Validate the physical field-ID mapping before enabling this tester. Column projection intentionally resolves file_path and pos by name because external files may omit Iceberg IDs, but the tester binds predicates to the canonical reserved IDs and the stats/Bloom readers index physical columns by their embedded IDs. With valid names/types but swapped reserved IDs, I reproduced an unfiltered read returning position 0 while this target-filtered read returned an empty map: pos statistics were interpreted as file_path bounds and the applicable row group was pruned. That can expose a deleted row. When IDs are present, require the canonical unique mapping (or return ErrInvalidSchema); when IDs are absent, conservatively disable stats/Bloom pruning. Please add swapped and duplicate-ID regressions.

[laskoviymishka]

Thanks for this, the path-filtering approach is a clear win. Streaming the delete file through GetRecords with a target-path set instead of materializing the whole table, and pruning row groups when the physical schema carries canonical field IDs, meaningfully cuts what we read on the position-delete path. The nil-sentinel whole-file fallback for mixed tasks is a clean way to stay correct when a task doesn't carry a usable data-file path.

I'd hold this before merging though.

Agreed with the earlier field-ID pushdown concern, and the guard you added addresses it for the pruning-enabled path. My one follow-on is that the guard is a bit too strict on the failure side: when file_path/pos are missing their canonical IDs but some other field carries one, it returns ErrInvalidSchema and aborts the entire delete read, where before this PR that file read fine (just slower). I'd degrade to no-pruning there, the same leniency the all-absent case already gets, and keep the hard error for genuinely corrupt canonical-ID collisions. Real delete files are almost always written by Iceberg writers that stamp canonical IDs, so this is defensive robustness more than a live bug, but I'd rather a weird file read slowly than fail the scan.

The other thing is test coverage on the mixed-task nil-fallback. It's the one correctness-critical branch here and I don't see it exercised end to end: two tasks sharing a delete file, one with a data-file path and one without, asserting the whole file still comes back. That path also depends on a specific ordering of the nil check vs the file-path check that nothing tests today.

A few smaller things I'd like to settle before merge:

• the >200-target cap boundary isn't tested (201 targets should send the tester nil while the row filter still applies)
• appendRecord's Column(0)/Column(1) ordering contract is undocumented
• the writePosDeleteParquetToMemFS wrapper lost its t.Helper()
• the bench "all paths" baseline re-runs four times inside the targetCount loop

Once those are addressed, happy to take another pass and approve.

[laskoviymishka]

The nil sentinel here is the whole-file fallback for the mixed-task case, and it's a different code path from the filtered read, but I don't see a test that exercises it end to end. TestReadAllDeleteFilesUsesTaskDataFilePaths only covers a single task with a valid path.

The construction I'd want: two tasks sharing this delete file, one with File.FilePath() set to data-A.parquet and one with File == nil, then assert readAllDeleteFiles returns rows for both data-A and data-B, i.e. the whole file, not just data-A. That also locks in the ordering here, since the targets == nil check has to stay ahead of the t.File == nil check and nothing tests that today. A refactor that reorders them, or swaps nil for an empty map, would silently drop the other task's deletes with everything still green.

[laskoviymishka]

appendRecord hard-codes Column(0) as file_path and Column(1) as pos. It's correct today because the only caller passes []int{filePathIndex, posIndex} to GetRecords and pqarrow preserves that order, but the contract is invisible from in here. A one-line precondition comment, or looking the columns up by name, would keep a future caller passing a different order from silently mis-attributing paths and positions.

[laskoviymishka]

len(targets) > inPredicateLimit disables the tester, but there's no test at that boundary. I'd add one with inPredicateLimit+1 targets against a delete file that has both target and non-target rows, asserting the non-target rows are still filtered out. That confirms the tester goes nil above the cap and the row-level filter still carries correctness on its own.

[laskoviymishka]

BloomPreds is asserted non-empty in the field-ID test, but I didn't find a test that writes a delete file with a bloom filter on file_path and confirms a row group actually gets pruned through this path. Worth adding one if it isn't already covered by the data-scan bloom tests, fine to skip if it is.

[laskoviymishka]

Both FindFieldByName errors get dropped here. Stable today, but if either name changes under a refactor the zero-value NestedField (ID 0) propagates silently into the ID checks below. I'd assert on the error, or pull these from named field-ID constants, so it fails loudly instead.

[laskoviymishka]

The guard you added is the right call for the genuinely corrupt cases, but I think this nil branch is a touch too strict.

A delete file from a mixed-version writer that stamps a field ID on some other column (say a v3 row field) but not on file_path/pos lands right here: len(physicalIDs) isn't 0, so the all-absent fallback above doesn't fire, and then fieldID == nil aborts the whole read. Before this PR that file read fine, just without pruning, and Java or PyIceberg would still read it. I'd rather degrade than fail the scan:

if fieldID == nil {
    // IDs present on other columns but not on file_path/pos:
    // fall back to name-based reading with no pruning.
    return false, nil
}

The murkier one is the *fieldID != want.id branch just below. A custom-but-valid writer that maps file_path to its own ID is indistinguishable from a genuinely swapped file at this check, and right now both fail the read even though the first is safe to read by name. I don't think you need to solve that here, but it's worth deciding whether non-canonical positive IDs should also degrade rather than error. wdyt?

[laskoviymishka]

The "all paths" sub-benchmark passes nil and is identical across every targetCount iteration, so it re-measures the same baseline four times. Hoisting it out of the targetCount loop runs it once and de-clutters the output.

[laskoviymishka]

The extracted WithSchema helper keeps its t.Helper(), but this wrapper dropped its own, so a failure inside the helper now points at this line instead of the calling test. Adding t.Helper() as the first line of the wrapper puts the attribution back.

[laskoviymishka]

I think this is good now.

[zeroshade]

The delete-path filtering itself is sound, but this branch has merge conflicts and needs a rebase before it can go further. Flagging that plus one test-coverage note.

Please rebase onto current main

mergeStateStatus is DIRTY — the branch conflicts with base. main has moved substantially over the last few days, which is almost certainly the cause. Please rebase and re-run CI.

The failing check is not your fault

To save you chasing it: ubuntu-latest go1.25.9 fails in the Run tests with race detector step, but the failure is a pre-existing flaky test unrelated to this PR.

TestSqlCatalog/TestConcurrentTableViewCollisionReturnsCatalogSentinel, at catalog/sql/sql_test.go:2378:

Error: Target error should be in err chain:
  expected: "view already exists"
  in chain: "failed to create table: database is locked (5) (SQLITE_BUSY)"

That's SQLite lock contention in a deliberately-concurrent sql-catalog test. This PR touches no sql-catalog code, github.com/apache/iceberg-go/table passed in the same race run, and macOS 1.25.9, macOS 1.26.1, Ubuntu 1.26.1 and s390x all passed. No action needed from you; it may well clear on the post-rebase re-run. I'll track the flake separately.

Path matching — checked, no finding

I went looking for a silent-delete-skip bug here, since appendFilePathChunk continues on an exact-map miss with no fallback (table/arrow_scanner.go:493-500, targets built at :104-109), and skipping deletes would silently resurrect deleted rows. It holds up:

• The spec requires the position-delete file_path column to be a "Full URI of a data file with FS scheme" that "must match the file_path of the target data file in a manifest entry".
• The pre-existing downstream lookup deletesPerFile[task.Value.File.FilePath()] (arrow_scanner.go:2094-2097) was already an exact string match on base main.

So s3:// vs s3a://, trailing-slash, percent-encoding, case, and relative-vs-absolute variants were already non-equivalent identifiers and already inapplicable before this PR. The change alters read materialization, not the identity contract, and introduces no new risk. Explicitly no finding.

Minor — the exactness contract isn't pinned by a test

New regression tests (table/arrow_scanner_posdelete_regression_test.go:104-117, :128-174, :258-285) use exact path strings only. Since exact matching is now load-bearing for which deletes get applied, a test asserting that a near-miss form is not treated as equivalent would lock the contract in. Non-blocking, and pre-existing behaviour — worth adding while you're here.

---

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

[zeroshade]

Following up on the CI note above: the -race flake is already tracked as #1793, so nothing new to file and nothing for you to do about it here. The rebase is the only thing blocking this one.

[zeroshade]

The delete-filtering core is correct — I went after it hard and could not break it. But the benchmark table doesn't reproduce and understates a real regression, and @laskoviymishka's explicit open question is still unanswered, so I'd like another pass before this lands.

What I verified (no correctness bug found)

I specifically hunted for a dropped-delete / resurrected-row bug and did not find one. Base for all A/B work is 87789102, materialized side by side so probes run identically on both trees.

Path normalization — 8 variants, head vs base, byte-identical. End-to-end through Scan.ReadTasks over a real LocalFS Parquet file, with the delete file recording the target path as exact, file://-prefixed, double-slash, ./-segment, trailing-slash, percent-encoded (data%2D1), uppercased, and s3a://. Only exact applies the delete (survivors [0 2 4 5 6 7 8 9]); all 7 near-misses leave [0..9] intact on base main too. Also asserted readAllDeleteFiles (filter on) == per-file readDeletes (filter off) for every variant. The exact-match identity contract is pre-existing (deletesPerFile[task.File.FilePath()]); this PR doesn't tighten it.

Field-ID fallback matrix — 10 variants, 2 row groups arranged so a bogus stats mapping loses a delete observably. The fallback is conservative in every missing-ID case: it disables pruning, never filters on a wrong field ID, never loses a delete. The head commit's fix is real, not inert — ids only on extra column and file_path canonical / pos none / extra id both read [3 4] on head.

Pruning is real and exact. 1,000 paths × 64 rows, one row group each: targets=1 → 1/1000 row groups kept, 64/64000 rows decoded; 10 → 10/1000; 100 → 100/1000; 200 → 200/1000; 201 → tester nil, all 1000 read.

Conservative on missing stats. WithStats(false) → filtered == unfiltered. Long paths up to 8192 bytes → no delete lost (arrow-go's ApplyStatSizeLimits drops oversized min/max rather than prefix-truncating, so there's no unsound-bound hazard). containsNullsOnly returns false on a missing valueCounts entry.

Multi-data-file (common v2) shape. One delete file covering 5 data files plus a 6th absent path; scanned subsets {0}, {0,4}, {1,2,3}, {0..4} → filtered == unfiltered in all four. Split tasks sharing a delete file → filtered == unfiltered. Cap boundary {1,2,199,200,201,260} → filtered == unfiltered at every n.

Build/vet/gofmt clean; golangci-lint run ./table/... → 0 issues; go test ./table/ -count=1 ok; -race -count=5 on the relevant tests ok. All 15/15 checks green on head 361bccea — the go1.25.9 race flake (#1793) cleared post-rebase, and mergeStateStatus: BLOCKED is branch protection, not a conflict.

Major — please address before merge

1. table/arrow_scanner.go:800-803 — a non-canonical positive field ID now aborts a read that succeeded on base main.

This is the second half of @laskoviymishka's :789 comment ("the murkier one … worth deciding whether non-canonical positive IDs should also degrade rather than error. wdyt?"). The head commit fixed only the fieldID == nil branch; this one is unchanged and got no reply.

Measured: with file_path/pos carrying IDs 1 and 2, base main returns [3 4]; head returns invalid schema: position delete column "file_path" has field ID 1, want 2147483546 and the whole readAllDeleteFiles call fails — so the scan fails. Same for a non-canonical ID on file_path alone.

This contradicts the module's own stated posture at positionDeleteColumnIndices:834-835 — "resolved by their spec-defined names because Arrow schemas read from external files do not always retain Iceberg IDs." A renumbered ID is just as external as an absent one; it's safe to read by name, only unsafe to prune. Same shape as the fix already applied above it:

if *fieldID != want.ID {
    // A non-canonical positive ID is indistinguishable from a swapped
    // file here; name-based reading is still correct, pruning is not.
    pruningEnabled = false
    continue
}

Keep the hard error for the genuine collision case at :790-792 (canonical ID claimed by a different column) — that's the corrupt shape I reproduced last round and it should stay fatal. If you'd rather keep the hard error here too that's defensible, but please answer the question and put it in the description, because it converts a slow read into a failed scan.

2. The benchmark table doesn't reproduce, and the headline no-regression claim is wrong.

Same harness, benchstat, n=14, interleaved:

targets	claimed B/op · allocs	measured B/op · allocs
1	18.9 MB · 89k	17.87 MB · 81.46k
10	20.8 MB · 91k	19.96 MB · 83.17k
100	38.5 MB · 108k	37.81 MB · 99.55k
1,000	172.5 MB · 201k	172.55 MB · 201.13k ✓
all paths	172.5 MB · 201k	172.55 MB · 201.13k ✓

The two unfiltered rows match to 0.1% while all three filtered rows are systematically ~7.5k allocs high — a pattern platform difference alone doesn't explain, since it would shift the unfiltered rows too. I can't fully settle a darwin/arm64 figure from this box, so I report it as not reproduced rather than wrong. I did rule out staleness: the alloc/byte counts are identical across 88ca0f01, a1341157, ed834af0 and head.

What I can settle is the relative claim. The table reads 1,000 targets: 63.0 ms vs 61.1 ms baseline (+3%, i.e. "no regression"). Measured: +25.24%, p=0.008, n=14, with zero memory benefit (B/op p=0.285, allocs p=0.743). Please re-measure on current head and state the above-cap regression explicitly.

3. The 200→201 cliff is a step function and isn't documented. "Larger target sets still use row-level filtering and avoid changing the read semantics" is true about semantics but hides the cost: at 200 targets 200/1000 row groups are read; at 201, all 1000 are, plus ~1.02 M extra map lookups in appendFilePathChunk:497-501 for zero pruning benefit. A broad MOR scan — or a rewrite via Transaction.makePositionDeleteRecordsForFilter (transaction.go:3142) — that touches every data file a delete file covers lands squarely in that regime.

To be clear, the 200 cap itself is fine and needs no measurement: inPredicateLimit is long pre-existing (table/evaluators.go:37, from #123) and is the same cap inclusiveMetricsEval.VisitIn and bloomPredicateCollector.VisitIn already apply. Reusing it is exactly right. Just record in the description that above the cap this is a net time regression bounded at ~25% of the delete-read phase, so it's a decision rather than a surprise.

4. table/arrow_scanner.go:497 vs :705 — two different emptiness guards; an empty non-nil map silently drops every delete. newPositionDeleteRowGroupTester guards len(targets) == 0; the row filter guards a.targets != nil. Probe: readDeletesForPaths(ctx, fs, delFile, nil) → [1 3]; with map[string]struct{}{} → 0 entries, no error. The failure mode is silent resurrection of every deleted row.

Latent, not live — in readAllDeleteFiles:96-111 the map is created and either populated or nil'd in the same iteration, so an empty non-nil map can't escape today. But that invariant is unstated and unasserted, and readDeletesForPaths is exactly the helper a future caller reaches for. One-line fix that also makes the two guards agree: if len(a.targets) > 0 { if _, ok := a.targets[path]; !ok { continue } }.

Minor

• :639-641 — readDeletes was the production entry point on base main; it's now a test-only shim with zero production callers and 15 test callsites. So the tests that used to exercise production now exercise a wrapper, while the real path (non-nil targets) is what needs coverage. Inline nil at the callsites and delete the shim, or add a // test-only note.
• arrow_scanner_posdelete_regression_test.go:1607 — TestReadDeletesProjectsLeafColumnsAroundNestedRow was switched to readDeletesForPaths(..., {dataPath}) with the assertion unchanged. Net coverage is fine (arrow_scanner_nested_delete_test.go:62 covers the unfiltered nested read), but the name no longer matches what it calls.
• My "exactness contract isn't pinned" note from last round is still open — every new test uses exact path strings. One case asserting s3a:// or a file:// prefix is not treated as equivalent would lock in the load-bearing semantics. I confirmed the behaviour on both head and base; nothing in the tree pins it.
• positionDeletePruningEnabled inspects Arrow field IDs while inclusiveMetricsEval.TestRowGroup (evaluators.go:835) and buildFieldIDToColIdx (parquet_files.go:2025) key off Parquet physical node IDs. They agree for pqarrow-read files, which is why the guard works — worth a sentence at :766-769, since the guard's soundness depends on it.

Prior items

Mine (CHANGES_REQUESTED 2026-08-28): validate the physical field-ID mapping before enabling the tester → Fixed. The swapped-canonical-ID case that previously exposed a deleted row now returns ErrInvalidSchema instead of an empty map, and no missing-ID shape filters on a wrong field ID.

Mine (COMMENTED 2026-09-01): rebase → Fixed (now on 87789102, MERGEABLE). Failing race check → N.A., all 15 green. Path matching → confirmed independently, no finding. Exactness test → still open.

@laskoviymishka (CHANGES_REQUESTED 08-30, APPROVED 08-31): degrade rather than error when file_path/pos lack canonical IDs → Partially fixed (the nil branch degrades; the non-canonical branch still errors — Major 1). Mixed-task nil-fallback e2e test → Fixed, and not tautological: swapping the checks at :103/:106 drops data-B. >200 cap boundary → Fixed, {199,200,201} covered. appendRecord column-order contract → Fixed (:541). Bloom-filter pruning test → Fixed, and genuine — it isolates Bloom by first asserting StatsFn prunes neither group. Dropped FindFieldByName errors → Fixed. Bench baseline hoisted out of the loop → Fixed. t.Helper() → Fixed.

Description

• Over-claims work already on main. "Read only file_path and pos from position-delete Parquet files" landed in #1898 (e9a0d3bf) — positionDeleteProjectionIndices, posDeleteAccumulator and the projected GetRecords call are all pre-existing on the merge base.
• Mislabels a new behaviour as existing. "Keep the existing whole-file fallback when a task has no usable data-file path" — that nil-sentinel fallback is new in this PR (:101-110); base main had no filter to fall back from.
• Omits the change that can fail scans. Nothing mentions positionDeletePruningEnabled or that it returns ErrInvalidSchema for delete files that read fine on base main. That's the most consequential non-perf change in the diff and it's invisible from the description.
• Minor: "Push a bounded file_path IN (...) predicate" — the single-target case (:722-726) uses EqualTo, not IN.

---

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.

[zeroshade]

All nine prior review threads are genuinely fixed and each fix is pinned by a non-vacuous test that I confirmed goes red under targeted mutation; only two minor, non-blocking observations remain.

Re-review verification: 9 of 9 prior findings confirmed fixed at ab57a44 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).

Verification performed

go build ./table/... (OK); go vet ./table (clean); go test ./table -count=1 (ok, 6.730s); go test -race ./table -run 'TestReadDeletes|TestReadAllDeleteFiles|TestPositionDelete|TestGroupPosDeletes|TestPosDelete' -count=1 (ok, 2.475s). Mutations, each reverted via git checkout: (1) removed the row-level target filter -> 15 subtests red incl. AtPredicateLimit/201; (2) cap '>' -> '>=' -> /200 red; (3) cap '>' -> '>limit+1' -> /201 red; (4) removed nested collectIDs recursion -> RejectsNestedBloomFieldIDCollision + RejectsNestedDuplicateFieldIDs (5 subtests) red; (5) removed BloomPreds -> UsesFilePathBloomFilters + ValidatesPhysicalFieldIDs red; (6) forced fieldID==nil to hard-error -> FallsBackForPartialFieldIDs (3) + ValidatesPartialFieldIDs (2) red. Throwaway probe table/pr1937_probe_test.go written, run, and deleted.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The maintainer approving this PR 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 Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

minor — Filtering also drops deletes from a delete file not assigned to the task — a semantic change, not just perf

The target set is built only from tasks that reference a given delete file, so rows in that file addressing a data file whose task did not list it are now discarded. Previously the whole file was read and those rows landed in deletesPerFile, where the other task's lookup would apply them. I believe the new behaviour is the spec-correct one (delete-to-data assignment is the planner's job, and applying an unassigned delete is over-deletion), and I confirmed no under-deletion is possible because targets always contains t.File.FilePath() for every task that references the file. Still, the PR is framed as pure perf with an explicit 'keeps the existing result shape' claim, and this changes observable output. Worth a sentence in the PR body and a regression test pinning the intended semantics.

[zeroshade]

minor — Field-ID validation is skipped by the early return, so the same corrupt file errors or reads depending on query shape

newPositionDeleteRowGroupTester returns (nil, nil) before calling positionDeletePruningEnabled whenever len(targets)==0 or len(targets)>inPredicateLimit. A delete file with swapped or duplicated reserved field IDs therefore fails with ErrInvalidSchema when a query touches 1..200 data files, but reads successfully when it touches >200, or when any task lacks a usable path and the nil whole-file fallback kicks in. Neither path is unsafe (validation only gates pushdown, and the unvalidated paths do no pruning), so this is a consistency/support concern rather than a correctness one — but a scan that fails only for some query shapes is hard to diagnose. Consider validating unconditionally, or documenting that validation is deliberately scoped to the pushdown path.