[SPARK-39966][SQL] Use V2 Filter in SupportsDelete #37393
Conversation
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsDeleteV2.java
Outdated
Show resolved
Hide resolved
|
cc @cloud-fan |
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsDelete.java
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsDelete.java
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsDelete.java
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsDelete.java
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsDelete.java
Outdated
Show resolved
Hide resolved
...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala
Show resolved
Hide resolved
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsDeleteV2.java
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/PredicateUtils.scala
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/PredicateUtils.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryBaseTable.scala
Outdated
Show resolved
Hide resolved
...talyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableWithV2Filter.scala
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTable.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/PredicateUtils.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/PredicateUtils.scala
Outdated
Show resolved
Hide resolved
|
@cloud-fan Could you check one more time, please? The test failure is not relevant. |
| def filtersToKeys( | ||
| keys: Iterable[Seq[Any]], | ||
| partitionNames: Seq[String], | ||
| filters: Array[Filter]): Iterable[Seq[Any]] = { |
There was a problem hiding this comment.
This method is currently used by class Overwrite(filters: Array[Filter]) at line 422, so I will leave this method in this base class for now. When I work on my next PR SupportsOverWriteV2, I will clean up this base class, move this method and all the Filter related methods to InMemoryTable.
|
|
||
| def toV1( | ||
| predicates: Array[Predicate], | ||
| skipIfNotConvertible: Boolean): Array[Filter] = { |
There was a problem hiding this comment.
thinking about this more, I think we can just have
def toV1(predicates: Array[Predicate]): Array[Filter] = ...
In SupportsDelete, we can check the number of returned v1 filter
default boolean canDeleteWhere(Predicate[] predicates) {
Filter[] v1Filters = PredicateUtils.toV1(predicates);
if (v1Filters.length < predicates) return false;
return this.canDeleteWhere(v1Filters);
}
default void deleteWhere(Predicate[] predicates) {
this.deleteWhere(PredicateUtils.toV1(predicates));
}
There was a problem hiding this comment.
I actually have thought about this, this works OK for SupportsDelete, but we will have to throw Exception later on for SupportsOverwrite, because we don't have an API canOverwrite.
There was a problem hiding this comment.
shall we add canOverwrite?
There was a problem hiding this comment.
at least we can still throw error if we want to, by checking the number of returned v1 filters
There was a problem hiding this comment.
NVM, I will change to this way, and check if (v1Filters.length < predicates.length) and throw Exception in SupportsOverwrite.overwrite.
There was a problem hiding this comment.
Replied without seeing your previous message.
Will add canOverwrite
|
Merged to master. Thanks @cloud-fan @beliefer @LuciferYang for reviewing! |
What changes were proposed in this pull request?
Migrate SupportsDelete to use V2 Filter
Why are the changes needed?
this is part of the V2Filter migration work
Does this PR introduce any user-facing change?
Yes
add
SupportsDeleteV2How was this patch tested?
new tests