Skip to content

Fix: AddFiles() should respect commit.manifest-merge.enabled#806

Open
RSP22 wants to merge 3 commits intoapache:mainfrom
RSP22:fix/addfiles-respects-manifest-merge-enabled
Open

Fix: AddFiles() should respect commit.manifest-merge.enabled#806
RSP22 wants to merge 3 commits intoapache:mainfrom
RSP22:fix/addfiles-respects-manifest-merge-enabled

Conversation

@RSP22
Copy link

@RSP22 RSP22 commented Mar 22, 2026

Summary

Closes #805

AddFiles() hardcodes .fastAppend() at line 780, bypassing appendSnapshotProducer()
which reads commit.manifest-merge.enabled. Setting this property has no effect when
using AddFiles() — manifests accumulate one-per-commit indefinitely regardless of the
property value.


Root Cause

appendSnapshotProducer() already exists at line 126 and correctly reads the property.
It is used by AddDataFiles() and Append() but was missed in AddFiles():

// transaction.go line 780 — BEFORE (hardcoded, ignores property):
updater := t.updateSnapshot(fs, snapshotProps, OpAppend).fastAppend()

// AFTER (same as AddDataFiles() and Append()):
updater := t.appendSnapshotProducer(fs, snapshotProps)
Method Uses appendSnapshotProducer()
AddDataFiles() ✅ yes
Append() ✅ yes
AddFiles() ❌ hardcoded .fastAppend()this bug

Java Reference

BaseTable.newAppend() always returns MergeAppend. MANIFEST_MERGE_ENABLED_DEFAULT = true
in the Java implementation. The Go implementation should behave consistently with the
reference implementation.


Test Gap

Three existing tests set commit.manifest-merge.enabled=true but none call AddFiles():

Test File Uses AddFiles()?
TestCommitV3RowLineageDeltaIncludesExistingRows snapshot_producers_test.go ❌ calls newMergeAppendFilesProducer() directly
TestMergeManifests table_test.go ❌ uses AppendTable()Append()
TestSetProperties transaction_test.go ❌ only sets the property, never appends

The new regression test table/addfiles_merge_regression_test.go exercises AddFiles()
directly with commit.manifest-merge.enabled=true and verifies the merge fires.
It can be moved into transaction_test.go if preferred by reviewers.


Changes

  • table/transaction.go — 1-line fix: use appendSnapshotProducer() instead of hardcoded .fastAppend()
  • table/addfiles_merge_regression_test.go — regression test proving the bug (fails before fix, passes after)

Verification

Regression test:

  • 3 sequential AddFiles() commits with minCountToMerge=2
  • Before fix: 3 manifests (merge never fires)
  • After fix: 1 merged manifest (merge fires on commit 3)

Production verification on a table with 1 commit/min:

  • Before fix: HEAD snapshot accumulated 1121 manifests after 18 hours (one per commit, never merged)
  • After fix: HEAD snapshot has 16 manifests after the next commit (merge fired)

Note on AI Assistance

This PR was developed with AI assistance (Claude). The author has verified the
implementation, Java reference behavior, test coverage gap, and production impact end-to-end.

@RSP22
Copy link
Author

RSP22 commented Mar 22, 2026

Added table/addfiles_merge_regression_test.go as a standalone file rather than
adding to transaction_test.go for the following reasons:

  1. Three existing tests already set commit.manifest-merge.enabled=true but none
    call AddFiles()
    — the gap is specific to AddFiles() and a separate file makes
    that immediately visible to anyone searching for test coverage.

  2. Regression tests are easier to find when named after the bug
    addfiles_merge_regression_test.go signals its purpose: prevent this specific
    regression from recurring. Contributors can locate it without reading
    transaction_test.go in full.

Happy to move it into transaction_test.go if preferred — the test itself
is unchanged either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: AddFiles() ignores commit.manifest-merge.enabled — hardcodes fastAppend()

1 participant