feat(transaction): add RowDelta transaction action for row-level modi… by wirybeaver · Pull Request #2203 · apache/iceberg-rust

wirybeaver · 2026-03-03T10:07:19Z

Fixes #2202, part of epic #2201 and epic #2205

Background

Iceberg's RowDelta is the fundamental transaction action backing MERGE INTO, UPDATE, and DELETE SQL operations. Unlike FastAppend (which only adds new files), RowDelta can both add new data files and mark existing ones as deleted in a single atomic snapshot.

Iceberg supports two strategies for row-level mutations:

Copy-on-Write (CoW): The engine reads affected data files, applies the mutation in-memory, writes new data files, and atomically marks the originals as deleted. Readers always see clean, fully-materialized data files. Trade-off: high write amplification, zero read overhead.
Merge-on-Read (MoR): Instead of rewriting files, the engine appends small "delete files" (position deletes or equality deletes). Readers must merge data files with delete files at scan time. Trade-off: low write amplification, higher read overhead.

What this PR does

Implements RowDeltaAction in the transaction layer with Copy-on-Write support:

Method	Purpose
`add_data_files()`	Add new/rewritten data files (INSERT rows, or CoW rewritten files)
`remove_data_files()`	Mark existing data files as deleted (CoW mode)
`add_delete_files()`	Stubbed API for future Merge-on-Read support
`validate_from_snapshot()`	Optimistic concurrency: reject commit if table has advanced
`set_snapshot_properties()`	Attach custom key/value metadata to the snapshot summary

How the snapshot is built

All existing manifests are carried forward to the new snapshot, except manifests that contain any of the removed data files.
A new manifest is written for the added data files.
The snapshot operation type is determined by what changed:
- Append — only new files added, nothing removed
- Overwrite — data files removed (CoW update/delete), or both added and removed

What is intentionally deferred

add_delete_files() is API-only scaffolding. Full MoR support requires:

Writing a separate delete manifest (Avro file with content = Deletes)
Correct sequence number propagation so delete files only apply to data files written before them
Reader-side delete file merge at scan time

MoR solution seems to already being contributed by other folks. I am willing to make further contribution for MoR if necessary.

Java reference

jdockerty

This looks great, thanks!

The implementation refer to the official Iceberg Java implementation (RowDelta API).

I'm not sure whether you want to link to the specific commit of iceberg that you're referring to here as well. That way, anyone doing some digging around in PRs at a later date will know where this came from. Not a huge deal though 👍

The tests here are really useful to demonstrate this functionality as well.

The CI issues are for typos, but it is a false-positive because MOR is standing for Merge-On-Read. So you'll need to flag that as okay.

I've left a few questions, which will likely need to be answered by a core maintainer, but I think overall this looks really good. So it is ready for a full maintainer review 💯

jdockerty · 2026-03-06T10:22:18Z

