rdblue commented on a change in pull request #1508:
URL: https://github.com/apache/iceberg/pull/1508#discussion_r497164536



##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -51,6 +51,23 @@
    */
   Schema schema();
 
+  /**
+   * Return the {@link Schema schema} for this table at the time of the 
snapshot
+   * specified by the snapshotId.
+   *
+   * @return this table's schema
+   */
+  Schema schemaForSnapshot(long snapshotId);
+
+  /**
+   * Return the {@link Schema schema} for this table at the time of the most 
recent
+   * snapshot as of the specified timestampMillis.
+   * Note: This is not necessarily the schema at the time of the specified 
timestampMillis.
+   *
+   * @return this table's schema
+   */
+  Schema schemaForSnapshotAsOfTime(long timestampMillis);

Review comment:
       Yeah, I agree with you. We have some flexibility here with how we look 
up the schema for a given snapshot. The main thing is that we don't want to 
expose any methods through the `Table` API right now. And since we don't have a 
schema id yet, we can't really add that method to `Snapshot`.
   
   Eventually, I think we would want `Snapshot` to return its own schema rather 
than requiring a lookup, which is why I suggested that option. But we might be 
able to avoid the problem for now.
   
   Like I said, I just don't think we want to add anything to `Table`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to