feat(table): add incremental changelog scan

[fallintoplace]

Summary

This adds Table.NewIncrementalChangelogScan to the table package.

It returns ChangelogScanTask values for file-level changes between snapshots:

• INSERT tasks for added data files
• DELETE tasks for deleted data files
• change ordinal and commit snapshot ID for ordering and context
• the underlying FileScanTask through ChangelogScanTask.ScanTask

Public API

• Table.NewIncrementalChangelogScan
• IncrementalChangelogScan.FromSnapshotInclusive
• IncrementalChangelogScan.FromSnapshotExclusive
• IncrementalChangelogScan.ToSnapshot
• IncrementalChangelogScan.PlanFiles
• ChangelogScanTask.ScanTask

PlanFiles returns tasks ordered by change ordinal, then DELETE before INSERT within an ordinal, and finally by data-file path.

What changed

• Added changelog planning for one snapshot ancestry range.
• Shared ancestry boundary handling with incremental append scans.
• Shared manifest-entry streaming with the regular scanner.
• Added scanner options to discard existing entries while retaining deleted entries for changelog planning.
• Preserved residual filters, data sequence numbers, and V3 row-lineage metadata.
• Added deterministic task ordering and scan-report metrics.

Scope

This is intentionally file-level:

• skips replace snapshots
• keeps existing manifest entries out of the result
• supports local planning and falls back from auto planning
• rejects a range when any selected snapshot's manifest list references a delete manifest, including delete manifests carried forward from earlier snapshots
• rejects a starting snapshot that is not an ancestor of the ending snapshot
• keeps unknown future snapshot operations, matching Java changelog scan behavior; missing operations still fail
• retains legacy V1 manifests with unknown counts instead of treating them as empty

Row-level delete changes, equality deletes, position deletes, and deletion vectors are left for follow-up work.

Tests

• added coverage for carried-forward delete manifests
• added coverage for a live divergent branch
• added coverage for unknown snapshot operations and explicit task ordering
• added coverage for cancelled scans that must not open manifest lists
• added coverage for the zero-value planner and public changelog task access
• go test ./table -count=1
• go test -race ./table -count=1
• go test ./... -run '^$' -count=1
• go vet ./...
• golangci-lint v2.12.2 in CI

[tanmayrauth]

Mechanics look correct and match the Java BaseIncrementalChangelogScan reference — delete-manifest rejection scope, change-ordinal numbering over non-replace snapshots, and replace-snapshot skipping all line up. Just a test gap and two wording nits, details inline.

[tanmayrauth]

