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: [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]
