nastra commented on code in PR #5984:
URL: https://github.com/apache/iceberg/pull/5984#discussion_r1179060666
##########
core/src/main/java/org/apache/iceberg/BaseIncrementalScan.java:
##########
@@ -44,6 +52,14 @@ public ThisT fromSnapshotInclusive(long fromSnapshotId) {
return newRefinedScan(table(), schema(), newContext);
}
+ @Override
+ public ThisT fromSnapshotExclusive(String ref) {
+ SnapshotRef snapshotRef = table().refs().get(ref);
+ Preconditions.checkArgument(snapshotRef != null, "Cannot find ref %s",
ref);
Review Comment:
```suggestion
Preconditions.checkArgument(snapshotRef != null, "Cannot find ref: %s",
ref);
```
##########
core/src/main/java/org/apache/iceberg/BaseIncrementalScan.java:
##########
@@ -34,6 +34,14 @@ protected BaseIncrementalScan(Table table, Schema schema,
TableScanContext conte
protected abstract CloseableIterable<T> doPlanFiles(
Long fromSnapshotIdExclusive, long toSnapshotIdInclusive);
+ @Override
+ public ThisT fromSnapshotInclusive(String ref) {
+ SnapshotRef snapshotRef = table().refs().get(ref);
+ Preconditions.checkArgument(snapshotRef != null, "Cannot find ref %s",
ref);
Review Comment:
so that it's consistent with `toSnapshot` further below
##########
core/src/main/java/org/apache/iceberg/BaseIncrementalScan.java:
##########
@@ -34,6 +34,14 @@ protected BaseIncrementalScan(Table table, Schema schema,
TableScanContext conte
protected abstract CloseableIterable<T> doPlanFiles(
Long fromSnapshotIdExclusive, long toSnapshotIdInclusive);
+ @Override
+ public ThisT fromSnapshotInclusive(String ref) {
+ SnapshotRef snapshotRef = table().refs().get(ref);
+ Preconditions.checkArgument(snapshotRef != null, "Cannot find ref %s",
ref);
Review Comment:
```suggestion
Preconditions.checkArgument(snapshotRef != null, "Cannot find ref: %s",
ref);
```
##########
core/src/main/java/org/apache/iceberg/BaseIncrementalScan.java:
##########
@@ -100,9 +132,23 @@ private boolean scanCurrentLineage() {
private long toSnapshotIdInclusive() {
if (context().toSnapshotId() != null) {
+ if (context().branch() != null) {
+ Snapshot currentSnapshot = table().snapshot(context().branch());
+ Preconditions.checkArgument(
+ SnapshotUtil.isAncestorOf(
+ table(), currentSnapshot.snapshotId(),
context().toSnapshotId()),
+ "End snapshot is not a valid snapshot on the current branch");
Review Comment:
should the error msg maybe mention the current branch?
##########
core/src/main/java/org/apache/iceberg/TableScanContext.java:
##########
@@ -96,6 +96,9 @@ public MetricsReporter metricsReporter() {
return LoggingMetricsReporter.instance();
}
+ @Nullable
+ public abstract String branch();
Review Comment:
I think it makes more sense to name this `ref()` because the ref can be
either a branch or a tag
--
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]
