You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/04/08 21:26:34 UTC
[GitHub] [iceberg] stevenzwu opened a new pull request, #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
stevenzwu opened a new pull request, #4529:
URL: https://github.com/apache/iceberg/pull/4529
…apshotId as Long type (instead of current primitive long).
There are two reasons to support null fromSnapshotId
- Support inclusive streaming starting strategy. We can simply pass in the parentId of the starting snapshot, which can be null
- Support incremental streaming scan for a empty table, where the fromSnapshotId could be null
This change is binary compatible. Old interfaces are marked as deprecated.
--
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] stevenzwu commented on pull request #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#issuecomment-1093454915
@rdblue @openinx @yittg @kbendick please take a look when you get a chance
--
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] yittg commented on pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
yittg commented on PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#issuecomment-1094501133
thanks @stevenzwu for providing this change.
Personally, i'm not quite sure should we use a such big change to achieve this target.
I wonder whether it is a better option to use a special invalid snapshot id value to do the same thing, such as `-1`. Checking this special value can be equivalent with null checking.
Or please correct me if it has other effects. Also looking forward others' opinions.
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r846510835
##########
api/src/main/java/org/apache/iceberg/events/IncrementalTableScanEvent.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.events;
+
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.expressions.Expression;
+
+/**
+ * Event sent to listeners when an incremental table scan is planned.
+ */
+public final class IncrementalTableScanEvent {
+ private final String tableName;
+ private final Long fromSnapshotId;
+ private final long toSnapshotId;
+ private final Expression filter;
+ private final Schema projection;
+
+ public IncrementalTableScanEvent(String tableName, Long fromSnapshotId, long toSnapshotId, Expression filter,
+ Schema projection) {
+ this.tableName = tableName;
+ this.fromSnapshotId = fromSnapshotId;
+ this.toSnapshotId = toSnapshotId;
+ this.filter = filter;
+ this.projection = projection;
+ }
+
+ public String tableName() {
+ return tableName;
+ }
+
+ public Long fromSnapshotId() {
Review Comment:
This class is cloned from `IncrementalScanEvent` because we can't change the return type from `Long` to `long` without breaking binary compatibility.
--
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] yittg commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
yittg commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r849125839
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -303,10 +306,8 @@ private List<String> appendsAfterScan(long fromSnapshotId) {
return filesToScan(appendsAfter);
}
- private List<String> appendsBetweenScan(long fromSnapshotId, long toSnapshotId) {
- Snapshot s1 = table.snapshot(fromSnapshotId);
- Snapshot s2 = table.snapshot(toSnapshotId);
- TableScan appendsBetween = table.newScan().appendsBetween(s1.snapshotId(), s2.snapshotId());
+ private List<String> appendsBetweenScan(Long fromSnapshotId, long toSnapshotId) {
+ TableScan appendsBetween = table.newScan().appendsInRange(fromSnapshotId, toSnapshotId);
return filesToScan(appendsBetween);
}
Review Comment:
Are there still cases where startSnapshotId is `null` after reverting?
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r846524714
##########
api/src/main/java/org/apache/iceberg/TableScan.java:
##########
@@ -147,8 +147,25 @@ default TableScan select(String... columns) {
* @param toSnapshotId read append data up to this snapshot id
* @return a table scan which can read append data from {@code fromSnapshotId}
* exclusive and up to {@code toSnapshotId} inclusive
+ *
+ * @deprecated Please use the appendsBetween with non-primitive fromSnapshotId of Long type
+ */
+ @Deprecated
+ default TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ return appendsInRange(Long.valueOf(fromSnapshotId), toSnapshotId);
+ }
+
+ /**
+ * Create a new {@link TableScan} to read appended data from {@code fromSnapshotId} exclusive to {@code toSnapshotId}
+ * inclusive.
+ *
+ * @param fromSnapshotId the last snapshot id read by the user, exclusive. If it is null,
+ * all the ancestor snapshots of the toSnapshotId should be discovered
+ * @param toSnapshotId read append data up to this snapshot id
+ * @return a table scan which can read append data from {@code fromSnapshotId}
+ * exclusive and up to {@code toSnapshotId} inclusive
*/
- TableScan appendsBetween(long fromSnapshotId, long toSnapshotId);
+ TableScan appendsInRange(Long fromSnapshotId, long toSnapshotId);
Review Comment:
Another naming option is `appendsBetweenScan`
--
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] stevenzwu closed pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu closed pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
URL: https://github.com/apache/iceberg/pull/4529
--
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] stevenzwu commented on pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#issuecomment-1095844443
@yittg this proposal is after the discussion with Ryan. The original plan was to provide an overloaded `appendsBetween` with `long` and `Long`. But during implementation, I found that overload caused compiling error as I mentioned in one of the comments. So I have to rename the new method to `appendsInRange`, which is not as good a name as `appendsBetween`. This PR is actually relatively small change even though it touched many files.
Regarding special value, I am not sure if there is a special invalid snapshot id that Iceberg defines in the table format spec. The only special value is `null`. E.g., the earliest snapshot has `parent-snapshot-id` as null.
--
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] chenjunjiedada commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r855006205
##########
api/src/main/java/org/apache/iceberg/TableScan.java:
##########
@@ -147,8 +147,25 @@ default TableScan select(String... columns) {
* @param toSnapshotId read append data up to this snapshot id
* @return a table scan which can read append data from {@code fromSnapshotId}
* exclusive and up to {@code toSnapshotId} inclusive
+ *
+ * @deprecated Please use the appendsBetween with non-primitive fromSnapshotId of Long type
+ */
+ @Deprecated
+ default TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ return appendsInRange(Long.valueOf(fromSnapshotId), toSnapshotId);
+ }
+
+ /**
+ * Create a new {@link TableScan} to read appended data from {@code fromSnapshotId} exclusive to {@code toSnapshotId}
+ * inclusive.
+ *
+ * @param fromSnapshotId the last snapshot id read by the user, exclusive. If it is null,
+ * all the ancestor snapshots of the toSnapshotId should be discovered
Review Comment:
do you mean all ancestor snapshots should be included? right?
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r846524593
##########
api/src/main/java/org/apache/iceberg/TableScan.java:
##########
@@ -147,8 +147,25 @@ default TableScan select(String... columns) {
* @param toSnapshotId read append data up to this snapshot id
* @return a table scan which can read append data from {@code fromSnapshotId}
* exclusive and up to {@code toSnapshotId} inclusive
+ *
+ * @deprecated Please use the appendsBetween with non-primitive fromSnapshotId of Long type
+ */
+ @Deprecated
+ default TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ return appendsInRange(Long.valueOf(fromSnapshotId), toSnapshotId);
+ }
+
+ /**
+ * Create a new {@link TableScan} to read appended data from {@code fromSnapshotId} exclusive to {@code toSnapshotId}
+ * inclusive.
+ *
+ * @param fromSnapshotId the last snapshot id read by the user, exclusive. If it is null,
+ * all the ancestor snapshots of the toSnapshotId should be discovered
+ * @param toSnapshotId read append data up to this snapshot id
+ * @return a table scan which can read append data from {@code fromSnapshotId}
+ * exclusive and up to {@code toSnapshotId} inclusive
*/
- TableScan appendsBetween(long fromSnapshotId, long toSnapshotId);
+ TableScan appendsInRange(Long fromSnapshotId, long toSnapshotId);
Review Comment:
We can't use the same method name. otherwise, we ran into the following resolution error for method overloading.
```
/home/runner/work/iceberg/iceberg/spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/source/Reader.java:172: error: reference to appendsBetween is ambiguous
scan = scan.appendsBetween(startSnapshotId, endSnapshotId);
^
both method appendsBetween(long,long) in org.apache.iceberg.TableScan and method appendsBetween(java.lang.Long,long) in org.apache.iceberg.TableScan match
```
--
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] chenjunjiedada commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r855710791
##########
core/src/main/java/org/apache/iceberg/BaseAllMetadataTableScan.java:
##########
@@ -40,7 +40,7 @@ abstract class BaseAllMetadataTableScan extends BaseMetadataTableScan {
protected abstract String tableType();
@Override
- public TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ public TableScan appendsInRange(Long fromSnapshotId, long toSnapshotId) {
Review Comment:
Cool, let me take a 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] chenjunjiedada commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r855009099
##########
core/src/main/java/org/apache/iceberg/BaseAllMetadataTableScan.java:
##########
@@ -40,7 +40,7 @@ abstract class BaseAllMetadataTableScan extends BaseMetadataTableScan {
protected abstract String tableType();
@Override
- public TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ public TableScan appendsInRange(Long fromSnapshotId, long toSnapshotId) {
Review Comment:
In the future, we want to support reading overwrite snapshots so maybe we could change to `dataBetween(startSnaphotId, endSnasphotId)`.
--
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] yittg commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
yittg commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r849957631
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -303,10 +306,8 @@ private List<String> appendsAfterScan(long fromSnapshotId) {
return filesToScan(appendsAfter);
}
- private List<String> appendsBetweenScan(long fromSnapshotId, long toSnapshotId) {
- Snapshot s1 = table.snapshot(fromSnapshotId);
- Snapshot s2 = table.snapshot(toSnapshotId);
- TableScan appendsBetween = table.newScan().appendsBetween(s1.snapshotId(), s2.snapshotId());
+ private List<String> appendsBetweenScan(Long fromSnapshotId, long toSnapshotId) {
+ TableScan appendsBetween = table.newScan().appendsInRange(fromSnapshotId, toSnapshotId);
return filesToScan(appendsBetween);
}
Review Comment:
it's better now from my side. thanks
--
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] yittg commented on pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
yittg commented on PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#issuecomment-1095870154
@stevenzwu Thanks for your explanation and great work.
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r855330008
##########
core/src/main/java/org/apache/iceberg/BaseAllMetadataTableScan.java:
##########
@@ -40,7 +40,7 @@ abstract class BaseAllMetadataTableScan extends BaseMetadataTableScan {
protected abstract String tableType();
@Override
- public TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ public TableScan appendsInRange(Long fromSnapshotId, long toSnapshotId) {
Review Comment:
if the community agrees with PR #4580 , we will abort this PR/approach. In PR #4580 , we can add more flexible `IncrementalScan` options (not just append) to support use cases like CDC read.
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r846528568
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -60,7 +60,7 @@ public void testInvalidScans() {
AssertHelpers.assertThrows(
"from and to snapshots cannot be the same, since from snapshot is exclusive and not part of the scan",
IllegalArgumentException.class, "from and to snapshot ids cannot be the same",
- () -> appendsBetweenScan(1, 1));
+ () -> appendsBetweenScan(1L, 1L));
Review Comment:
need to change the `fromSnapshotId` from `1` to `1L` because the type is `Long` now. there is no auto type conversion from `int` to `Long`.
Since I am changing the constants for `fromSnapshotId`, I also changed the `toSnapshotId` as well to be consistent.
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r849079014
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -303,10 +306,8 @@ private List<String> appendsAfterScan(long fromSnapshotId) {
return filesToScan(appendsAfter);
}
- private List<String> appendsBetweenScan(long fromSnapshotId, long toSnapshotId) {
- Snapshot s1 = table.snapshot(fromSnapshotId);
- Snapshot s2 = table.snapshot(toSnapshotId);
- TableScan appendsBetween = table.newScan().appendsBetween(s1.snapshotId(), s2.snapshotId());
+ private List<String> appendsBetweenScan(Long fromSnapshotId, long toSnapshotId) {
+ TableScan appendsBetween = table.newScan().appendsInRange(fromSnapshotId, toSnapshotId);
return filesToScan(appendsBetween);
}
Review Comment:
This change seems unnecessary. let me try to revert it and minimize the change in this test class
--
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] yittg commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
yittg commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r849068303
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -303,10 +306,8 @@ private List<String> appendsAfterScan(long fromSnapshotId) {
return filesToScan(appendsAfter);
}
- private List<String> appendsBetweenScan(long fromSnapshotId, long toSnapshotId) {
- Snapshot s1 = table.snapshot(fromSnapshotId);
- Snapshot s2 = table.snapshot(toSnapshotId);
- TableScan appendsBetween = table.newScan().appendsBetween(s1.snapshotId(), s2.snapshotId());
+ private List<String> appendsBetweenScan(Long fromSnapshotId, long toSnapshotId) {
+ TableScan appendsBetween = table.newScan().appendsInRange(fromSnapshotId, toSnapshotId);
return filesToScan(appendsBetween);
}
Review Comment:
How about adding another method, and keeping the original?
```suggestion
private List<String> appendsBeforeScan(long toSnapshotId) {
TableScan appendsBetween = table.newScan().appendsInRange(null, toSnapshotId);
return filesToScan(appendsBetween);
}
private List<String> appendsBetweenScan(long fromSnapshotId, long toSnapshotId) {
TableScan appendsBetween = table.newScan().appendsInRange(fromSnapshotId, toSnapshotId);
return filesToScan(appendsBetween);
}
```
--
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] stevenzwu commented on pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#issuecomment-1113659475
close this one in favor of PR #4580
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r846528568
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -60,7 +60,7 @@ public void testInvalidScans() {
AssertHelpers.assertThrows(
"from and to snapshots cannot be the same, since from snapshot is exclusive and not part of the scan",
IllegalArgumentException.class, "from and to snapshot ids cannot be the same",
- () -> appendsBetweenScan(1, 1));
+ () -> appendsBetweenScan(1L, 1L));
Review Comment:
Since we changed the arg type from long to Long for `fromSnapshotId` param in `appendsBetweenScan` method, we need to change the from `1` to `1L`. there is no auto type conversion from `int` to `Long`.
Since I am changing the constants for `fromSnapshotId`, I also changed the `toSnapshotId` as well to be consistent.
--
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] stevenzwu commented on pull request #4529: Add a new TableScan#appendBetween method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#issuecomment-1093402576
We can't really overload the `appendBetween` with long and Long. Otherwise, we are going to see compiling error (Ambiguous) like this.
```
> Task :iceberg-flink:iceberg-flink-1.14:compileJava
scan = scan.appendsBetween(context.startSnapshotId(), context.endSnapshotId());
^
both method appendsBetween(long,long) in org.apache.iceberg.TableScan and method appendsBetween(java.lang.Long,long) in org.apache.iceberg.TableScan match
```
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r855331211
##########
api/src/main/java/org/apache/iceberg/TableScan.java:
##########
@@ -147,8 +147,25 @@ default TableScan select(String... columns) {
* @param toSnapshotId read append data up to this snapshot id
* @return a table scan which can read append data from {@code fromSnapshotId}
* exclusive and up to {@code toSnapshotId} inclusive
+ *
+ * @deprecated Please use the appendsBetween with non-primitive fromSnapshotId of Long type
+ */
+ @Deprecated
+ default TableScan appendsBetween(long fromSnapshotId, long toSnapshotId) {
+ return appendsInRange(Long.valueOf(fromSnapshotId), toSnapshotId);
+ }
+
+ /**
+ * Create a new {@link TableScan} to read appended data from {@code fromSnapshotId} exclusive to {@code toSnapshotId}
+ * inclusive.
+ *
+ * @param fromSnapshotId the last snapshot id read by the user, exclusive. If it is null,
+ * all the ancestor snapshots of the toSnapshotId should be discovered
Review Comment:
correct. let's say the snapshot history is Sn -> Sn+1 -> Sn+2 -> Sn+3. `appendsBetween(null, Sn+3) should discover files appended by all 4 snapshots.
--
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] stevenzwu commented on a diff in pull request #4529: Add a new TableScan#appendInRange method that accepts nullable fromSn…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on code in PR #4529:
URL: https://github.com/apache/iceberg/pull/4529#discussion_r849783919
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -303,10 +306,8 @@ private List<String> appendsAfterScan(long fromSnapshotId) {
return filesToScan(appendsAfter);
}
- private List<String> appendsBetweenScan(long fromSnapshotId, long toSnapshotId) {
- Snapshot s1 = table.snapshot(fromSnapshotId);
- Snapshot s2 = table.snapshot(toSnapshotId);
- TableScan appendsBetween = table.newScan().appendsBetween(s1.snapshotId(), s2.snapshotId());
+ private List<String> appendsBetweenScan(Long fromSnapshotId, long toSnapshotId) {
+ TableScan appendsBetween = table.newScan().appendsInRange(fromSnapshotId, toSnapshotId);
return filesToScan(appendsBetween);
}
Review Comment:
Not in the existing unit test of `TestIncrementalDataTableScan`. But this reminds me that maybe we should add unit test with null startSnapshotId for the `appendsInRange`. But we do not need to change this method. if there is a need, we can create a new method as you suggested.
--
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