This only covers a delete manifest owned by the single in-range snapshot. It doesn't cover the case that actually depends on the check ordering in incremental_changelog_scan.go:189 — a delete manifest created by an out-of-range snapshot and carried forward into an otherwise pure-append range. I
confirmed that case currently errors (correct — it matches Java's BaseIncrementalChangelogScan:103-118, where snapshot.deleteManifests() returns carried-forward manifests too), but nothing pins it: a future refactor that moves the changelogSnapshotIDs[manifest.SnapshotID()] range filter above the content check would silently start accepting those ranges and diverge from Java, with no test failing. Worth adding a table with snapshots [append S1, MoR-delete S2, append S3] and asserting a scan over (S2, S3] fails with ErrInvalidOperation / "do not support delete manifests".

[tanmayrauth]

The message interpolates snapshot.SnapshotID, i.e. the snapshot whose manifest list is being iterated — not the snapshot that created the delete manifest. In the carried-forward case (delete manifest from an older out-of-range snapshot, appearing in a later append snapshot's list) this names a pure-append
snapshot as the offender, so someone debugging why their append-only range was rejected is pointed at the wrong commit. Either reference manifest.SnapshotID() (the origin), or reword to "snapshot %d references a delete manifest" so it's clear it's about the manifest list, not an operation that snapshot
performed.

[tanmayrauth]

"not supported" reads like delete-file-driven changes are silently omitted, but PlanFiles actually hard-errors whenever an in-range snapshot's manifest list references a delete manifest — including delete manifests carried forward from earlier snapshots. That means a pure-append range on any table that has
ever done a MoR delete (and still has live delete files) fails rather than returning the appends. That's the intended parity behavior, but the doc should set the expectation: e.g. "PlanFiles returns an error if any in-range snapshot's manifest list references a delete manifest, including ones carried forward from
earlier snapshots."

[zeroshade]

LGTM but I'll wait for @tanmayrauth to resolve the threads and confirm his requested changes are addressed before merging.

[laskoviymishka]

The mechanics here are solid. Ordinal assignment over non-replace snapshots, replace-snapshot skipping, the eager delete-manifest rejection, and the v3 row-lineage propagation all line up with Java's BaseIncrementalChangelogScan. tanmayrauth already confirmed that and zeroshade's given it a conditional LGTM, so this is close.

One thing I'd want nailed down before it lands: the change-operation sort only produces deletes-before-inserts within an ordinal because "delete" happens to sort before "insert" lexically. That's a real replay contract riding on an accidental property of two string literals, so renaming the constant would silently flip it. I'd make the ordering explicit (a tiny comparator, or at least a comment) so the intent lives in the code and not in a coincidence.

A couple of smaller things I've left inline. The changelogSnapshots default rejects unknown operations where Java only skips REPLACE, so a future spec op would error here but not there, worth making a deliberate choice. The TotalDataManifests metric also diverges from a regular scan because the no-change pre-filter runs before the counter, which I'd just document. tanmayrauth's already got the test-gap thread open, and asserting the manifest count there would pin that behavior down, so I'd fold it into his thread rather than open a new one.

Fix the sort ordering and I'm happy to approve. The rest are non-blocking.

[laskoviymishka]

manifest.SnapshotID() here is the snapshot that wrote the delete manifest, not the one we're iterating. With FromSnapshotExclusive(2) this prints "from snapshot 2" even though 2 is the excluded start, outside the caller's range (the test encodes exactly that), so it reads as if 2 were in range.

Small thing: I'd either drop the ID the way Java does, or rephrase to something like "carried into the scan range from snapshot %d" so it's clear the named snapshot is the source, not a member. wdyt?

[laskoviymishka]

This pre-filter runs before filterManifestsWithSchema increments totalDataManifests, so the ScanReport for a changelog scan counts fewer manifests than a regular scan over the same endpoint (the test asserts 3; a normal scan would see more). That's a defensible choice, but an operator comparing the two reports would read it as an efficiency difference that isn't real.

I'd add a comment at the filter site noting totalDataManifests intentionally excludes no-change manifests and diverges from Java's count. tanmayrauth's already got a thread open on SkipsManifestsWithoutChanges not pinning this down; asserting the manifest count there closes both, so I'd fold it into his thread rather than duplicate it.

[laskoviymishka]

These two bools differ only by the middle word and are positional, so a future caller could transpose them and silently start including DELETED entries. This site is correct, but nothing guards it.

At minimum I'd add named-arg comments here (/*discardDeleted=*/ false, /*discardExisting=*/ true). If you'd rather, folding them into a small manifestReadOptions struct on openManifestWithOptions removes the footgun entirely. Non-blocking.

[laskoviymishka]

This ordering only holds because "delete" sorts before "insert" lexically. Deletes-before-inserts within a change ordinal is a real replay contract, but nothing here states it. If the string value of ChangelogOperationDelete ever changed to something that sorts after "insert", this would silently flip and we'd emit inserts before deletes in the same commit window.

I'd make it explicit instead of leaning on the string values: a small changelogOperationOrder(op) that returns 0 for delete and 1 for insert, then compare those. A test catches a regression, but the intent should live in the code. wdyt?

[laskoviymishka]

Java's orderedChangelogSnapshots only skips REPLACE and includes every other operation. Here the explicit default rejects anything outside {append, overwrite, delete}. If a future spec revision adds a new snapshot operation (the way "delete" was once added), we'd error out on tables that Java and PyIceberg still read.

Not a blocker since it's correct for today's spec, but I'd lean toward matching Java (default: continue, with a comment), or keep it strict and add a note that this deliberately assumes the current operation set. Either is fine, I'd just want it to be a choice. wdyt?

[zeroshade]

First, a correction to the record: my 2026-08-27 approval was submitted against 4d44cdc3, which has since been force-pushed away. It endorsed a tree that no longer exists in this branch's history, so I'm withdrawing it as stale rather than letting it stand as current sign-off. Please don't treat that earlier green check as approval of the current head.

There is also a standing CHANGES_REQUESTED from @laskoviymishka (2026-08-31) and three conditional comments from @tanmayrauth that my original approval was explicitly waiting on — I can find no reply or resolution event for any of them. I'm not approving around those.

Major — add a valid non-ancestor regression test (table/incremental_changelog_scan_test.go:284-301, guard at table/incremental_scan.go:49-67)

The ancestry helper correctly rejects a live starting snapshot that isn't on the ending snapshot's lineage, but the tests only cover an unknown start ID (:284) and an expired exclusive parent (:303). The genuine divergent-branch case — a valid, live snapshot on a sibling branch — is never constructed, so the core error path is unpinned.

The current guard does the right thing today; this is a regression-coverage gap, not a claim that the behaviour is wrong. But an ancestry refactor could start silently returning changelog rows across diverged branches and still pass CI, and "silently wrong rows" is exactly the failure mode worth pinning.

Please add a fixture with a root S1 and two sibling children (say S2 and S3), then assert that planning from S3 to S2 returns ErrInvalidArgument with no tasks, covering the inclusive path and the applicable exclusive variant, and that the error states the start is not an ancestor.

Verified as correct

• Non-ancestor rejection — table/incremental_scan.go:49-67.
• Carried-forward delete-manifest rejection before the manifest-origin range filter — table/incremental_changelog_scan.go:173-175, matching Java's BaseIncrementalChangelogScan.
• REPLACE snapshots skipped; ordinary overwrite and delete data-manifest entries included.
• Row-level delete handling explicitly out of scope, which is a reasonable boundary for this PR.

The code changes for @tanmayrauth's three points do appear to be in place (carried-forward coverage, manifest-origin reporting, hard-error documentation) — please reply on those threads so they can be closed out, and address @laskoviymishka's five open points, particularly the explicit operation-ordering decision.

CI is green (15 checks). Once the divergent-lineage test lands and those threads are resolved, I'm happy to take another look.

---

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]

The divergent-branch regression test I asked for is in and mutation-verified, and the mechanics now match Java's BaseIncrementalChangelogScan closely. Two things I'd like fixed before this lands: the one behaviour that justified the shared-scanner.go refactor is untested end to end, and the new public return type can't be consumed outside package table.

Blocking

1. table/incremental_changelog_scan.go:217 — discardExisting is not pinned by any test, and getting it wrong hard-errors on ordinary manifest-merged tables.

Flipping /* discardExisting= */ true to false — removing the Go equivalent of Java's ManifestGroup.ignoreExisting() — leaves the entire changelog suite green: ok github.com/apache/iceberg-go/table 0.656s, 24 in-package tests plus the external row-lineage test.

TestOpenManifestWithOptionsCanDiscardExistingEntries (:70-114) exercises the helper in isolation, which makes the behaviour look covered while the production call site is unverified. And TestIncrementalChangelogScanPreservesChangesAcrossManifestRewrite (:156) reads like it covers this but doesn't — its rewrite manifest is owned by a REPLACE snapshot, so it's dropped at the manifest level and its EXISTING entries are never read. It passes under the mutation too.

The only shape that exercises this is a manifest owned by an in-range snapshot carrying an EXISTING entry whose adding snapshot is also in range — exactly what Iceberg manifest merging produces (commit.manifest-merge.enabled defaults true in Java writers). No fixture builds it. I built it (S1 appends A → manifest-1; S2 appends B → merged manifest [EXISTING(A, sn=1), ADDED(B, sn=2)]):

# current head
task[0] .../data-a.parquet op=INSERT ordinal=0 commit=1
task[1] .../data-b.parquet op=INSERT ordinal=1 commit=2   → correct

# with discardExisting=false
incremental changelog scan snapshot 1: invalid metadata: unknown manifest entry status 0

So the failure mode isn't a silently-wrong row — it's a hard ErrInvalidMetadata on every merged-manifest table, and nothing in CI would catch a refactor that reintroduced it. This is also the sole justification for the three new scanner.go entry points: the +61 lines of shared plumbing are load-bearing but unguarded.

Fix: add a fixture with an in-range snapshot whose manifest carries an EXISTING entry from another in-range snapshot, and assert exactly the two insert tasks. That single test turns the mutation red.

2. incremental_changelog_scan.go:80,:324 + changelog_scan_task.go:37 — the new public return type is unusable outside package table, and the interface is unsealed.

PlanFiles returns []ChangelogScanTask, which exposes only Operation(), ChangeOrdinal(), CommitSnapshotID() — no file, path, size, or residual. To do anything with a task a caller must write a 3-arm type switch over AddedRowsScanTask / DeletedDataFileScanTask / DeletedRowsScanTask and invent their own default case. The library already needs exactly that helper — changelogTaskFileScanTask (:324-335) — and keeps it unexported, panicking in its default arm. Your own external-package test proves the friction: incremental_changelog_row_lineage_test.go:64-66 has to do tasks[0].(table.AddedRowsScanTask) to reach .File.FilePath().

Compounding it: Scan.ReadTasks(ctx, tasks []FileScanTask) is the only reader in the library, and nothing converts changelog tasks into []FileScanTask. Unlike IncrementalAppendScan, whose doc points callers at Scan.ReadTasks, the changelog doc says nothing about reading tasks — because there's no exported path.

ChangelogScanTask also has no unexported method, so it isn't sealed: once released, no method can ever be added. Java gets away with the same three-method interface because AddedRowsScanTask extends ChangelogScanTask, ContentScanTask<DataFile> — a shared exported supertype yielding file() with one cast, not an exhaustive switch. Go has no equivalent here.

Still fixable: git ls-tree v0.6.0 table/ | grep -i increm is empty, and 67b7d86d (#1897, which defined ChangelogScanTask) is not in v0.6.0-rc0/rc1/rc2. Nothing in this surface has shipped. Either export the accessor (func ChangelogTaskFileScanTask(ChangelogScanTask) (FileScanTask, error), erroring rather than panicking) or add FileScanTask() FileScanTask plus a sealing isChangelogScanTask(), before the shape is permanent.

Major

3. :244-247 — the file-path tiebreak has 0% coverage and its guarantee is unpinned. Replacing the comparator's third clause with return 0 leaves everything green; the coverage profile confirms :244-247 never execute, because no fixture produces two tasks sharing both change ordinal and operation. That's the common case in practice (one snapshot appending several files). I verified current behaviour is correct — a snapshot adding data-z, data-m, data-a plans as a, m, z — but nothing defends it.

4. :115 — Residual diverges from a regular scan and from Java on identical input. bindTaskFilter puts the whole bound row filter on every task; Scan.PlanFiles (scanner.go:1372-1387) computes a per-partition simplified residual and nils it when not simplified. Measured on the same table and filter (identity(id) spec, id == 2):

REGULAR   .../data-b.parquet residual=AlwaysTrue()
CHANGELOG .../data-b.parquet residual=BoundEqual(term=BoundReference(field=1: id ...), literal=2)

Java's BaseIncrementalChangelogScan passes context.residuals() — the same ResidualEvaluator a regular Java scan uses — so Java's changelog tasks do get partition-simplified residuals. Correctness-safe (the full filter is conservative), but every reader re-evaluates a predicate the partition already guarantees. IncrementalAppendScan has the identical gap, so a shared follow-up is fine — but the description claims "preserves residual filters" and the residual it preserves isn't the one a regular scan produces. Make it an explicit documented decision.

5. :78-79 — the ordering contract still isn't in the exported doc. @laskoviymishka asked for the deletes-before-inserts replay contract to live in the code rather than in a string-sort coincidence. The internal comment at :258-260 does that, and mutation confirms the code is pinned — but PlanFiles' doc says only "returns one task for each added or deleted data-file entry and emits a ScanReport". The full guarantee (change ordinal, then deletes before inserts within an ordinal, then file path) is a new public behavioural contract with no Java counterpart — Java's planFiles() returns an unordered CloseableIterable. Callers can't rely on what's undocumented.

6. incremental_changelog_scan_test.go:62-68 — TestIncrementalChangelogScanSkipsManifestsWithoutChanges asserts nothing about its name. It's a byte-for-byte duplicate of ...PlansAddedAndDeletedEntries' setup with only require.Len(t, tasks, 4). The manifest-count assertion @laskoviymishka asked for landed in TestIncrementalChangelogScanEmitsScanReport instead (TotalDataManifests == 3), which is what actually pins the pre-filter. Either fold that assertion in here or delete the test — a test named for a behaviour it doesn't check is worse than none.

What checks out

11 mutations run against your test set; 7 are properly pinned (operation comparator, no-change manifest pre-filter, range-filter ordering vs the delete-manifest check, DELETED→INSERT mapping, unknown-operation handling, REPLACE skipping, both non-ancestor guards). Java parity confirmed by reading BaseIncrementalChangelogScan, BaseIncrementalScan, IncrementalScan, ChangelogScanTask, AddedRowsScanTask, DeletedDataFileScanTask on apache/iceberg main. Build/vet/gofmt clean, golangci-lint 0 issues, -race -count=5 clean, CI 15/15. Package coverage 86.0%.

Equality vs positional vs DV deletes are correctly not distinguished — all three live in ManifestContentDeletes manifests, and the range check at :171-174 rejects any of them before entry classification. Matches Java's blanket UnsupportedOperationException and is the right boundary for a file-level first cut.

On #1897: no overlap or conflict — it's already merged (67b7d86d) and is an ancestor of this PR's base, so changelog_scan_task.go is pre-existing and untouched here. This PR is its first consumer. One cross-PR consequence though: #1897 exported ChangelogScanTask unsealed and accessor-less, but nothing public returned it, so it was inert. This PR makes it a public return type, and neither has shipped in v0.6.0. This is the last window to fix the shape.

Minor

• :350-367 — changelogSnapshots has no doc comment. The default-append is a deliberate Java-parity choice but nothing records it, and it silently contradicts sibling appendOnlySnapshots (incremental_append_scan.go:265-284), whose doc promises ErrInvalidOperation for unrecognized operations. Also switch x { case OpReplace: continue; default: append } is plainly if x == OpReplace { continue }.
• :143-145 — TestIncrementalChangelogScanHonorsContextCancellation passes with the loop's ctx.Err() guard deleted, so it doesn't pin the only thing preventing a cancelled scan from reading every snapshot's manifest list.
• :120-133 — finish omits acc.applyResultDeleteMetrics(tasks), which the append-scan twin calls. Harmless today, will under-report once row-level deletes land.
• :51,:70 — FromSnapshotInclusive/ToSnapshot drop the "The snapshot is validated when files are planned" sentence their append-scan twins carry. That deferred validation is a real divergence from Java, which validates eagerly.
• :330-333 — a panic reachable from an unsealed exported interface should be an error return.
• IncrementalChangelogScan{} (zero value) panics at :81. Exported struct with unexported fields, so external code can construct it. Same as IncrementalAppendScan, so consistency argues for leaving it — but a one-line nil guard costs nothing.
• :253-255 — manifestHasChangelogEntries uses != 0, not > 0, and it's load-bearing and uncommented: v1 manifest-list entries with absent counts decode to -1, so != 0 correctly keeps unknown-count manifests (verified by v1 round-trip). Same applies to scanner.go:1026-1027. A note would stop someone "tidying" it to > 0 and silently dropping changes on legacy v1 tables.
• incremental_changelog_scan_test.go:481-488 — existingOnlyManifest is registered in snapshot 4's list but its .avro is never written to the mem FS; the fixture only works because the pre-filter removes it. A regular tbl.Scan(...) over it fails with file does not exist. That couples the fixture to an optimization and blocks changelog-vs-regular comparisons.
• Expired snapshot mid-range: incremental_scan.go:38 uses AncestorsOf, whose own doc warns it silently truncates and points at AncestorsOfChecked. A full-range changelog scan then returns a partial changelog with ordinals restarting at 0 and no error. Exact Java parity, so not a bug — but for a changelog the ordinals are the replay contract, so a doc note is cheap.
• 7f472ef3 doesn't compile. Cosmetic under squash-merge, but it breaks bisect on the branch.

Prior items

@tanmayrauth (3 threads): carried-forward delete manifest test → Fixed, and mutation-verified (hoisting the range filter above the content check — the exact refactor he named — fails it). Error naming the wrong snapshot → Fixed. Doc stating PlanFiles hard-errors including carried-forward → Fixed.

@laskoviymishka (5 threads): explicit operation ordering → Fixed in substance. Worth flagging: the first attempt (7f472ef3) returned INSERT=0, DELETE=1, which inverted the contract he was protecting; 37013a25 corrected it. Unknown-operation handling → Partially fixed — behaviour now matches Java, but he asked for a comment explaining it, and the resulting contradiction with appendOnlySnapshots is unexplained. TotalDataManifests divergence → Fixed and pinned. Positional bool footgun → Fixed with /* name= */ comments; he noted a struct would remove it entirely, and Blocking 1 is the direct consequence — the comment documents the argument but no test defends it. Error wording → Fixed.

Mine: divergent-branch regression test → Fixed, exactly as requested (asserts ErrInvalidArgument, the right message, and Nil(tasks) for both inclusive and exclusive paths), and mutation-verified. Replying on @tanmayrauth's threads so they can be closed → still open; no reply on any of the eight inline threads, and @laskoviymishka's CHANGES_REQUESTED still stands.

Description

Every affirmative claim checks out and all four listed validation commands reproduce green. Omissions:

1. No mention of table/scanner.go (+61/-4) — three new internal entry points plus a change to the manifest-skip predicate on the shared path every regular scan uses. It also changes openManifest's capacity math from max(0, added+existing) to per-term clamping, a behaviour change on the non-changelog hot path (v1 manifest with added=5, existing=-1: cap 4 → cap 5). Benign but undisclosed.
2. No mention of table/incremental_append_scan.go (-40 lines) — snapshotsBetween was gutted and re-pointed at the new shared incrementalSnapshotsBetween.
3. The task-ordering guarantee is absent — a new public behavioural contract with no Java counterpart, added during review.
4. The unknown-operation behaviour change is absent — the last two commits moved changelogSnapshots from rejecting unrecognized operations to accepting them, diverging from the sibling scan.
5. "rejects snapshots with delete manifests" understates the blast radius the way @tanmayrauth flagged: a pure-append range on any table that has ever done a MoR delete and still has live delete files fails. The code doc was fixed; the PR body wasn't.
6. New exported API isn't enumerated.

---

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]

Both prior blocking findings are genuinely fixed and mutation-verified, but the documented file-path tiebreak in PlanFiles' public ordering contract is still unpinned — mutating it to return 0 leaves the entire suite green.

Re-review verification: 19 of 21 prior findings confirmed fixed at 6f2a7f7 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust). Still open:

