c-thiel commented on code in PR #1682:
URL: https://github.com/apache/iceberg-rust/pull/1682#discussion_r2476720741
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -420,8 +434,16 @@ impl<'a> SnapshotProducer<'a> {
.with_sequence_number(next_seq_num)
.with_summary(summary)
.with_schema_id(self.table.metadata().current_schema_id())
- .with_timestamp_ms(commit_ts)
- .build();
+ .with_timestamp_ms(commit_ts);
+
+ let new_snapshot = if let Some(writer_next_row_id) =
writer_next_row_id {
+ let assigned_rows = writer_next_row_id -
self.table.metadata().next_row_id();
+ new_snapshot
+ .with_row_range(first_row_id, assigned_rows)
+ .build()
+ } else {
+ new_snapshot.build()
+ };
Review Comment:
Isn't this how its supposed to be?
From the spec:
Snapshots that are created after upgrading to v3 must set the snapshot's
first-row-id and assign row IDs to existing and added files in the snapshot.
When writing the manifest list, all data manifests must be assigned a
first_row_id, which assigns a first_row_id to all data files via inheritance.
This says that also exiting rows should be respected.
Java:
https://github.com/apache/iceberg/blob/92ae3d9da2afba3fa6fa0f74af6d78d773a6ab26/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L294-L297
https://github.com/apache/iceberg/blob/92ae3d9da2afba3fa6fa0f74af6d78d773a6ab26/core/src/main/java/org/apache/iceberg/ManifestListWriter.java#L132-L143
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
