feat(table): add delete-file-backed changelog task types by dgvj-work · Pull Request #1897 · apache/iceberg-go

dgvj-work · 2026-08-27T04:57:32Z

Summary

First slice for delete-file-backed incremental changelog scans.

This adds the Java-shaped task types that later planning and readers need:

AddedRowsScanTask for inserts from added data files, including same-snapshot deletes
DeletedDataFileScanTask for removed data files, with existing deletes applied first
DeletedRowsScanTask for row-level deletes, keeping added vs existing delete files separate

Planning, readers, deletion vectors, and end-to-end scans are left for follow-up. File-level changelog scanning remains in #1883.

Related to #1884

Test plan

go test ./table -count=1 -run 'TestAddedRowsScanTask|TestDeletedDataFileScanTask|TestDeletedRowsScanTask'

Introduce AddedRowsScanTask, DeletedDataFileScanTask, and DeletedRowsScanTask so later changelog planning can distinguish row inserts from file removal and added versus existing deletes. Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>

Store position deletes, equality deletes, and deletion vectors on the matching FileScanTask fields instead of collapsing every delete into positional DeleteFiles. Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>

laskoviymishka

Nice start: the three-task taxonomy mirrors Java's changelog scan tasks cleanly, and landing the types before planning/readers is the right way to slice this. The doc comments carry a lot of the intent, which I appreciated.

I'd hold this before merging though. CI won't go green as-is (two nlreturn violations), and there's one modeling call I'd really like to settle before it calcifies into the follow-ups.

The main one is addedDeletes being a full FileScanTask. Java keeps it as a plain list of delete files, and here it buys us a second .File pointing at the same data file that nothing reads, plus a FileScanTask whose Start/Length/FirstRowID/DataSequenceNumber are all zero/nil. That's the part that worries me: when the reader slice lands and consumes this, the zeroed range and nil lineage fields will read as "intentionally absent" and silently suppress row-lineage synthesis, so the changelog output looks fine but is wrong. A small internal struct holding the classified lists makes that failure impossible to express, and it's much cheaper to change now than after readers depend on the shape.

Things I'd want to settle before merge:

the two nlreturn fixes (CI)
classifyDeleteFiles silently dropping anything that isn't a DV/eq/pos file; I'd return an error like scanner.go does rather than swallow it
the addedDeletes shape above
whether we define the ChangelogScanTask interface + Operation() now (Java has it) or defer to the planning PR (happy either way, just want it to be a decision)

The rest (the typed-nil guard, the redundant test assertion, thin classify coverage) are inline and minor. None of the deferred scope (planning, readers, DVs, e2e) is a concern for this PR; the taxonomy is the right foundation. Fix the CI and the addedDeletes shape and I'm happy to take another pass.

laskoviymishka · 2026-08-27T18:32:20Z