• not fixed — Major 3: the file-path tiebreak has 0% coverage and its guarantee is unpinned
• not fixed — Minor: commit 7f472ef doesn't compile, breaking bisect

Verification performed

go build ./... => PASS; go vet ./table/ => PASS; go test ./table/ -count=1 => ok 5.567s PASS; go test -race ./table/ -count=1 => ok 23.761s PASS; go test ./table/ -coverprofile -covermode=count => PASS (line :270 execcount=1). Mutations: discardExisting=true->false => FAIL (PreservesChangesAcrossManifestRewrite, 'unknown manifest entry status 0') PINNED; includeDeleted=true->false => FAIL (PreservesV3RowLineageMetadata, 2 tasks not 3) PINNED; discardDeleted=false->true => FAIL (same test) PINNED; ctx.Err() guard deleted => FAIL ('Should be zero, but was 3') PINNED; file-path tiebreak -> return 0 => ok 0.955s GREEN, NOT PINNED. Throwaway probe table/pr1883_probe_test.go (3 tests, since deleted; worktree clean at 6f2a7f7) => PASS at head, FAIL under the tiebreak mutation. gofmt -l over PR-touched files => clean (table/snapshot_producers.go is unformatted but is not touched by this PR). golangci-lint v2.12.2 crashed locally on go 1.26.5 (go/types panic in goanalysis, environmental); CI lint is green, gh pr checks 1883 => 15/15 pass.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The findings below are observations, not blockers; an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.

