dannycjones commented on code in PR #2367:
URL: https://github.com/apache/iceberg-rust/pull/2367#discussion_r3617009005
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -375,11 +457,50 @@ impl<'a> SnapshotProducer<'a> {
summary_collector.set_partition_summary_limit(partition_summary_limit);
+ // Helper: look up the partition spec for a file. Returns DataInvalid
+ // if the file references a spec that doesn't exist in the table.
+ let spec_for = |data_file: &DataFile| {
+ table_metadata
+ .partition_spec_by_id(data_file.partition_spec_id)
+ .cloned()
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Cannot find partition spec {} for file: {}",
+ data_file.partition_spec_id,
+ data_file.file_path()
+ ),
+ )
+ })
+ };
+
for data_file in &self.added_data_files {
summary_collector.add_file(
data_file,
table_metadata.current_schema().clone(),
- table_metadata.default_partition_spec().clone(),
+ spec_for(data_file)?,
+ );
+ }
+ for delete_file in &self.added_delete_files {
+ summary_collector.add_file(
+ delete_file,
+ table_metadata.current_schema().clone(),
+ spec_for(delete_file)?,
+ );
+ }
+ for data_file in &self.removed_data_files {
+ summary_collector.remove_file(
+ data_file,
+ table_metadata.current_schema().clone(),
+ spec_for(data_file)?,
+ );
+ }
+ for delete_file in &self.removed_delete_files {
+ summary_collector.remove_file(
+ delete_file,
+ table_metadata.current_schema().clone(),
+ spec_for(delete_file)?,
);
}
Review Comment:
There's something that really bothers me about the way we're handling a lot
of these structs (`DataFile`, `Schema`, `PartitionSpec`).
We have a few function calls to the summary collector in this PR:
```rust
for data_file in &self.added_data_files {
summary_collector.add_file(
data_file,
table_metadata.current_schema().clone(),
spec_for(data_file)?,
);
}
for delete_file in &self.added_delete_files {
summary_collector.add_file(
delete_file,
table_metadata.current_schema().clone(),
spec_for(delete_file)?,
);
}
for data_file in &self.removed_data_files {
summary_collector.remove_file(
data_file,
table_metadata.current_schema().clone(),
spec_for(data_file)?,
);
}
for delete_file in &self.removed_delete_files {
summary_collector.remove_file(
delete_file,
table_metadata.current_schema().clone(),
spec_for(delete_file)?,
);
}
```
Ultimately, new data-layer files are added or old ones are removed. The new
ones should always be using the schema and partition spec with which they were
written (note, not the one which may be the default at the time of commit?).
Later, we are removing data-layer files which were written with older schemas
and partition specs.
If we do not use the schemas and partition specs with which these data files
were written, the partition-specific snapshot summaries are wrong, or maybe not
even possible to calculate with the current code ending in a failed job.
This makes me want a few things, which I also think is in tension with the
way the code base is currently organized.
- When I'm handling these `DataFile` structs, I know which partition values,
partition spec, and schema was used. It would be great if these could be kept
with the struct, rather than passed along outside. The latter is error prone,
as we saw here.
- When I'm looking at `PartitionSpec` and `UnboundPartitionSpec`, I'm seeing
several use cases with different requirements. Adding a partition spec to the
table stores fake values in the spec_id to be replaced later. Much of the time,
the partition spec is passed along with a schema since they need to be used
together (but not always with a partition struct/key). `PartitionSpec` implies
its bound, but its not really - it just happened to be valid at some point with
some schema, and has assigned field IDs.
- Some of these types are used for serde. For example, `PartitionSpec` does
not currently contain a reference to a schema. If I was to add it, suddenly
this would introduce issues later when we pass `FileScanTask` between workers
(not currently serializable, but the plan is to make it so). I'd need to
investigate further how we're using things like `DataFile` as well to
understand if there are similar risks, especially since I see its used in the
DataFusion integration.
Ultimately I'd prefer this section of code look like this:
```rust
for data_file in &self.added_data_files {
summary_collector.add_file(data_file);
}
for delete_file in &self.added_delete_files {
summary_collector.add_file(delete_file);
}
for data_file in &self.removed_data_files {
summary_collector.remove_file(data_file);
}
for delete_file in &self.removed_delete_files {
summary_collector.remove_file(delete_file);
}
```
I'm still trying to make sense of this, and exploring the layers above and
below (Arrow + DataFusion) to ensure I do understand the use cases of these
structs well before I start proposing changes. I wanted to share some thoughts
of where I'm at, in case you have anything you'd like to chime in on or point
out issues I've missed.
--
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]
