wypoon commented on a change in pull request #1508:
URL: https://github.com/apache/iceberg/pull/1508#discussion_r717105699
##########
File path: core/src/main/java/org/apache/iceberg/BaseTableScan.java
##########
@@ -123,8 +124,14 @@ public TableScan useSnapshot(long scanSnapshotId) {
"Cannot override snapshot, already set to id=%s",
context.snapshotId());
Preconditions.checkArgument(ops.current().snapshot(scanSnapshotId) != null,
"Cannot find snapshot with ID %s", scanSnapshotId);
- return newRefinedScan(
- ops, table, schema, context.useSnapshotId(scanSnapshotId));
+ if (this instanceof DataTableScan) {
Review comment:
Q. (In your example) why check if `newScan.snapshot().schemaId() !=
null`? We can simply call `SnapshotUtil.schemaFor(table(), scanSnapshotId)`
without checking, since the util method currently returns the table schema as a
fallback, pending a TODO to recover the schema for snapshot by reading old
metadata. This way, once that TODO is implemented, we don't need to revisit
this code.
We don't need `newScan` at all; the only reason for it would be to call
`super.useSnapshot` for its side-effects (argument validation), which I prefer
to avoid.
--
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
