You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "manuzhang (via GitHub)" <gi...@apache.org> on 2023/07/22 15:20:13 UTC
[GitHub] [iceberg] manuzhang opened a new pull request, #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
manuzhang opened a new pull request, #8133:
URL: https://github.com/apache/iceberg/pull/8133
(no comment)
--
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
[GitHub] [iceberg] flyrain commented on pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#issuecomment-1658922298
Will take a close look this week. Thanks @manuzhang for working on this.
--
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
[GitHub] [iceberg] flyrain commented on a diff in pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#discussion_r1284724837
##########
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:
We should check that even endTimestamp is null, right? Currently, it only checks when endTimestamp != null. With that, we can move the check out of the range conversion. WDYT?
--
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
[GitHub] [iceberg] manuzhang commented on pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#issuecomment-1678741183
@flyrain please take another look.
--
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
[GitHub] [iceberg] flyrain commented on a diff in pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
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: 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
[GitHub] [iceberg] manuzhang commented on pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#issuecomment-1657456477
@aokolnychyi @flyrain Could you help review this? The requirement is coming from our customers whose upstream streaming writes could be unstable and pause for some time.
--
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
[GitHub] [iceberg] manuzhang commented on a diff in pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#discussion_r1284111730
##########
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:
However, we need to both check snapshot and timestamp to decide whether we create an empty scan. There's also a case where `startSnapshotId.equals(endSnapshotId)`.
--
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
[GitHub] [iceberg] flyrain commented on a diff in pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#discussion_r1295268496
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkScanBuilder.java:
##########
@@ -535,12 +537,22 @@ public Scan buildChangelogScan() {
SparkReadOptions.END_TIMESTAMP);
}
- if (startTimestamp != null) {
- startSnapshotId = getStartSnapshotId(startTimestamp);
- }
+ boolean emptyScan = false;
+ if (startTimestamp != null || endTimestamp != null) {
+ Pair<Long, Long> snapshots = getSnapshotsFromTimeRange(startTimestamp, endTimestamp);
Review Comment:
It seems not worth to have a separated method. Would suggest to revert this part of change.
--
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
[GitHub] [iceberg] manuzhang commented on pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#issuecomment-1667274689
@flyrain Please help review again and check whether that's what you have in your mind.
--
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
[GitHub] [iceberg] manuzhang commented on pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#issuecomment-1647133379
@aokolnychyi @flyrain Please kindly review and suggest.
--
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
[GitHub] [iceberg] flyrain commented on a diff in pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#discussion_r1284731129
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkChangelogScan.java:
##########
@@ -66,7 +66,8 @@ class SparkChangelogScan implements Scan, SupportsReportStatistics {
IncrementalChangelogScan scan,
SparkReadConf readConf,
Schema expectedSchema,
- List<Expression> filters) {
+ List<Expression> filters,
+ boolean emptyScan) {
Review Comment:
An alternative way is to pass a null scan object to avoid an additional parameter, but either works for me.
--
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
[GitHub] [iceberg] manuzhang commented on a diff in pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #8133:
URL: https://github.com/apache/iceberg/pull/8133#discussion_r1295329346
##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkScanBuilder.java:
##########
@@ -535,12 +537,22 @@ public Scan buildChangelogScan() {
SparkReadOptions.END_TIMESTAMP);
}
- if (startTimestamp != null) {
- startSnapshotId = getStartSnapshotId(startTimestamp);
- }
+ boolean emptyScan = false;
+ if (startTimestamp != null || endTimestamp != null) {
+ Pair<Long, Long> snapshots = getSnapshotsFromTimeRange(startTimestamp, endTimestamp);
Review Comment:
reverted
--
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
[GitHub] [iceberg] flyrain merged pull request #8133: Spark 3.4: Return empty changed rows when there are no snapshots between start time and end time
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #8133:
URL: https://github.com/apache/iceberg/pull/8133
--
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