laskoviymishka commented on code in PR #3253:
URL: https://github.com/apache/iceberg-rust/pull/3253#discussion_r4186820778
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -85,6 +86,12 @@ pub(crate) trait SnapshotProduceOperation: Send + Sync {
&self,
snapshot_produce: &SnapshotProducer<'_>,
) -> impl Future<Output = Result<Vec<ManifestFile>>> + Send;
+
+ /// Returns whether the snapshot summary resets the table totals. Defaults
to every
+ /// overwrite; an operation that overwrites only some files returns
`false`.
+ fn truncate_full_table(&self) -> bool {
+ self.operation() == Operation::Overwrite
Review Comment:
This is last round's concern flipped rather than closed: with the default
now opt-out, a *partial* overwrite that returns `Overwrite` and forgets to
override truncates the totals instead of accumulating them. Same silent
failure, opposite trigger, still riding on a trait default rather than the type.
Main-parity is a fine call, but let's pin the contract where it's defined —
either a one-line doc note that partial overwrites must return `false`, or a
`debug_assert!(!(self.truncate_full_table() &&
!deleted_data_files.is_empty()))` on commit, since a full-table truncate that
also reports explicit deletes is the tell of a mis-declared partial overwrite.
Doc note or assert — which do you prefer?
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -396,6 +412,19 @@ impl<'a> SnapshotProducer<'a> {
);
}
+ for data_file in &self.deleted_data_files {
Review Comment:
This is concern #2 from last round — it moved but didn't close. Dropping
`removed_data_files()` killed the two-hook disagreement, but the producer still
has two independent inputs: the summary counts `deleted_data_files`, while the
manifests' `Deleted` entries come from the operation's own
`existing_manifest`/`delete_entries`. Nothing in this PR reconciles them.
So a caller can commit a snapshot claiming `deleted-data-files=N` with
manifests that still list those files as live, and since `total-*` is derived
cumulatively from these counts, the drift gets inherited by every later
snapshot — permanently. The body conceding the summary trusts the caller's list
is exactly that gap, now explicit rather than enforced, and "#3300 rejects
paths it can't find" is an invariant of a different PR, not of this primitive.
For a step-1 primitive that later PRs inherit, I'd want the binding visible
from this diff: either have `produce_manifests` own the `Deleted` entries for
`deleted_data_files` so one list drives both (the Java
`MergingSnapshotProducer` shape), or keep summary-only but say so loudly on the
field/constructor doc and add a cheap `debug_assert!` in `commit()` summing the
`Deleted` entries this commit wrote against `deleted_data_files.len()`.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -396,6 +412,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)
+ })?;
+ summary_collector.remove_file(
+ data_file,
+ table_metadata.current_schema().clone(),
Review Comment:
New one this pass: this can panic on the commit path. We resolve the file's
historical spec by `partition_spec_id` (good), but pair it with
`current_schema()` — and `partition_to_path` does
`partition_type(&schema).unwrap()`. If an old spec references a source column
that's since been dropped or renamed, that `unwrap` panics instead of erroring,
and old-spec files are exactly the ones a delete/overwrite is most likely to
touch. Added files get a `validate_partition_value` check; deleted files get
none.
I'd bind the schema the spec was actually built against (`schema_by_id`, or
the spec's own bound schema) instead of `current_schema()`, and make
`partition_to_path` fallible — or at least guard the `unwrap` — so this returns
`invalid_data!` rather than unwinding. The tests only exercise the
unpartitioned default spec, so a fixture with a deleted file on an
older/partitioned spec would pin both this and the unknown-spec-id error branch
right above it.
--
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]