More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

major — Documented file-path tiebreak is still unpinned; mutation to return 0 leaves the suite green

PlanFiles' doc (:83-86) now promises tasks are ordered '...and finally by data-file path' — a new public behavioural contract with no Java counterpart. No fixture presents two tasks sharing both change ordinal and operation in non-sorted order, so the comparator's third clause never changes an outcome. Add a fixture where one in-range snapshot adds several files in reverse-sorted order within a single manifest (e.g. data-z, data-m, data-a) and assert the planned order is a, m, z. That single test turns the mutation red.

Evidence

Coverage -covermode=count: line 270 execcount=1 (263=29, 266=10) — executed but result unobservable, since the new b/z pair is already sorted in manifest order and n=4 uses stable insertion sort. Mutation `return cmp.Compare(left.file.File.FilePath(), ...)` -> `return 0`: `go test ./table/ -run Changelog -count=1` => ok 0.955s (GREEN). Throwaway probe adding z, m, a in one manifest: at head logs 'PROBE order: [data-a data-m data-z]' and PASSES; under the same mutation it FAILS with '- data-a.parquet / + data-z.parquet ... - data-z.parquet / + data-a.parquet'.

[zeroshade]

minor — finish rebuilds FileScanTasks it already has, behind an unreachable error path

plannedTasks (:241-260) already stores the FileScanTask alongside each ChangelogScanTask, but finish re-derives them via changelogTaskFileScanTask and threads an error return for it. That error is unreachable: the interface is sealed (isChangelogScanTask) and newChangelogScanTask returns concrete value types, so the task is never a nil interface — the same applies to :256-259. Pass plannedTasks into finish and drop the dead error path.

