Core, API: Support scanning from refs #5364

amogh-jahagirdar · 2022-07-27T01:32:38Z

This change adds support for scanning from refs, where the scan can either be a time travel from a tag or a time travel from the tip of a branch.

api/src/main/java/org/apache/iceberg/TableScan.java

core/src/main/java/org/apache/iceberg/DataTableScan.java

core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java

core/src/main/java/org/apache/iceberg/DataTableScan.java

.palantir/revapi.yml

amogh-jahagirdar · 2022-08-26T01:02:39Z

Discussed offline with @rdblue , I got an understanding of how history entries are different than the snapshot ancestor lineage, posting the information here so folks referring to the PR have context:

"History is the state of main at any given time. That can be changed, much like master in git can be changed.
Ancestors can't be changed."

The current main can change when rollback is performed. The snapshot which gets resolved for a given time T should remain constant regardless of what is the current main ancestor lineage. History entries capture all the changes on the main table state and are what enable this ability to time travel to a snapshot which are outside the current table ancestors which is ultimately desired behavior. It also doesn't make sense to treat main as a separate case, and have a different semantic for non-main branches when it comes to time travel.

Maybe we could maintain separate history metadata for branches, but most likely it would need to be opt-in by users for specific branches and would need to not add too much metadata to manage.

So for now, it makes sense just to do time travel to a branch tip or time travel to a given tag. Updated the PR @rdblue @namrathamyske @jackye1995

…tag, and scanning from branch tips

jackye1995

Overall looks good to me. However I am a bit concerned about the fact that we cannot do time travel in branches, I see that as a pretty important feature for many use cases. I agree with the argument of the difference of tracing ancestor vs history entry, but in that case it seems like we need to update the spec to accommodate that change and record history entries in a way that allows time travel by branch.

@amogh-jahagirdar I think after this PR we should raise another PR against spec to discuss what is the best way to solve the time travel by branch use case.

jackye1995 · 2022-09-23T21:25:31Z

api/src/main/java/org/apache/iceberg/TableScan.java

@@ -39,13 +39,25 @@ public interface TableScan extends Scan<TableScan, FileScanTask, CombinedScanTas
   */
  TableScan useSnapshot(long snapshotId);

+  /**
+   * Create a new {@link TableScan} from this scan's configuration that will use the given
+   * reference.


nit: use the snapshot ID of the given reference?

jackye1995 · 2022-09-23T21:26:05Z

api/src/main/java/org/apache/iceberg/TableScan.java

   *
   * @param timestampMillis a timestamp in milliseconds.
   * @return a new scan based on this with the current snapshot at the given time
-   * @throws IllegalArgumentException if the snapshot cannot be found
+   * @throws IllegalArgumentException if the snapshot cannot be found or time travel is attempted on


I think the doc change here is no longer needed?

jackye1995 · 2022-09-23T21:26:30Z

core/src/main/java/org/apache/iceberg/BaseTableScan.java

@@ -85,7 +85,7 @@ public TableScan appendsAfter(long fromSnapshotId) {
  @Override
  public TableScan useSnapshot(long scanSnapshotId) {
    Preconditions.checkArgument(
-        snapshotId() == null, "Cannot override snapshot, already set to id=%s", snapshotId());
+        snapshotId() == null, "Cannot override snapshot, already set snapshot id=%s", snapshotId());


nit: "set to snapshot id=%s"?

rdblue · 2022-10-02T23:43:29Z

core/src/main/java/org/apache/iceberg/IncrementalDataTableScan.java

@@ -46,6 +46,14 @@ public TableScan asOfTime(long timestampMillis) {
            timestampMillis, context().fromSnapshotId(), context().toSnapshotId()));
  }

+  @Override
+  public TableScan useRef(String ref) {
+    throw new UnsupportedOperationException(


This seems fine for now, but we may have a case where we want incremental to be able to specify a branch. It doesn't matter currently because the snapshot IDs are always explicit.

rdblue · 2022-10-02T23:46:55Z

Looks great. Thanks, @amogh-jahagirdar! I'll merge when tests are passing. I had to fix a conflict in revapi.

amogh-jahagirdar · 2022-10-03T17:43:37Z

Thanks @rdblue @yzhaoqin @namrathamyske @hililiwei for the review!

github-actions bot added API core labels Jul 27, 2022

amogh-jahagirdar force-pushed the scan-from-branch branch 3 times, most recently from f6aa023 to 9393ade Compare July 27, 2022 02:06

jackye1995 reviewed Jul 27, 2022

View reviewed changes

api/src/main/java/org/apache/iceberg/TableScan.java Outdated Show resolved Hide resolved