fix: use OpReplace instead of OpOverwrite in ReplaceDataFiles and ReplaceFiles

[Bahtya]

Summary

Fixes #841 (parent #832)

Changes ReplaceDataFiles, ReplaceDataFilesWithDataFiles, and ReplaceFiles to use OpReplace instead of OpOverwrite when creating snapshot updates.

Problem

Per the Iceberg spec, REPLACE is the correct operation when data content is equivalent but reorganized into different files (e.g., compaction). The three replace methods were unconditionally using OpOverwrite despite a TODO comment acknowledging this was incorrect.

Changes

• table/transaction.go: Changed OpOverwrite → OpReplace in three locations:
  • ReplaceDataFiles (line ~418)
  • ReplaceDataFilesWithDataFiles (line ~713)
  • ReplaceFiles (line ~826)
• Removed the TODO comment at ReplaceDataFiles that acknowledged the incorrect operation type
• table/replace_files_test.go: Updated TestReplaceFiles_DataAndDeleteFiles to assert OpReplace

Testing

All existing tests pass:

=== RUN   TestReplaceFiles_DataAndDeleteFiles
--- PASS
=== RUN   TestReplaceFiles_DelegatesToReplaceDataFilesWhenNoDeleteFiles
--- PASS
=== RUN   TestReplaceFiles_ValidationErrors
--- PASS

[laskoviymishka]

Thnx for contribution!

Looks good to me.

[Bahtya]

Hi team, just wanted to follow up on this PR. It has been reviewed and approved by @laskoviymishka, and all CI checks are passing. Would appreciate if a maintainer could take a look and merge when ready. Thank you!

[Bahtya]

@zeroshade Friendly ping 👋 @laskoviymishka has already approved this PR. It fixes a bug where OpOverwrite was used instead of OpReplace in ReplaceDataFileActions. Would appreciate your review for merge.

[zeroshade]

we can only use OpReplace here instead of OpOverwrite if there are no changes to the underlying data. We should probably validate this before we use OpReplace instead of OpOverwrite, right?

[zeroshade]

same as above, we should validate that there's no actual data changes before using OpReplace, right?

[zeroshade]

same as above

[Bahtya]

Hi @zeroshade, thanks for the review! I've updated the PR to validate data changes before using OpReplace in all three replace methods:

1. ReplaceDataFiles: Compares total record count of deleted files vs added files. Uses OpOverwrite when counts differ.
2. ReplaceDataFilesWithDataFiles: Same validation pattern.
3. ReplaceFiles: Accounts for removed delete file records: uses OpOverwrite when (deletedCount - removedDeleteCount) != addedCount.

The key insight: OpReplace is now only used when record counts match (metadata-only reorganization like compaction), and OpOverwrite is used when actual data content has changed. This aligns with the Iceberg spec where REPLACE is for operations that don't change data content.