[zeroshade]

minor — Scan-report metrics count DELETE tasks as result data files, double-counting a file inserted then deleted in range

acc.resultDataFiles = len(tasks) and acc.totalFileSize sum over every task including DELETEs, so a data file added in S1 and removed in S2 contributes twice to ResultDataFiles and TotalFileSize. Java emits no ScanReport for changelog scans, so there is no parity reference; the semantics are a Go-specific choice. Document what these fields mean for a changelog scan, or count only INSERT tasks.

[zeroshade]

minor — No ScanReport is emitted on the no-snapshot early return, though PlanFiles' doc promises one

PlanFiles documents 'It emits a ScanReport through the configured reporter on successful planning' (:85-86), but the toSnapshot == nil path returns nil, nil without calling finish, so a successful empty plan produces no report. This exactly mirrors IncrementalAppendScan:108, so consistency argues for leaving the behavior and instead qualifying the doc.

[zeroshade]

nit — 5 of 11 commits on the branch do not compile

Previously 1 non-compiling commit was flagged; it is now 5. Four are a ChangelogOperation redeclaration against #1897's changelog_scan_task.go (a rebase artifact) and one is a stale filterManifestsWithSchema arity. Cosmetic under squash-merge but it breaks bisect on the branch; a rebase would clean it up.

