You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/09/16 19:07:47 UTC
[GitHub] [spark] sunchao opened a new pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
sunchao opened a new pull request #29775:
URL: https://github.com/apache/spark/pull/29775
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This is a follow-up on #29565, and addresses a few issues in the last PR:
- style issue pointed by [this comment](https://github.com/apache/spark/pull/29565#discussion_r487646749)
- skip optimization when `fromExp` is foldable (by [this comment](https://github.com/apache/spark/pull/29565#discussion_r487646973)) as there could be more efficient rule to apply for this case.
- pass timezone info to the generated cast on the literal value
- a bunch of cleanups and test improvements
Originally I plan to handle this when implementing [SPARK-32858](https://issues.apache.org/jira/browse/SPARK-32858) but now think it's better to isolate these changes from that.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
To fix a few left over issues in the above PR.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Added a test for the foldable case. Otherwise relying on existing 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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693603760
----------------------------------------------------------------
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.
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 #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693693944
Thank you for this follow-up and pinging me, @sunchao .
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694006907
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694180565
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128802/
Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694180553
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693603760
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693967730
**[Test build #128799 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128799/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694184485
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694025625
----------------------------------------------------------------
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.
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] sunchao commented on a change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489792562
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
Thanks @dongjoon-hyun . I discovered this in tests - without this some of the existing tests will fail because of timezone mismatch (although I think this shouldn't affect correctness given we only handle integral types?).
In the follow-up, the current plan is to only extend to numeric types (e.g., float/double/decimal). I don't know whether support for time-related types is important or not.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694006907
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694180553
----------------------------------------------------------------
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.
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] sunchao commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693687162
cc @cloud-fan and @dongjoon-hyun
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693967730
**[Test build #128799 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128799/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694024027
**[Test build #128802 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128802/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693606720
**[Test build #128778 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128778/testReport)** for PR 29775 at commit [`4291ec2`](https://github.com/apache/spark/commit/4291ec23a9516ff0922f1f65858bfb5d7c34aafc).
----------------------------------------------------------------
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.
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] cloud-fan commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694016591
retest this please
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694025625
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694184485
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693731225
**[Test build #128778 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128778/testReport)** for PR 29775 at commit [`4291ec2`](https://github.com/apache/spark/commit/4291ec23a9516ff0922f1f65858bfb5d7c34aafc).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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 change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489787499
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
It's hard for me to think a valid test case for this in the scope of SPARK-24994 (Integral Types). Although Spark handles some time-related types with the integral types, I believe we need to revisit this on new PR which aims to support time.
----------------------------------------------------------------
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.
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] sunchao commented on a change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489706969
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -184,7 +185,7 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
} else {
// This means `value` is within range `(min, max)`. Optimize this by moving the cast to the
// literal side.
- val lit = Cast(Literal(value), fromType)
+ val lit = Cast(Literal(value), fromType, tz)
Review comment:
We can also evaluate this and pass the value. It will be optimized away by ConstantFolding anyway.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694179239
**[Test build #128802 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128802/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694006919
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/128799/
Test FAILed.
----------------------------------------------------------------
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.
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 closed pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #29775:
URL: https://github.com/apache/spark/pull/29775
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694024027
**[Test build #128802 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128802/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693969011
----------------------------------------------------------------
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.
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 change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489786289
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
This looks like irrelevant as a follow-up.
Is there a comment on this? `pass timezone info to the generated cast on the literal value`?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
This looks irrelevant as a follow-up.
Is there a comment on this? `pass timezone info to the generated cast on the literal value`?
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694424770
**[Test build #128823 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128823/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694183969
**[Test build #128823 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128823/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
----------------------------------------------------------------
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.
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] sunchao commented on a change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489988859
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
（oops just found out my comment was not sent out successfully)
This is because `ResolveTimeZone` will try to add timezone info to all expressions that don't have it during query analysis. However, since the `Cast` expr was generated at optimization phase, it will not have the timezone info. As result, `PlanTest.comparePlans` will fail because of mismatch. I can try to come up with a test if necessary.
On the other hand, I think instead of using `Cast`, we may just directly use the value, since the `Cast` will be optimized away by ConstantFolding anyways later. What do you think?
----------------------------------------------------------------
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.
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] sunchao commented on a change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489996551
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
Thanks. I'll do that then. I'm not totally sure about`Cast.canonicalize` - somehow it is not used in query analysis and optimization and the lazy val is not initialized before the plan comparison.
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489992483
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
yea this also works: just evaluate the cast and make a literal.
----------------------------------------------------------------
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.
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] cloud-fan commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693920672
> pass timezone info to the generated cast on the literal value
I'd also like to understand this more. `Cast.canonicalize` will drop the timezone if it's not needed, so `UnwrapCastInBinaryComparison` shouldn't care about timezone.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694006700
**[Test build #128799 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128799/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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 change in pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #29775:
URL: https://github.com/apache/spark/pull/29775#discussion_r489794434
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala
##########
@@ -103,9 +103,9 @@ object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
// In case both sides have integral type, optimize the comparison by removing casts or
// moving cast to the literal side.
case be @ BinaryComparison(
- Cast(fromExp, toType: IntegralType, _), Literal(value, literalType))
+ Cast(fromExp, toType: IntegralType, tz), Literal(value, literalType))
if canImplicitlyCast(fromExp, toType, literalType) =>
- simplifyIntegralComparison(be, fromExp, toType, value)
+ simplifyIntegralComparison(be, fromExp, toType, value, tz)
Review comment:
Could you give me some pointer for the failures? Or, can we have a reproducer? Usually, every code change had better have a test coverage. If there is a test case for this, it sounds reasonable.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694426230
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694426230
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693731743
----------------------------------------------------------------
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.
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] cloud-fan commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694183435
retest this please
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693969011
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-694183969
**[Test build #128823 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128823/testReport)** for PR 29775 at commit [`3a0f4b0`](https://github.com/apache/spark/commit/3a0f4b0c6019b937c27609b2347b55f254265396).
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693731743
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29775: [SPARK-24994][SQL][FOLLOW-UP] Handle foldable, timezone and cleanup
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29775:
URL: https://github.com/apache/spark/pull/29775#issuecomment-693606720
**[Test build #128778 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/128778/testReport)** for PR 29775 at commit [`4291ec2`](https://github.com/apache/spark/commit/4291ec23a9516ff0922f1f65858bfb5d7c34aafc).
----------------------------------------------------------------
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.
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