[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #5984: Core, API: Support incremental scanning with branch

["amogh-jahagirdar (via GitHub)" <gi...@apache.org>]

amogh-jahagirdar commented on code in PR #5984:
URL: https://github.com/apache/iceberg/pull/5984#discussion_r1016157787


##########
api/src/main/java/org/apache/iceberg/IncrementalScan.java:
##########
@@ -33,6 +34,18 @@
    */
   ThisT fromSnapshotInclusive(long fromSnapshotId);
 
+  /**
+   * Instructs this scan to look for changes starting from a particular snapshot (inclusive).
+   *
+   * <p>If the start snapshot is not configured, it is defaulted to the oldest ancestor of the end
+   * snapshot (inclusive).

Review Comment:
   Nit: Imo bit more concise if we substitute "it is defaulted to" with "it defaults to"



##########
api/src/main/java/org/apache/iceberg/IncrementalScan.java:
##########
@@ -33,6 +34,18 @@
    */
   ThisT fromSnapshotInclusive(long fromSnapshotId);
 
+  /**
+   * Instructs this scan to look for changes starting from a particular snapshot (inclusive).
+   *
+   * <p>If the start snapshot is not configured, it is defaulted to the oldest ancestor of the end
+   * snapshot (inclusive).
+   *
+   * @param fromSnapshotRef the start ref name that points to a particular snapshot ID (inclusive)
+   * @return this for method chaining
+   * @throws IllegalArgumentException if the start snapshot is not an ancestor of the end snapshot
+   */
+  ThisT fromSnapshotInclusive(String fromSnapshotRef);

Review Comment:
   I think we should just call this `ref` the method name already indicates it's "from"



##########
api/src/main/java/org/apache/iceberg/IncrementalScan.java:
##########
@@ -45,6 +58,18 @@
    */
   ThisT fromSnapshotExclusive(long fromSnapshotId);
 
+  /**
+   * Instructs this scan to look for changes starting from a particular snapshot (exclusive).
+   *
+   * <p>If the start snapshot is not configured, it is defaulted to the oldest ancestor of the end

Review Comment:
   Same as above on "it defaults to"



##########
core/src/main/java/org/apache/iceberg/TableScanContext.java:
##########
@@ -45,6 +45,7 @@ final class TableScanContext {
   private final ExecutorService planExecutor;
   private final boolean fromSnapshotInclusive;
   private final MetricsReporter metricsReporter;
+  private final String ref;

Review Comment:
   I think we should be explicit here and call it `branch`, this ref should always be a branch.



##########
api/src/main/java/org/apache/iceberg/IncrementalScan.java:
##########
@@ -55,4 +80,23 @@
    * @return this for method chaining
    */
   ThisT toSnapshot(long toSnapshotId);
+
+  /**
+   * Instructs this scan to look for changes up to a particular snapshot ref (inclusive).
+   *
+   * <p>If the end snapshot is not configured, it is defaulted to the current table snapshot
+   * (inclusive).
+   *
+   * @param fromSnapshotRef the end snapshot Ref (inclusive)
+   * @return this for method chaining
+   */
+  ThisT toSnapshot(String fromSnapshotRef);

Review Comment:
   I think we should just call this `ref` the method name already indicates it's "to"



-- 
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-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org