[zeroshade]

A PR-added line trips the repo's enabled nlreturn linter at table/incremental_changelog_scan.go:278, failing all four Go CI jobs; the prior major ordering finding is genuinely fixed and mutation-verified.

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

• partially fixed — minor — finish rebuilds FileScanTasks it already has, behind an unreachable error path
• partially fixed — nit — 5 of 11 commits on the branch do not compile (ChangelogOperation redeclaration / stale arity)

Verification performed

In worktree .pi-worktrees/pr1883 at 855fd9c: `go build ./...` OK; `go vet ./table/ ./` OK; `go test ./table/ -run 'Changelog|changelog' -count=1` ok 0.687s; `go test -race ./table/ -count=1` ok 21.255s; `golangci-lint run -c <nlreturn-only> ./table/` -> 1 issue (the blocking finding). Four mutation probes run and reverted: (1) file-path tiebreak -> `return 0` => TestIncrementalChangelogScanPreservesChangesAndSortsByFilePath FAIL; (2) delete-manifest check moved below the snapshot-ownership filter => TestIncrementalChangelogScanRejectsCarriedForwardDeleteManifests FAIL; (3) changelogOperationOrder delete/insert ranks swapped => 4 FAIL incl. TestIncrementalChangelogScanPlansAddedAndDeletedEntries, TestIncrementalChangelogScanHonorsSnapshotBoundaries, TestIncrementalChangelogScanPreservesV3RowLineageMetadata; (4) inter-snapshot ctx.Err() check deleted => TestIncrementalChangelogScanChecksContextBetweenSnapshots FAIL. Final `git status --porcelain` empty.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. After you've addressed the points above and pushed an update, an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. 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 Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

