flyrain commented on code in PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#discussion_r1284045051
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkScanBuilder.java:
##########
@@ -528,12 +529,20 @@ public Scan buildChangelogScan() {
SparkReadOptions.END_TIMESTAMP);
}
+ boolean emptyScan = false;
if (startTimestamp != null) {
startSnapshotId = getStartSnapshotId(startTimestamp);
+ if (startSnapshotId == null && endTimestamp == null) {
+ emptyScan = true;
+ }
}
if (endTimestamp != null) {
- endSnapshotId = SnapshotUtil.snapshotIdAsOfTime(table, endTimestamp);
+ endSnapshotId = SnapshotUtil.snapshotIdAsOfTime(table, endTimestamp,
true);
+ if ((startSnapshotId == null && endSnapshotId == null)
+ || (startSnapshotId != null &&
startSnapshotId.equals(endSnapshotId))) {
+ emptyScan = true;
+ }
Review Comment:
I'd suggest a new method to generate a snapshot range from the time range
when time range is used(there is at least one time range options). This method
can return either a valid snapshot range, or empty range. In case of the empty
range, we create an empty scan.
##########
core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java:
##########
@@ -345,7 +349,7 @@ public static long snapshotIdAsOfTime(Table table, long
timestampMillis) {
}
Preconditions.checkArgument(
- snapshotId != null,
+ snapshotId != null || nullable,
"Cannot find a snapshot older than %s",
DateTimeUtil.formatTimestampMillis(timestampMillis));
return snapshotId;
Review Comment:
Not a fan of flag parameters. How about something like this? I assume this
is more readable as welll
```
public static long snapshotIdAsOfTime(Table table, long timestampMillis) {
Long snapshotId = nullableSnapshotIdAsOfTime(table, timestampMillis);
Preconditions.checkArgument(snapshotId != null, "Cannot find a snapshot
older than %s",
DateTimeUtil.formatTimestampMillis(timestampMillis));
return snapshotId;
}
public static Long nullableSnapshotIdAsOfTime(Table table, long
timestampMillis) {
Long snapshotId = null;
for (HistoryEntry logEntry : table.history()) {
if (logEntry.timestampMillis() <= timestampMillis) {
snapshotId = logEntry.snapshotId();
}
}
return snapshotId;
}
```
--
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]