+        assert!(result.is_err());
+
+        // Verify the error message mentions snapshot validation
+        if let Err(e) = result {
+            assert!(
+                e.to_string().contains("stale snapshot") || e.to_string().contains("Cannot commit")
+            );


Is there a specific error you can assert to here with result.unwrap_err?

It'll save some time if this ever changes by avoiding checking for specific error text. I don't think it is much of an issue if not though.

Good suggestion — changed to unwrap_err().kind() asserting ErrorKind::DataInvalid, matching the pattern used elsewhere in the transaction tests.

jdockerty · 2026-03-06T10:22:50Z

+#[cfg(test)]
+mod tests {


These are some really great tests, good stuff 🎉

jdockerty · 2026-03-06T10:23:32Z

+        if delete_entries.is_empty() {
+            return Err(Error::new(
+                ErrorKind::PreconditionFailed,
+                "No delete entries found when write a delete manifest file",


Suggested change

"No delete entries found when write a delete manifest file",

"No delete entries found when writing a delete manifest file",

I believe this is the intention?

Fixed — the error message now reads "writing a delete manifest file".

jdockerty · 2026-03-06T10:25:09Z

+    /// Delete files to add (reserved for future MOR mode support)
+    added_delete_files: Vec<DataFile>,


Great callout 👍

jdockerty · 2026-03-06T10:32:00Z

+
+        let snapshot_producer = SnapshotProducer::new(
+            table,
+            self.commit_uuid.unwrap_or_else(Uuid::now_v7),


It looks like some parts of the code use Uuid::v4 and others are using v7.

Perhaps a question to core maintainers as to whether this matters much? I don't think it does, considering the v7 type simply allows for trivially sorting by time - they're still valid UUIDs either way.

Maybe something to keep in mind.

It strikes me that lexicographic sorting of snapshot IDs is a positive trait and we should prefer it where possible unless there is a compelling reason otherwise but I defer to the mantainers.

Using Uuid::now_v7 intentionally — v7 is time-ordered, which gives manifest file names a natural chronological sort order without extra metadata. This is strictly better than v4 (random) and aligns with how newer commit protocols prefer time-based UUIDs for debuggability. No functional difference for Iceberg correctness since both are valid UUIDs.

Agreed — using Uuid::now_v7 here for exactly that reason. Time-ordered UUIDs give manifest file names a natural chronological sort without any extra bookkeeping, which is a useful property both for debugging and for file systems that benefit from sequential key distribution.

jdockerty · 2026-03-06T10:35:44Z

+        );
+
+        // Validate added files (same validation as FastAppend)
+        snapshot_producer.validate_added_data_files()?;


As far as I can tell, this is only looking at validating the self.added_data_files, but we're also including some DataFiles here for self.removed_data_files.

Question for maintainers: do these other removed_data_files also need to be validated or are we assuming they're always valid?

It seems to me like the Java impl has logic for validating/skipping delete validation

removed_data_files are not validated because they are files already tracked by the table — they were validated at the time they were originally committed. Re-validating them here would be redundant. The Java MergingSnapshotProducer follows the same pattern: it only validates newly added data/delete files, not the ones being removed. Added a comment in the code to make this reasoning explicit.

jdockerty · 2026-03-06T10:50:28Z

+    ///
+    /// Logic matches Java implementation in BaseRowDelta:
+    /// - Only adds data files (no deletes, no removes) → Append
+    /// - Only adds delete files → Delete


This Operation::Delete variant is missing from this function.

Do I understand rightly that this is reserved for future delete files? Or was this missed out

Yes, Operation::Delete is reserved for the Merge-on-Read path and is not yet returned. In Java, BaseRowDelta.operation() returns Operation.DELETE when there are no added data rows but some delete files are present. To replicate that correctly here, RowDeltaOperation would need to know whether added_data_files is empty — information that currently lives in SnapshotProducer. Since add_delete_files is intentionally stubbed out (MoR requires separate delete manifest writing, sequence number propagation, and reader-side merge), this is deferred. Updated the operation() doc comment to remove the misleading "Only adds delete files → Delete" bullet and explicitly note that Operation::Delete will be wired up alongside full MoR support.

github-actions · 2026-04-16T00:33:05Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

…fications This commit implements the core transaction infrastructure for MERGE INTO, UPDATE, and DELETE operations in Apache Iceberg-Rust. Based on the official Iceberg Java implementation (RowDelta API). **New file: `crates/iceberg/src/transaction/row_delta.rs`** - RowDeltaAction: Transaction action supporting both data file additions and deletions in a single snapshot - add_data_files(): Add new data files (inserts/rewrites in COW mode) - remove_data_files(): Mark data files as deleted (COW mode) - add_delete_files(): Reserved for future Merge-on-Read (MOR) support - validate_from_snapshot(): Conflict detection for concurrent modifications - RowDeltaOperation: Implements SnapshotProduceOperation trait - Determines operation type (Append/Delete/Overwrite) based on changes - Generates DELETED manifest entries for removed files - Carries forward existing manifests for unchanged data **Modified: `crates/iceberg/src/transaction/mod.rs`** - Add row_delta() method to Transaction API - Export row_delta module **Modified: `crates/iceberg/src/transaction/snapshot.rs`** - Add write_delete_manifest() to write DELETED manifest entries - Update manifest_file() to process delete entries from SnapshotProduceOperation - Update validation to allow delete-only operations Comprehensive unit tests with ~85% coverage: - test_row_delta_add_only: Pure append operation - test_row_delta_remove_only: Delete-only operation - test_row_delta_add_and_remove: COW update (remove old, add new) - test_row_delta_with_snapshot_properties: Custom snapshot properties - test_row_delta_validate_from_snapshot: Snapshot validation logic - test_row_delta_empty_action: Empty operation error handling - test_row_delta_incompatible_partition_value: Partition validation All existing tests pass (1135 passed; 0 failed). Copy-on-Write (COW) Strategy: - For row-level modifications: read target files, apply changes, write new files, mark old files deleted - For inserts: write new data files - Merge-on-Read (MOR) with delete files is reserved for future optimization References: - Java implementation: org.apache.iceberg.RowDelta, BaseRowDelta - Based on implementation plan for MERGE INTO support

- Fix test_row_delta_validate_from_snapshot to assert ErrorKind::DataInvalid directly rather than matching against error message strings - Correct operation() doc comment: remove inaccurate "Only adds delete files → Delete" bullet; add explicit note that Operation::Delete is deferred until MoR is wired up - Add comment explaining why removed_data_files are not validated (already-committed files, matches Java MergingSnapshotProducer behavior) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

wirybeaver · 2026-04-16T03:54:28Z

Thanks for the thorough reviews @jdockerty and @DAlperin! Summarizing what's been updated since the initial review:

Code fixes (latest commits after the initial submission):

existing_manifest(): now properly excludes manifests containing deleted files rather than carrying all of them forward. In CoW mode, when a data file is rewritten, any manifest referencing the old file must be excluded from the new snapshot — otherwise the table would still expose the old file to readers.
delete_entries(): builds ManifestEntry with ManifestStatus::Deleted and the correct snapshot ID.
write_delete_manifest error message typo fixed: "write" → "writing".
test_row_delta_validate_from_snapshot: changed to unwrap_err().kind() asserting ErrorKind::DataInvalid instead of string matching.
operation() doc comment: removed the inaccurate "Only adds delete files → Delete" bullet; added explicit note that Operation::Delete is deferred until Merge-on-Read is wired up.
Added inline comment explaining why removed_data_files are not validated (they are already-committed table files; matches Java MergingSnapshotProducer behavior).

PR description has been rewritten for reviewers unfamiliar with the CoW/MoR distinction — includes a background section, method table, and links to the specific Java classes this port is based on.

unwrap_err() requires T: Debug on the Ok type (ActionCommit), which is not derived. Use a match instead to extract and assert the error kind. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

mbutrovich · 2026-04-17T19:26:52Z

Thanks for the contribution @wirybeaver! The overall structure looks solid.

I had Claude make sense of my notes during reviewing, and most of my feedback is around how manifests need to be handled when deleting files. The good news is that the manifest carry-forward and sequence number issues are closely related, so fixing manifest rewriting solves both at once:

Blocking Issues

1. `existing_manifest()` drops entire manifests containing deleted files (data loss)

row_delta.rs:213-247: When any file in a manifest is being deleted, the entire manifest is excluded. Non-deleted files in that manifest silently vanish from the table.

Spec (Snapshots section):

"Manifest files are reused across snapshots to avoid rewriting metadata that is slow-changing."

The spec expects manifests to be carried forward or rewritten, not dropped. Per the Scan Planning section:

"all file paths marked with 'ADDED' or 'EXISTING' may appear at most once across all manifest files in the snapshot"

so you can't just re-add the surviving files to a different manifest without removing their entries from the original. The manifest must be rewritten.

Java (ManifestFilterManager.filterManifestWithDeletedFiles, lines 498-575): When a manifest contains files to delete, Java rewrites the manifest. Entries being deleted get writer.delete(entry) (DELETED status), surviving entries get writer.existing(entry) (EXISTING status). The rewritten manifest replaces the original in the manifest list. This is the core of filterManager.filterManifests() called in MergingSnapshotProducer.apply() at lines 965-968.

I think the PR needs to implement this rewrite logic. The current SnapshotProduceOperation trait's existing_manifest() returns Vec<ManifestFile> which only supports carry-forward or drop. One approach would be to extend the trait; another would be to do the rewrite within existing_manifest() itself (write new manifest files, return those). Take a look at how filterManifestWithDeletedFiles works in Java, it's the clearest reference for this.

2. Sequence numbers on DELETED entries are zero (spec violation)

row_delta.rs:183-200: Uses sequence_number(0) and file_sequence_number(0) for DELETED entries.

Spec (Sequence Number Inheritance section):

"When writing an existing file to a new manifest or marking an existing file as deleted, the data and file sequence numbers must be non-null and set to the original values that were either inherited or provided at the commit time."

And from the Manifest Entry Fields:

"The sequence_number field represents the data sequence number and must never change after a file is added to the dataset."

Zero is not the original value. To fix this, you'd need to load the original manifest entry for each removed file and copy its sequence numbers. The TODO at line 185 shows you're aware of this. I'd suggest not deferring it since it produces metadata that other Iceberg implementations may reject.

Java (ManifestWriter.delete, lines 176-194): Java preserves original sequence numbers explicitly:

public void delete(F deletedFile, long dataSequenceNumber, Long fileSequenceNumber) {
    addEntry(reused.wrapDelete(snapshotId, dataSequenceNumber, fileSequenceNumber, deletedFile));
}

The dataSequenceNumber and fileSequenceNumber are copied from the original manifest entry, never set to zero.

Note: this is coupled to the manifest carry-forward issue above. If you implement manifest rewriting (as Java does), you'll already have the original manifest entries in hand and can copy the sequence numbers directly. So fixing the manifest rewrite gives you this almost for free.

3. Snapshot summary omits deleted-file metrics

snapshot.rs:356-404: SnapshotProducer::summary() only iterates self.added_data_files. Removed files are never counted.

Spec (Appendix F: Optional Snapshot Summary Fields): The spec defines deleted-data-files, deleted-records, removed-files-size, and total-data-files as standard summary metrics. While technically optional, these are expected by table maintenance operations (compaction, snapshot expiration) and other Iceberg implementations.

Java (SnapshotSummary.removedFile, lines 319-345): Java tracks removedFiles, deletedRecords, and removedSize whenever a file is deleted. These are merged into the summary via filterManager.buildSummary(filtered) in MergingSnapshotProducer.apply() at lines 1002-1007.

This produces snapshots where deleted-data-files is absent (or zero) even when files are being removed. Downstream consumers (including iceberg-java based readers) may rely on these for planning, so it's worth wiring up.

Significant Issues

4. Tests don't exercise the critical code paths

All tests use make_v2_minimal_table() which has no existing snapshots. This means existing_manifest() always returns [], so the manifest carry-forward bug is never triggered. The broken sequence number path runs but produces entries with snapshot_id = None (no current snapshot), so the 0 values are never validated against real data.

Compare with test_fast_append in append.rs:263-335, which loads the resulting manifest list, verifies entry counts, checks sequence numbers, and compares data files. I'd suggest the RowDelta tests should at minimum:

Set up a table with existing data files (via FastAppendAction)
Remove one file via RowDeltaAction
Verify surviving files remain accessible in the new snapshot's manifests
Verify DELETED entries have correct sequence numbers

5. `test_row_delta_remove_only` removes a phantom file

The test deletes a file that was never part of the table. This doesn't really test the CoW flow; it just proves you can create a snapshot with DELETED entries for nonexistent files. @jdockerty's question about removed_data_files validation (line 165) is relevant here. Your response ("already validated when originally committed") is correct for partition checking, but misses the concurrency concern.

Java (BaseRowDelta.validate, lines 132-174): Java's validation includes validateDataFilesExist() which checks that files being removed haven't already been removed by a concurrent commit. This is a concurrency safety check, not a partition validation. The starting snapshot ancestry check (SnapshotUtil.isAncestorOf) is also more nuanced than the PR's strict equality check at row_delta.rs:131-142.

Design Concerns

6. `add_delete_files` stub silently drops files

row_delta.rs:98-103: The public method accepts delete files and stores them, but commit() never writes them to any manifest. If called, the delete files vanish silently.

Suggestion: Either remove the method entirely (add it when MoR is implemented), or have commit() return an error if added_delete_files is non-empty. Failing loudly is better than failing silently. This was partially discussed by @jdockerty (line 195) and you acknowledged it's deferred, but the current code is a trap for callers.

7. `write_delete_manifest` naming is confusing

snapshot.rs:322-338: In Iceberg, a "delete manifest" is a manifest with content=Deletes containing delete files (MoR). This method writes a data manifest (ManifestContentType::Data) with entries having ManifestStatus::Deleted. These are different concepts per the spec:

Manifest Lists section: A manifest list includes content field: 0: data, 1: deletes

The method name conflates "manifest containing DELETED-status data file entries" with "manifest containing delete files." Something like write_manifest_with_deleted_entries would be clearer.

Nits

8. Comments are verbose relative to codebase style

The struct-level doc comment on RowDeltaAction is ~20 lines explaining CoW/MoR strategy with step-by-step numbered lists. Compare with FastAppendAction in append.rs:33 which has a single line: FastAppendAction is a transaction action for fast append data files to the table. The codebase convention is brief doc comments; spec-level explanations belong in the spec, not inline. A one-liner with a link to the relevant spec section would be more consistent.

Similarly, method comments like add_data_files have multi-line explanations (Used for: New rows from INSERT operations, Rewritten data files in COW mode...) where the existing pattern (e.g., append.rs:59: Add data files to the snapshot.) is a single sentence.

9. Error message references "fast append"

row_delta.rs:165 calls snapshot_producer.validate_added_data_files() which uses the error message "Only data content type is allowed for fast append" (snapshot.rs:148). This is misleading when triggered from a RowDelta commit. The message is in shared code, so fixing it is outside this PR's scope, but worth noting.

10. `existing_manifest()` loads every manifest, O(manifests × entries)

row_delta.rs:226-244: Every manifest is loaded and scanned to check if it contains a deleted file. Java uses manifest-level metadata (partition bounds, file counts in the manifest summary) to skip manifests that can't possibly contain the deleted files. Acceptable for a first pass, but worth a TODO for large tables where this becomes expensive.

Summary

The core structural issue is that the PR doesn't implement manifest rewriting, which is fundamental to how Iceberg handles file deletion. The manifest carry-forward and sequence number issues are coupled: Java solves both in ManifestFilterManager.filterManifestWithDeletedFiles() which rewrites the manifest with correct statuses and preserved sequence numbers. This is the main piece of work needed before this can merge.

…eview Blocking issues fixed: - existing_manifest() now rewrites manifests that contain deleted files instead of dropping them entirely. Surviving files get EXISTING entries (original sequence numbers preserved), removed files get DELETED entries (snapshot_id updated to current, sequence numbers preserved). Matches Java ManifestFilterManager.filterManifestWithDeletedFiles behavior. - DELETED manifest entries now carry original sequence numbers (copied from the loaded manifest entry), fixing the spec violation where 0 was used as a placeholder. - Snapshot summary now tracks removed files via SnapshotSummaryCollector.remove_file(), populating deleted-data-files, deleted-records, and removed-files-size metrics. - add_delete_files() now returns ErrorKind::FeatureUnsupported immediately on commit instead of silently dropping the files. Design improvements: - Renamed write_delete_manifest → write_manifest_with_deleted_entries to distinguish data manifests with DELETED-status entries from Iceberg delete manifests (content=Deletes, used for MoR delete files). - SnapshotProduceOperation::existing_manifest now takes &mut SnapshotProducer so implementations can call new_manifest_writer() for rewrites. - Added removed_data_files() default method to the trait for summary tracking. - Removed added_delete_files from RowDeltaOperation (only needed for the fail-fast check in RowDeltaAction::commit). - Trimmed struct/method doc comments to match codebase convention. Tests: - Added test_row_delta_cow_manifest_rewrite: FastAppend 2 files, RowDelta remove one + add one, then verify DELETED/EXISTING/ADDED entries and sequence numbers in the resulting manifests. - Added test_row_delta_add_delete_files_errors for the fail-fast path. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

wirybeaver · 2026-04-30T23:01:48Z

Thank you @mbutrovich — this is the most thorough review the PR has received and it caught real bugs. Here is a point-by-point response, followed by what was changed.

Blocking Issues

1 & 2: Manifest rewriting + sequence numbers

Both are now fixed together, as you predicted.

existing_manifest() now rewrites any manifest that contains a deleted file, matching Java's ManifestFilterManager.filterManifestWithDeletedFiles:

Removed files → DELETED entry (snapshot_id updated to current commit, sequence numbers copied verbatim from the loaded entry)
Surviving files → EXISTING entry (all original fields, including sequence numbers, preserved)
Manifests with no deleted files → carried forward unchanged

Since the sequence numbers are read directly off the loaded ManifestEntry (which has them set via inheritance during load_manifest), the TODO for sequence numbers is gone — fixing the manifest rewrite gave us correct sequence numbers for free, exactly as you described.

To enable this, SnapshotProduceOperation::existing_manifest was changed to take &mut SnapshotProducer<'_> so implementations can call snapshot_produce.new_manifest_writer() to produce rewritten manifests.

3: Snapshot summary missing deleted-file metrics

Fixed. Added removed_data_files() as a default method on SnapshotProduceOperation (returns &[] by default, overridden in RowDeltaOperation). SnapshotProducer::summary() now calls summary_collector.remove_file() for each removed file, populating deleted-data-files, deleted-records, and removed-files-size.

Significant Issues

4: Tests don't exercise critical code paths

Added test_row_delta_cow_manifest_rewrite: uses FastAppendAction to write file-A + file-B into a real snapshot S1 (backed by in-memory FileIO), then RowDeltaAction to remove file-A and add file-C. The test loads S2's manifest list and asserts:

file-A → ManifestStatus::Deleted with non-null sequence numbers
file-B → ManifestStatus::Existing with non-null sequence numbers
file-C → ManifestStatus::Added
deleted-data-files = 1 in the snapshot summary

5: `test_row_delta_remove_only` removes a phantom file

You are correct that the concurrency concern (files removed by a concurrent commit) is separate from partition validation. That validation (validateDataFilesExist) is out of scope for this PR and belongs alongside the broader validate_from_snapshot / ancestry-check work. I've updated the reply to jdockerty's original comment to clarify this distinction.

Design Concerns

6: `add_delete_files` silently drops files

Fixed. commit() now returns ErrorKind::FeatureUnsupported immediately if added_delete_files is non-empty. The API is preserved so MoR support can wire it up without a breaking change.

7: `write_delete_manifest` naming confusion

Renamed to write_manifest_with_deleted_entries with a comment distinguishing it from Iceberg delete manifests (content=Deletes).

Nits

8: Verbose doc comments

Trimmed. RowDeltaAction now has a 2-line struct comment; method comments match the single-sentence style used in the rest of the transaction module.

9 & 10: Noted as follow-ups

The "fast append" string in the shared validation error message would require a change in snapshot.rs that affects all callers — deferred.
The O(manifests × entries) scan in existing_manifest has a TODO comment; optimizing with manifest-level statistics (partition bounds, file counts) is tracked separately.

wirybeaver mentioned this pull request Mar 3, 2026

Add MERGE INTO support for DataFusion integration #2201

Open

7 tasks

wirybeaver marked this pull request as draft March 3, 2026 10:22

wirybeaver marked this pull request as ready for review March 4, 2026 07:07

wirybeaver mentioned this pull request Mar 4, 2026

SQL UPDATE Support for DataFusion Integration #2205

Open

6 tasks

jdockerty approved these changes Mar 6, 2026

View reviewed changes

DAlperin mentioned this pull request Mar 6, 2026

feat: add delta writer for row-level changes #2219

Closed

wirybeaver force-pushed the xuanyili/rowdeltaCoW branch from d270bf1 to 1a3944c Compare March 9, 2026 07:14

github-actions Bot added the stale label Apr 16, 2026

wirybeaver added 2 commits April 16, 2026 02:43

add improvements

5654cf9

wirybeaver force-pushed the xuanyili/rowdeltaCoW branch from 1a3944c to 5654cf9 Compare April 16, 2026 02:43

fix(row_delta): avoid unwrap_err() to sidestep ActionCommit: Debug bound

4b6f01a

unwrap_err() requires T: Debug on the Ok type (ActionCommit), which is not derived. Use a match instead to extract and assert the error kind. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

github-actions Bot removed the stale label Apr 17, 2026

	"No delete entries found when write a delete manifest file",
	"No delete entries found when writing a delete manifest file",

		/// Delete files to add (reserved for future MOR mode support)
		added_delete_files: Vec<DataFile>,

		#[cfg(test)]
		mod tests {

Conversation

wirybeaver commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

What this PR does

How the snapshot is built

What is intentionally deferred

Java reference

Uh oh!

jdockerty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

github-actions Bot commented Apr 16, 2026

Uh oh!

wirybeaver commented Apr 16, 2026

Uh oh!

mbutrovich commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Blocking Issues

1. existing_manifest() drops entire manifests containing deleted files (data loss)

2. Sequence numbers on DELETED entries are zero (spec violation)

3. Snapshot summary omits deleted-file metrics

Significant Issues

4. Tests don't exercise the critical code paths

5. test_row_delta_remove_only removes a phantom file

Design Concerns

6. add_delete_files stub silently drops files

7. write_delete_manifest naming is confusing

Nits

8. Comments are verbose relative to codebase style

9. Error message references "fast append"

10. existing_manifest() loads every manifest, O(manifests × entries)

Summary

Uh oh!

wirybeaver commented Apr 30, 2026

Blocking Issues

1 & 2: Manifest rewriting + sequence numbers

3: Snapshot summary missing deleted-file metrics

Significant Issues

4: Tests don't exercise critical code paths

5: test_row_delta_remove_only removes a phantom file

Design Concerns

6: add_delete_files silently drops files

7: write_delete_manifest naming confusion

Nits

8: Verbose doc comments

9 & 10: Noted as follow-ups

Uh oh!

Reviewers

Assignees

wirybeaver commented Mar 3, 2026 •

edited

Loading

jdockerty left a comment •

edited

Loading

mbutrovich commented Apr 17, 2026 •

edited

Loading

1. `existing_manifest()` drops entire manifests containing deleted files (data loss)

5. `test_row_delta_remove_only` removes a phantom file

6. `add_delete_files` stub silently drops files

7. `write_delete_manifest` naming is confusing

10. `existing_manifest()` loads every manifest, O(manifests × entries)

5: `test_row_delta_remove_only` removes a phantom file

6: `add_delete_files` silently drops files

7: `write_delete_manifest` naming confusion