kbendick commented on a change in pull request #1421:
URL: https://github.com/apache/iceberg/pull/1421#discussion_r510408804
##########
File path: core/src/main/java/org/apache/iceberg/BaseTableScan.java
##########
@@ -105,6 +109,14 @@ protected abstract TableScan newRefinedScan(
TableOperations ops, Snapshot snapshot, Expression rowFilter,
boolean ignoreResiduals, boolean caseSensitive, boolean colStats);
+ public Long fromSnapshotId() {
+ return context().fromSnapshotId();
+ }
+
+ public Long toSnapshotId() {
+ return context.toSnapshotId();
+ }
Review comment:
👍
##########
File path: api/src/main/java/org/apache/iceberg/DataFile.java
##########
@@ -80,7 +80,8 @@ static StructType getType(StructType partitionType) {
UPPER_BOUNDS,
KEY_METADATA,
SPLIT_OFFSETS,
- EQUALITY_IDS
+ EQUALITY_IDS,
+ ManifestFile.SPEC_ID.asOptional()
Review comment:
Should we document why this is optional @RussellSpitzer or do we prefer
to leave the code more pristine and maybe document elsewhere. Definitely
something I would imagine forgetting over time.
##########
File path: core/src/main/java/org/apache/iceberg/BaseFile.java
##########
@@ -100,6 +100,10 @@ public PartitionData copy() {
found = true;
fromProjectionPos[i] = j;
}
+ if (fields.get(i).fieldId() == ManifestFile.SPEC_ID.fieldId()) {
+ found = true;
+ fromProjectionPos[i] = 14;
+ }
Review comment:
This is not related to your PR but while we're here: once we find the
projected value and `found` is true, do we need to iterate over the rest of the
entries?
##########
File path: core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java
##########
@@ -107,4 +112,47 @@ public static boolean ancestorOf(Table table, long
snapshotId, long ancestorSnap
return newFiles;
}
+
+ public static List<Snapshot> snapshotsWithin(Table table, long
fromSnapshotId, long toSnapshotId) {
Review comment:
Can we put a TODO for the doc or potentially mark this protected? I
don't want to block your PR because you extracted it from elsewhere where it
was already private, but it would be nice to have a way to mark this with `//
Consider snapshotIdsBetween` in one way or another. But I don't want to block
your PR so if you'll come back to it later then that's ok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
