[SPARK-47463][SQL] Use V2Predicate to wrap expression with return type of boolean #45589
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@beliefer @cloud-fan could you please take a look? |
Member
Author
|
@beliefer @cloud-fan sorry to bother you again, could you please take a look if you have time? |
Member
Author
|
The |
Contributor
|
With hindsight, we shouldn't create the v2 It's too late to change the API now. One suggestion is to follow how we handle CASE WHEN: always use v2 |
cloud-fan
reviewed
Apr 11, 2024
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2Suite.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Apr 11, 2024
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2Suite.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Apr 11, 2024
| None | ||
| } | ||
| case iff: If => generateExpressionWithName("CASE_WHEN", iff.children) | ||
| case iff: If => |
Contributor
There was a problem hiding this comment.
can you check other expressions matched here and see if they may return boolean type?
Member
Author
There was a problem hiding this comment.
can you check other expressions matched here and see if they may return boolean type?
there is also Coalesce.
cloud-fan
reviewed
Apr 11, 2024
| } | ||
| } | ||
|
|
||
| private def generatePredicateWithName( |
Contributor
There was a problem hiding this comment.
one idea: can we update generateExpressionWithName to take the target expression instead of the children? Then we can simply do
val children = e.children
...
if (d.dataType == BooleanType && isPredicate) {
new V2Predicate ...
} else {
new GeneralScalarExpression
}
Member
Author
There was a problem hiding this comment.
sounds good, I will try that.
wForget
commented
Apr 11, 2024
| case unaryMinus @ UnaryMinus(_, true) => | ||
| generateExpressionWithName("-", unaryMinus, isPredicate) | ||
| case _: BitwiseNot => generateExpressionWithName("~", expr, isPredicate) | ||
| case CaseWhen(branches, elseValue) => |
Member
Author
There was a problem hiding this comment.
CaseWhen always seems to return V2Predicate, is this correct?
Contributor
There was a problem hiding this comment.
I don't think so, but this probably doesn't hurt as V2Predicate extends GeneralScalarExpression. We should still fix it to make code clearer.
wForget
changed the title
wForget
commented
Apr 12, 2024
| generateExpressionWithName("-", unaryMinus, isPredicate) | ||
| case _: BitwiseNot => generateExpressionWithName("~", expr, isPredicate) | ||
| case caseWhen @ CaseWhen(branches, elseValue) => | ||
| val conditions = branches.map(_._1).flatMap(generateExpression(_, true)) |
Member
Author
There was a problem hiding this comment.
I reserved isPredicate=true for conditions of casewhen
cloud-fan
reviewed
Apr 12, 2024
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Apr 12, 2024
sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Apr 12, 2024
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2Suite.scala
Outdated
Show resolved
Hide resolved
cloud-fan
reviewed
Apr 12, 2024
| val df1 = sql( | ||
| s""" | ||
| |select * from | ||
| |(select if(i = 1, i, 0) as c from t1) t |
Contributor
There was a problem hiding this comment.
I think this test is sufficient. If can be a pedicate and before this PR we don't return V2Predicate which causes errors.
Member
Author
There was a problem hiding this comment.
I think this test is sufficient.
Ifcan be a pedicate and before this PR we don't return V2Predicate which causes errors.
makes sense to me
cloud-fan
reviewed
Apr 15, 2024
| s""" | ||
| |select * from | ||
| |(select if(i = 1, i, 0) as c from t1) t | ||
| |where t.c > 0 |
Contributor
There was a problem hiding this comment.
I think where if(i = 1, i, 0) > 0 is a valid SQL? BTW, please upper case the SQL keywords in the SQL statement.
Member
Author
There was a problem hiding this comment.
I think
where if(i = 1, i, 0) > 0is a valid SQL? BTW, please upper case the SQL keywords in the SQL statement.
Indeed, thank you. I have changed.
cloud-fan
approved these changes
Apr 15, 2024
Contributor
|
thanks, merging to master! |
Contributor
|
@wForget can you help to create a 3.5 backport PR? thanks! |
Member
Author
Sure, I will create it as soon as possible, and thanks for your review. |
cloud-fan
pushed a commit
that referenced
this pull request
Apr 18, 2024
…n type of boolean Backports #45589 to 3.5 ### What changes were proposed in this pull request? Use V2Predicate to wrap If expr when building v2 expressions. ### Why are the changes needed? The `PushFoldableIntoBranches` optimizer may fold predicate into (if / case) branches and `V2ExpressionBuilder` wraps `If` as `GeneralScalarExpression`, which causes the assertion in `PushablePredicate.unapply` to fail. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? added unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes #46074 from wForget/SPARK-47463_3.5. Authored-by: Zhen Wang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
|
I encountered a similar issue, I am using |
|
@wForget could you please take a look |
Member
Author
Can you provide a sql to reproduce? |
Contributor
|
I finally have been able to reproduce it: Note that the exception is only thrown when the source is an iceberg table. |
I am not sure! The issue with this query is with the #48621 seems to solve an issue with |
Contributor
|
late LGTM. |
turboFei
pushed a commit
to turboFei/spark
that referenced
this pull request
Nov 6, 2025
…n type of boolean (apache#381) Backports apache#45589 to 3.5 ### What changes were proposed in this pull request? Use V2Predicate to wrap If expr when building v2 expressions. ### Why are the changes needed? The `PushFoldableIntoBranches` optimizer may fold predicate into (if / case) branches and `V2ExpressionBuilder` wraps `If` as `GeneralScalarExpression`, which causes the assertion in `PushablePredicate.unapply` to fail. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? added unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#46074 from wForget/SPARK-47463_3.5. Authored-by: Zhen Wang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> Co-authored-by: Zhen Wang <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Use V2Predicate to wrap If expr when building v2 expressions.
Why are the changes needed?
The
PushFoldableIntoBranchesoptimizer may fold predicate into (if / case) branches andV2ExpressionBuilderwrapsIfasGeneralScalarExpression, which causes the assertion inPushablePredicate.unapplyto fail.Does this PR introduce any user-facing change?
No
How was this patch tested?
added unit test
Was this patch authored or co-authored using generative AI tooling?
No