You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "wangyum (via GitHub)" <gi...@apache.org> on 2023/03/10 06:20:13 UTC
[GitHub] [spark] wangyum opened a new pull request, #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
wangyum opened a new pull request, #40360:
URL: https://github.com/apache/spark/pull/40360
### What changes were proposed in this pull request?
This PR makes `UnwrapCastInBinaryComparison` not to unwrap casts in binary comparison when literal is null.
### Why are the changes needed?
In order to make the logic of `UnwrapCastInBinaryComparison` more clear. Null literals are already handled by `NullPropagation`:
https://github.com/apache/spark/blob/2de0d45887509fac8d5fc9448764a0e71f618797/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala#L823-L824
https://github.com/apache/spark/blob/2de0d45887509fac8d5fc9448764a0e71f618797/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala#L850-L851
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing unit tests.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40360:
URL: https://github.com/apache/spark/pull/40360#issuecomment-1464840038
Thank you for updating.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on PR #40360:
URL: https://github.com/apache/spark/pull/40360#issuecomment-1467294557
cc @cloud-fan
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum closed pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum closed pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
URL: https://github.com/apache/spark/pull/40360
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on a diff in pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on code in PR #40360:
URL: https://github.com/apache/spark/pull/40360#discussion_r1132000965
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparisonSuite.scala:
##########
@@ -192,7 +192,7 @@ class UnwrapCastInBinaryComparisonSuite extends PlanTest with ExpressionEvalHelp
})
}
- test("unwrap casts when literal is null") {
+ test("SPARK-42741: Do not unwrap casts in binary comparison when literal is null") {
Review Comment:
Expressions in this test are optimized by `NullPropagation`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40360:
URL: https://github.com/apache/spark/pull/40360#discussion_r1133022197
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparisonSuite.scala:
##########
@@ -192,7 +192,7 @@ class UnwrapCastInBinaryComparisonSuite extends PlanTest with ExpressionEvalHelp
})
}
- test("unwrap casts when literal is null") {
+ test("SPARK-42741: Do not unwrap casts in binary comparison when literal is null") {
Review Comment:
@wangyum . Although I understand what you mean, this test passed without your patch already.
Could you provide some test case which fails without our patch, @wangyum ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on a diff in pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on code in PR #40360:
URL: https://github.com/apache/spark/pull/40360#discussion_r1133032106
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparisonSuite.scala:
##########
@@ -192,7 +192,7 @@ class UnwrapCastInBinaryComparisonSuite extends PlanTest with ExpressionEvalHelp
})
}
- test("unwrap casts when literal is null") {
+ test("SPARK-42741: Do not unwrap casts in binary comparison when literal is null") {
Review Comment:
OK. Remove `NullPropagation` in this test.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparisonSuite.scala:
##########
@@ -192,7 +192,7 @@ class UnwrapCastInBinaryComparisonSuite extends PlanTest with ExpressionEvalHelp
})
}
- test("unwrap casts when literal is null") {
+ test("SPARK-42741: Do not unwrap casts in binary comparison when literal is null") {
Review Comment:
OK. Removed `NullPropagation` in this test.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on a diff in pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on code in PR #40360:
URL: https://github.com/apache/spark/pull/40360#discussion_r1133032223
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala:
##########
@@ -141,14 +141,14 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// 2. this rule only handles the case when both `fromExp` and value in `in.list` are of numeric
// type.
// 3. this rule doesn't optimize In when `in.list` contains an expression that is not literal.
- case in @ In(Cast(fromExp, toType: NumericType, _, _), list @ Seq(firstLit, _*))
+ case in @ In(Cast(fromExp, toType: NumericType, tz, mode), list @ Seq(firstLit, _*))
if canImplicitlyCast(fromExp, toType, firstLit.dataType) && in.inSetConvertible =>
val buildIn = {
(nullList: ArrayBuffer[Literal], canCastList: ArrayBuffer[Literal]) =>
// cast null value to fromExp.dataType, to make sure the new return list is in the same
// data type.
- val newList = nullList.map(lit => Cast(lit, fromExp.dataType)) ++ canCastList
+ val newList = nullList.map(lit => Cast(lit, fromExp.dataType, tz, mode)) ++ canCastList
Review Comment:
Add `tz`, `mode` makes the test pass.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on pull request #40360: [SPARK-42741][SQL] Do not unwrap casts in binary comparison when literal is null
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on PR #40360:
URL: https://github.com/apache/spark/pull/40360#issuecomment-1468021595
Merged to master.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org