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/01/04 09:24:13 UTC
[GitHub] [iceberg] izchen opened a new pull request #3840: Spark 3.0: Fix delete from snapshot of table
izchen opened a new pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840
Fix UT case testDeleteFromTableAtSnapshot. This UT failed in spark version 3.0. I think this is because the [[SPARK-33623][SQL] Add canDeleteWhere to SupportsDelete](https://github.com/apache/spark/pull/30562) does not exist in 3.0.
--
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] izchen commented on pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#issuecomment-1004647474
Related PR: https://github.com/apache/iceberg/pull/3799 https://github.com/apache/iceberg/pull/3769
--
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] wypoon commented on pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
wypoon commented on pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#issuecomment-1005120666
Apart from one comment, LGTM.
@izchen thanks for finding and fixing 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] wypoon commented on a change in pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
wypoon commented on a change in pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#discussion_r778344448
##########
File path: spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java
##########
@@ -244,6 +244,10 @@ private boolean requiresRewrite(Filter filter, Schema schema, Set<Integer> ident
@Override
public void deleteWhere(Filter[] filters) {
+ Preconditions.checkArgument(
+ snapshotId == null,
+ "Cannot delete from table at a specific snapshot: %s", snapshotId);
+
Review comment:
Can you please remove this same check from `canDeleteWhere` above? That way, the check is present in only one place, the place where it is actually called.
IIUC, Spark 3.0 doesn't actually call `SparkTable#canDeleteWhere` (Spark 3.1 does); we have `canDeleteWhere` in the code here because it was added when we had a single code base for both Spark 3.0 and 3.1.
--
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] izchen commented on pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#issuecomment-1004686327
cc @rdblue @wypoon
--
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] izchen commented on a change in pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#discussion_r778300877
##########
File path: spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java
##########
@@ -244,6 +244,7 @@ private boolean requiresRewrite(Filter filter, Schema schema, Set<Integer> ident
@Override
public void deleteWhere(Filter[] filters) {
+ canDeleteWhere(filters);
Review comment:
@rdblue Thanks for the review!
--
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] rdblue commented on a change in pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#discussion_r778219541
##########
File path: spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java
##########
@@ -244,6 +244,7 @@ private boolean requiresRewrite(Filter filter, Schema schema, Set<Integer> ident
@Override
public void deleteWhere(Filter[] filters) {
+ canDeleteWhere(filters);
Review comment:
This is where the check is done for the other versions of Spark, but adding `canDeleteWhere` here is expensive and not the right solution. Can you add the precondition instead?
--
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] rdblue commented on a change in pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#discussion_r778357745
##########
File path: spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkTable.java
##########
@@ -244,6 +244,10 @@ private boolean requiresRewrite(Filter filter, Schema schema, Set<Integer> ident
@Override
public void deleteWhere(Filter[] filters) {
+ Preconditions.checkArgument(
+ snapshotId == null,
+ "Cannot delete from table at a specific snapshot: %s", snapshotId);
+
Review comment:
I don't think that the implementation of `canDeleteWhere` matters much for Spark 3.0. We could simply remove the function since it is unused, but it doesn't seem worth letting the code drift either to remove it or to modify it to have this check in just one place. So I guess I disagree with this suggestion. Let's just leave it as is.
--
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] rdblue merged pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840
--
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] izchen commented on pull request #3840: Spark 3.0: Fix delete from snapshot of table
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3840:
URL: https://github.com/apache/iceberg/pull/3840#issuecomment-1005459827
Thank you everyone!
--
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