laskoviymishka commented on code in PR #3253:
URL: https://github.com/apache/iceberg-rust/pull/3253#discussion_r4286748123
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -396,6 +425,19 @@ impl<'a> SnapshotProducer<'a> {
);
}
+ for data_file in &self.deleted_data_files {
+ let partition_spec = table_metadata
+ .partition_spec_by_id(data_file.partition_spec_id)
+ .ok_or_else(|| {
+ invalid_data!("Unknown partition spec id {}",
data_file.partition_spec_id)
Review Comment:
Optional: this branch still has no test. A deleted `DataFile` with a
`partition_spec_id` that isn't in the metadata, asserting `summary()` returns
`Err`, would pin it. Happy for that to come in a follow-up.
##########
crates/iceberg/src/spec/values/datum.rs:
##########
@@ -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)) => {
Review Comment:
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:
```rust
(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.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -558,3 +600,140 @@ impl<'a> SnapshotProducer<'a> {
Ok(ActionCommit::new(updates, requirements))
}
}
+
+#[cfg(test)]
+mod tests {
+ use std::collections::HashMap;
+ use std::sync::Arc;
+
+ use uuid::Uuid;
+
+ use super::*;
+ use crate::spec::{
+ DataContentType, DataFileBuilder, Literal, Manifest, NestedField,
PartitionSpec,
+ PrimitiveType, Schema, Transform, Type,
+ };
+ use crate::transaction::tests::make_v2_minimal_table;
+
+ struct DeleteOperation;
+
+ impl SnapshotProduceOperation for DeleteOperation {
+ fn operation(&self) -> Operation {
+ Operation::Delete
+ }
+
+ async fn delete_entries(&self, _: &SnapshotProducer<'_>) ->
Result<Vec<ManifestEntry>> {
+ Ok(vec![])
+ }
+
+ async fn existing_manifest(&self, _: &SnapshotProducer<'_>) ->
Result<Vec<ManifestFile>> {
+ Ok(vec![])
+ }
+ }
+
+ #[test]
+ fn test_summary_counts_deleted_data_files() {
+ let table = make_v2_minimal_table();
+ let deleted = DataFileBuilder::default()
+ .content(DataContentType::Data)
+ .file_path("test/deleted.parquet".to_string())
+ .file_format(DataFileFormat::Parquet)
+ .file_size_in_bytes(100)
+ .record_count(2)
+ .partition_spec_id(table.metadata().default_partition_spec_id())
+ .partition(Struct::from_iter([Some(Literal::long(300))]))
+ .build()
+ .unwrap();
+
+ let producer = SnapshotProducer::new(&table, Uuid::now_v7(),
HashMap::new(), vec![], vec![
+ deleted,
+ ]);
+ let summary = producer.summary(&DeleteOperation).unwrap();
+ let properties = &summary.additional_properties;
+
+ assert_eq!(properties.get("deleted-data-files").unwrap(), "1");
+ assert_eq!(properties.get("deleted-records").unwrap(), "2");
+ assert_eq!(properties.get("removed-files-size").unwrap(), "100");
+ }
+
+ #[cfg(debug_assertions)]
+ #[tokio::test]
+ #[should_panic(expected = "deleted files must match this commit's manifest
entries")]
+ async fn test_commit_rejects_unrecorded_deletions() {
Review Comment:
nit, optional: `_rejects_` reads like an `Err` path, but this pins a
debug-only panic and isn't compiled under `--release`. Something like
`test_commit_panics_on_unrecorded_deletions_in_debug` would make that obvious.
--
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]
