amogh-jahagirdar commented on code in PR #11040:
URL: https://github.com/apache/iceberg/pull/11040#discussion_r1861360140
##########
api/src/main/java/org/apache/iceberg/Table.java:
##########
@@ -373,4 +374,14 @@ default Snapshot snapshot(String name) {
return null;
}
+
+ /**
+ * Returns the statistics file for the given snapshot id, if available.
+ *
+ * @return the {@link StatisticsFile} for the given snapshot id, if
available.
+ */
+ default Optional<StatisticsFile> statistics(long snapshotId) {
Review Comment:
Thanks for adding this, I think this API is in the right direction but I
wonder if it's taking enough information to be able to resolve the right file.
There's an implicit assumption in the current implementation of this API that a
Puffin file will have all the blobs needed. I think it's possible that there
could multiple puffins, and one puffin has blob type NDV and the other puffin
has blob type SomeFutureIndex. both of these puffin files are for snapshot 1.
In the current implementation if someone wants the Puffin where there's
SomeFutureIndex, but we happen to return the Puffin due to the generic "Find me
a puffin with the given snapshot logic" I feel like the API isn't really doing
what it needs.
Here's the signature I'm thinking at the moment:
default StatisticsFile statisticsFor(long snapshotId, String blobType)
This will attempt to attempt to find the statistics file produced at the
given snapshot which contains the blob type.
I also think that in case a statistics file for exactly that snapshot
couldn't be found, we should return the latest statistics rather than just
return nothing. While it's possible that the statistics are not completely
accurate in this approach, I think in the average case data distributions
wouldn't change so drastically between snapshots that the statistics would work
horribly against the query. It's probably better in the average case to have
some statistics. If there's no there's no statistics file containing the
desired blob then I think we'd just return null.
WDYT?
##########
core/src/main/java/org/apache/iceberg/BaseTable.java:
##########
@@ -280,6 +281,11 @@ public String toString() {
return name();
}
+ @Override
+ public Optional<StatisticsFile> statistics(long snapshotId) {
+ return statisticsFiles().stream().filter(file -> file.snapshotId() ==
snapshotId).findFirst();
Review Comment:
See comment above on a different signature to incorporate the desired blob
type a user is looking for and how I think this APi should be more best effort
(return the latest statistics file that we can find) rather than returning
Optional.empty
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
