You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "c-thiel (via GitHub)" <gi...@apache.org> on 2024/12/13 17:52:16 UTC
[PR] feat: TableMetadata Statistics [iceberg-rust]
c-thiel opened a new pull request, #799:
URL: https://github.com/apache/iceberg-rust/pull/799
Adds `StatisticFile` and `PartitionStatisticsFile` to spec, builder and REST TableUpdate.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: TableMetadata Statistic Files [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #799:
URL: https://github.com/apache/iceberg-rust/pull/799#discussion_r1886956770
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -524,6 +526,52 @@ impl TableMetadataBuilder {
self
}
+ /// Set statistics for a snapshot
+ pub fn set_statistics(mut self, statistics: StatisticsFile) -> Self {
+ self.metadata
+ .statistics
+ .insert(statistics.snapshot_id, statistics.clone());
Review Comment:
I thought about this as well, but then followed java.
Currently statistics and snapshots are quite separate from each other. If we implement your check (which I like), I think we should eventually also implement:
* Upon deserialization discard statistics that belong to nonexistant snapshots
* When a snapshot is removed delete the statistics for it as well
This would result in snapshots for statistics not missing. It is unclear however what should happen to the puffin files in these cases. We would have coherent metadata, but probably also orphan files.
Do we know why the check is not there in Java?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: TableMetadata Statistic Files [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #799:
URL: https://github.com/apache/iceberg-rust/pull/799
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: TableMetadata Statistic Files [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #799:
URL: https://github.com/apache/iceberg-rust/pull/799#discussion_r1886823887
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -524,6 +526,52 @@ impl TableMetadataBuilder {
self
}
+ /// Set statistics for a snapshot
+ pub fn set_statistics(mut self, statistics: StatisticsFile) -> Self {
+ self.metadata
+ .statistics
+ .insert(statistics.snapshot_id, statistics.clone());
Review Comment:
Do we want to check if a `snapshot-id` exists?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: TableMetadata Statistic Files [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #799:
URL: https://github.com/apache/iceberg-rust/pull/799#discussion_r1884798642
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -158,11 +160,15 @@ pub struct TableMetadata {
/// writers, but is not used when reading because reads use the specs
/// stored in manifest files.
pub(crate) default_sort_order_id: i64,
- ///A map of snapshot references. The map keys are the unique snapshot reference
+ /// A map of snapshot references. The map keys are the unique snapshot reference
/// names in the table, and the map values are snapshot reference objects.
/// There is always a main branch reference pointing to the current-snapshot-id
/// even if the refs map is null.
pub(crate) refs: HashMap<String, SnapshotReference>,
+ /// Mapping of snapshot ids to statistics files.
+ pub(crate) statistics: HashMap<i64, StatisticsFile>,
Review Comment:
Can we ever have more than one `StatisticsFile` for a `snapshot_id`?
In java this is modeled as a mapping `snapshot_id` : `Vec<StatisticsFile>`, however I couldn't find a way to get more than one element into the `Vec` for a `snapshot_id` other than deserializing.
https://github.com/apache/iceberg/blob/540d6a6251e31b232fe6ed2413680621454d107a/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1310-L1320
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: TableMetadata Statistic Files [iceberg-rust]
Posted by "c-thiel (via GitHub)" <gi...@apache.org>.
c-thiel commented on code in PR #799:
URL: https://github.com/apache/iceberg-rust/pull/799#discussion_r1884298259
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -446,6 +446,30 @@ pub enum TableUpdate {
/// Properties to remove
removals: Vec<String>,
},
+ /// Set statistics for a snapshot
+ #[serde(with = "_serde_set_statistics")]
+ SetStatistics {
+ /// File containing the statistics
+ statistics: StatisticsFile,
+ },
Review Comment:
In IRC and Java `SetStatistics` has an additional field `snapshot_id` ([link](https://github.com/apache/iceberg/blob/540d6a6251e31b232fe6ed2413680621454d107a/open-api/rest-catalog-open-api.yaml#L2902)).
This field is redundant with `StatisticsFile.snapshot_id` and is only used as an [assertion in Java](https://github.com/apache/iceberg/blob/540d6a6251e31b232fe6ed2413680621454d107a/core/src/main/java/org/apache/iceberg/TableMetadata.java#L1314).
I removed the redundancy for rust and will start a discussion on the mailing List how to handle this.
As we still need to be spec compliant, we need custom serializer / deserializer.
Slack: https://apache-iceberg.slack.com/archives/C03LG1D563F/p1734109745807119
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: TableMetadata Statistic Files [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #799:
URL: https://github.com/apache/iceberg-rust/pull/799#discussion_r1886819635
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -446,6 +446,30 @@ pub enum TableUpdate {
/// Properties to remove
removals: Vec<String>,
},
+ /// Set statistics for a snapshot
+ #[serde(with = "_serde_set_statistics")]
+ SetStatistics {
+ /// File containing the statistics
+ statistics: StatisticsFile,
+ },
Review Comment:
Thanks for raising this on the dev-list: https://lists.apache.org/thread/6fnrp73wokfqlt5vormpjyjmtvl29ll1
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org