Spark 3.0: Fix delete from snapshot of table #3840
Conversation
| @@ -244,6 +244,7 @@ private boolean requiresRewrite(Filter filter, Schema schema, Set<Integer> ident | |||
|
|
|||
| @Override | |||
| public void deleteWhere(Filter[] filters) { | |||
| canDeleteWhere(filters); | |||
There was a problem hiding this 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?
There was a problem hiding this comment.
@rdblue Thanks for the review!
| Preconditions.checkArgument( | ||
| snapshotId == null, | ||
| "Cannot delete from table at a specific snapshot: %s", snapshotId); | ||
|
|
There was a problem hiding this 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.
There was a problem hiding this 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.
|
Apart from one comment, LGTM. |
|
Thank you everyone! |
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 does not exist in 3.0.