blocking — PR-added line trips the repo's enabled nlreturn linter, failing all four Go CI jobs

return finish(plannedTasks), nil immediately follows the closing }) of the slices.SortFunc block with no blank line. nlreturn is enabled at .golangci.yml:28, so this fails the lint step on ubuntu/macos x go1.25.9/go1.26.1 — the sole cause of the red CI. Fix is to insert a blank line before the return, matching the style used everywhere else in this same file (e.g. :63, :74, :83, :152).

Evidence

CI (job 100728632052 ubuntu go1.25.9 and 100728632137 macos go1.25.9): '##[error]table/incremental_changelog_scan.go:278:2: return with no blank line before (nlreturn)' / '##[error]issues found'. Reproduced locally with golangci-lint 2.12.2 (same version CI pins) using a minimal nlreturn-only config: 'incremental_changelog_scan.go:278:2: return with no blank line before (nlreturn)\n\treturn finish(plannedTasks), nil\n\t^\n1 issues:\n* nlreturn: 1'. Still reproduces at clean head after all probes were reverted.

[zeroshade]

minor — changelogTaskFileScanTask is now dead production code reachable only from tests

The prior round moved finish() onto the stored plannedChangelogTask.file, which removed the only production caller of this helper. It now exists solely to serve two tests, and its error branch is unreachable in production: ChangelogScanTask is sealed by isChangelogScanTask() and every implementation returns a concrete FileScanTask from the public ScanTask() accessor, so the nil-task guard can only fire from a test passing an explicit nil. Either delete it and have the tests call task.ScanTask() directly, or move the helper into the test file.

[zeroshade]

nit — 3 of 13 branch commits still do not compile (ChangelogOperation redeclared)

Down from the 5 flagged last round but not resolved. Commits 0ab1321, f936818 and af32225 each carry two type ChangelogOperation string declarations in table/ (the rebase artifact against #1897's changelog_scan_task.go), so go build ./table/ fails at those points and git bisect is broken across the branch. Cosmetic under squash-merge; a rebase would clear it.