Skip to content

feat(transaction): let a snapshot operation report removed files and rewrite manifests #3253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laskoviymishka merged 14 commits into from
Oct 15, 2026
Merged

feat(transaction): let a snapshot operation report removed files and rewrite manifests #3253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
fed895f
Let a snapshot operation report the files it removed
jopdorp Sep 18, 2026
3fc4e80
Keep manifest names unique across a commit
jopdorp Sep 18, 2026
6325d22
Write a manifest under the schema and spec it was written with
jopdorp Sep 18, 2026
1cad731
test(transaction): cover partial overwrite and per-file spec in snaps…
jopdorp Sep 29, 2026
d5623e3
Drop the full-table truncation from the snapshot summary
jopdorp Oct 2, 2026
296532e
Omit a summary total that would go negative, as Java does
jopdorp Oct 2, 2026
eeff802
Name the removed-file fields and take the manifest spec by ref
jopdorp Oct 2, 2026
495f3ad
Take the added files again instead of cloning them for the summary
jopdorp Oct 2, 2026
96da179
Assert an operation reports exactly the files it marked deleted
jopdorp Oct 2, 2026
a5653bc
Say in the removed_data_files doc what it must match
jopdorp Oct 2, 2026
b194d10
Merge remote-tracking branch 'origin/main' into review-3253
jopdorp Oct 2, 2026
2977261
Summarize deleted files from the producer's list, as pyiceberg does
jopdorp Oct 2, 2026
683fae4
Check that deletion summaries match manifest entries
jopdorp Oct 13, 2026
5f36938
Format dropped-source integer partition values
jopdorp Oct 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions crates/iceberg/src/spec/partition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1913,6 +1913,53 @@ mod tests {
assert!(!spec.has_sequential_ids());
}

#[test]
fn test_partition_to_path_with_dropped_source_column() {
for (source_type, transform, value, expected) in [
(
PrimitiveType::Int,
Transform::Identity,
Literal::int(1),
"p=1",
),
(
PrimitiveType::Timestamp,
Transform::Identity,
Literal::long(1),
"p=1",
),
(
PrimitiveType::Int,
Transform::Truncate(10),
Literal::int(10),
"p=10",
),
] {
let schema = Schema::builder()
.with_fields(vec![
NestedField::required(1, "kept", PrimitiveType::Int.into()).into(),
NestedField::required(2, "dropped", source_type.into()).into(),
])
.build()
.unwrap();
let spec = PartitionSpec::builder(schema)
.add_partition_field("dropped", "p", transform)
.unwrap()
.build()
.unwrap();
let current_schema = Schema::builder()
.with_fields(vec![
NestedField::required(1, "kept", PrimitiveType::Int.into()).into(),
])
.build()
.unwrap();
assert_eq!(
spec.partition_to_path(&Struct::from_iter([Some(value)]), current_schema.into()),
expected
);
}
}

#[test]
fn test_partition_to_path() {
let schema = Schema::builder()
Expand Down
24 changes: 23 additions & 1 deletion crates/iceberg/src/spec/snapshot_summary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,10 @@ fn update_totals(
return;
};

let new_total = previous_total + added - removed;
// Like Java, a total that would go negative is omitted rather than written as a false basis.
let Some(new_total) = previous_total.saturating_add(added).checked_sub(removed) else {
return;
};
summary
.additional_properties
.insert(total_property.to_string(), new_total.to_string());
Expand Down Expand Up @@ -1274,4 +1277,23 @@ mod tests {
assert_eq!(props.get(TOTAL_POSITION_DELETES).unwrap(), "2");
assert_eq!(props.get(TOTAL_EQUALITY_DELETES).unwrap(), "1");
}

#[test]
fn test_update_totals_omits_total_when_removed_exceeds_previous_total() {
let previous_summary = Summary {
operation: Operation::Append,
additional_properties: HashMap::from([(TOTAL_DATA_FILES.to_string(), "2".to_string())]),
};
let summary = Summary {
operation: Operation::Delete,
additional_properties: HashMap::from([(
DELETED_DATA_FILES.to_string(),
"3".to_string(),
)]),
};

let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap();

assert!(!updated.additional_properties.contains_key(TOTAL_DATA_FILES));
}
}
8 changes: 6 additions & 2 deletions crates/iceberg/src/spec/values/datum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,12 @@ impl Display for Datum {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
match (&self.r#type, &self.literal) {
(_, PrimitiveLiteral::Boolean(val)) => write!(f, "{val}"),
(PrimitiveType::Int, PrimitiveLiteral::Int(val)) => write!(f, "{val}"),
(PrimitiveType::Long, PrimitiveLiteral::Long(val)) => write!(f, "{val}"),
(PrimitiveType::Int | PrimitiveType::Unknown, PrimitiveLiteral::Int(val)) => {

@laskoviymishka laskoviymishka Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional follow-up: this covers Int/Long literals only. partition_type gives Unknown for identity/truncate over a dropped column, so a dropped decimal or uuid source should end up as (Unknown, Int128) / (Unknown, UInt128), miss the Decimal/Uuid arms below and land on the unreachable!() — same commit-path panic, two more types. I haven't run it, this is from reading the arms.

Two arms before the catch-all plus decimal/uuid cases in test_partition_to_path_with_dropped_source_column would close it:

(PrimitiveType::Unknown, PrimitiveLiteral::Int128(val)) => write!(f, "{val}"),
(PrimitiveType::Unknown, PrimitiveLiteral::UInt128(val)) => {
    write!(f, "{}", uuid::Uuid::from_u128(*val))
}

Since this is #3432's change, that PR or a small follow-up is a fine home for it.

@JeonDaehong JeonDaehong Oct 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this one: identity(decimal), truncate(decimal) and identity(uuid) on a dropped source all hit that unreachable!(). #3432 adds those two arms in b765bbf (uuid prints like Java, decimal prints the unscaled value since the scale is gone with the source).

write!(f, "{val}")
}
(PrimitiveType::Long | PrimitiveType::Unknown, PrimitiveLiteral::Long(val)) => {
write!(f, "{val}")
}
(_, PrimitiveLiteral::Float(val)) => write!(f, "{val}"),
(_, PrimitiveLiteral::Double(val)) => write!(f, "{val}"),
(PrimitiveType::Date, PrimitiveLiteral::Int(val)) => {
Expand Down
1 change: 1 addition & 0 deletions crates/iceberg/src/transaction/append.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ impl TransactionAction for FastAppendAction {
self.commit_uuid.unwrap_or_else(Uuid::now_v7),
self.snapshot_properties.clone(),
self.dedupe_added_files(),
vec![],
);

// validate added files
Expand Down
Loading
Loading