+// Matching delete files committed in the same snapshot, or from squashed
+// snapshots, are applied while reading so deleted rows are not emitted as
+// inserts.
+type AddedRowsScanTask struct {


Design-direction question for this first slice: Java has these three implement a common ChangelogScanTask interface with operation(), changeOrdinal(), commitSnapshotId(). Here each type carries ChangeOrdinal/CommitSnapshotID but there's no shared interface and no Operation().

Without it the planning follow-up can't return a uniform []ChangelogScanTask and every consumer needs a type switch to tell inserts from deletes. I'd lean toward defining the interface plus a ChangelogOperation enum now so the follow-ups have something to build against, but if you'd rather defer until planning lands that's reasonable too. wdyt?

Good call — I added ChangelogScanTask with Operation(), ChangeOrdinal(), and CommitSnapshotID(), plus a ChangelogOperation enum matching Java (INSERT / DELETE / UPDATE_BEFORE / UPDATE_AFTER). The three task types implement it so the planning follow-up can return []ChangelogScanTask without a type switch just to tell inserts from deletes.

laskoviymishka · 2026-08-27T18:32:20Z

+// those rows must not be emitted again.
+type DeletedRowsScanTask struct {
+	FileScanTask
+	addedDeletes     FileScanTask


I'd hold off on modeling addedDeletes as a FileScanTask. Java keeps it as a plain List<DeleteFile>, and here we get a second .File pointing at the same data file that no method ever reads, plus a whole FileScanTask whose Start/Length/FirstRowID/DataSequenceNumber are all zero/nil.

That last part is the real trap: once the reader slice lands and passes this into the read path, the zeroed range and nil FirstRowID/DataSequenceNumber will read as "intentionally absent" and silently suppress row-lineage synthesis (arrow_scanner gates on those being non-nil), so the output looks valid but is wrong.

I'd store the classified lists directly, a small internal struct like classifiedDeletes{pos, eq, dv []iceberg.DataFile}, and have AddedDeletes() flatten it. That makes the incomplete-metadata state impossible to express. wdyt?

Agreed, thanks for catching that. addedDeletes is now a small internal classifiedDeletes struct holding the pos/eq/dv lists, and AddedDeletes() just flattens it. That way we never carry a second FileScanTask whose range and lineage fields would read as intentionally absent.

laskoviymishka · 2026-08-27T18:32:20Z

+
+func classifyDeleteFiles(files []iceberg.DataFile) (pos, eq, dv []iceberg.DataFile) {
+	for _, f := range files {
+		if f == nil {


Minor, but this nil guard is a little misleading: f is an interface, so a typed-nil (a (*dataFile)(nil) appended to the slice) passes f == nil and then panics on IsDeletionVector's FileFormat() call right below. Real callers get DataFiles from Build() or manifest entries, neither of which produces a typed-nil, so I'd just drop the guard and note that nil elements aren't a supported input rather than half-guarding against a case that can't happen.

Dropped the nil guard. Real callers get files from Build() or manifest entries, so a half-working interface-nil check wasn't worth keeping.

laskoviymishka · 2026-08-27T18:32:20Z

+		if f == nil {
+			continue
+		}
+		switch {


This switch has no default, so any file that isn't a DV, eq-delete, or pos-delete (a plain data file, ContentType 0) is silently dropped. scanner.go classifies into these same three buckets but returns a wrapped ErrInvalidMetadata on unknown content (scanner.go:776-791). I'd match that here rather than swallow it, since once the planner is feeding this, a misrouted data file would produce a silently-wrong changelog instead of a loud failure.

Given scanner.go already does this exact three-way split with the error branch, it's worth extracting one shared helper so the two don't drift. Returning an error means threading it through fileScanTaskWithDeletes and the constructors, but I think that's the right trade.

Makes sense. I extracted a shared classifyDataFile helper and wired it through both manifestEntries.merge and the changelog constructors. Unknown content — including a plain data file in the deletes slice — now returns ErrInvalidMetadata instead of being dropped.

laskoviymishka · 2026-08-27T18:32:20Z

+			pos = append(pos, f)
+		}
+	}
+	return pos, eq, dv


nlreturn wants a blank line before this return, and the same before return out in allDeleteFiles just below. CI will fail on both until they're added. Quick fix, but it's the thing currently keeping the build red.

Fixed — the returns now have the blank line nlreturn wants. Thanks for flagging it.

laskoviymishka · 2026-08-27T18:32:20Z

+	return b.Build()
+}
+
+func TestAddedRowsScanTaskAppliesSameSnapshotDeletes(t *testing.T) {


These three tests only exercise the happy path: every input is a cleanly-typed delete file. There's no case for the branch that matters most, a plain data file (or nil) in the deletes slice, which today is silently dropped. If that path becomes an error (per the classifyDeleteFiles comment), I'd want a test asserting the error; if it stays a skip, a test that documents the intent.

Added TestClassifyDeleteFiles for the pos/eq/dv split and for a data file in the deletes slice, which now errors with ErrInvalidMetadata.

laskoviymishka · 2026-08-27T18:32:21Z

+	require.Equal(t, []iceberg.DataFile{existing}, task.ExistingDeletes())
+	require.Equal(t, existing.FilePath(), task.DeleteFiles[0].FilePath())
+	require.Empty(t, task.EqualityDeleteFiles)
+	require.Equal(t, []iceberg.DataFile{added}, task.addedDeletes.EqualityDeleteFiles)


The assertion a few lines up already checks AddedDeletes() returns []{added}; this one reaches into the unexported addedDeletes.EqualityDeleteFiles to assert the same fact through internal layout. I'd drop it. If the goal is to prove eq-vs-pos classification, a dedicated classifyDeleteFiles unit test reads better and doesn't couple to the struct shape.

Removed it. AddedDeletes() already covers the public result, and the classify test now owns the eq-vs-pos split without reaching into the struct layout.

Add ChangelogScanTask and Operation(), store addedDeletes as classified lists instead of a FileScanTask, and error on unknown delete content. Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>

Keep changelog classifyDataFile next to the unlocked manifest classification from apache#1913. Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>

dgvj-work · 2026-08-29T02:33:01Z

@laskoviymishka conflicts with main are resolved. I don't have merge rights on the repo — could you merge this when CI is green?

dgvj-work requested a review from zeroshade as a code owner August 27, 2026 04:57

fix(table): split changelog deletes by kind

d20d366

Store position deletes, equality deletes, and deletion vectors on the matching FileScanTask fields instead of collapsing every delete into positional DeleteFiles. Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>

laskoviymishka requested changes Aug 27, 2026

View reviewed changes

fix(table): align changelog tasks with review feedback

89c1c77

Add ChangelogScanTask and Operation(), store addedDeletes as classified lists instead of a FileScanTask, and error on unknown delete content. Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>

laskoviymishka approved these changes Aug 28, 2026

View reviewed changes

Merge branch 'main' into feat/changelog-delete-file-tasks

e222a73

Keep changelog classifyDataFile next to the unlocked manifest classification from apache#1913. Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>

Uh oh!

Conversation

dgvj-work commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgvj-work commented Aug 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dgvj-work commented Aug 27, 2026 •

edited

Loading