You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by guowei2 <gi...@git.apache.org> on 2015/02/10 08:14:34 UTC
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
GitHub user guowei2 opened a pull request:
https://github.com/apache/spark/pull/4497
[SPARK-5710][SQL] Combines two adjacent Cast expressions into one
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/guowei2/spark SPARK-5710
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/4497.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4497
----
commit af0f56a6200b49455b78af9d283ff4a2b72bdb19
Author: guowei2 <gu...@asiainfo.com>
Date: 2015-02-10T07:11:59Z
Combines two adjacent Cast expressions into one
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/4497#discussion_r24393934
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala ---
@@ -602,6 +603,16 @@ object CombineLimits extends Rule[LogicalPlan] {
}
/**
+ * Combines two adjacent [[Cast]] expressions into one
+ */
+object CombineCasts extends Rule[LogicalPlan] {
+ def apply(plan: LogicalPlan): LogicalPlan = plan transformAllExpressions {
+ case cc @ Cast(c2 @ Cast(c1, dataType1), dataType2) =>
--- End diff --
`cc` and `c2` is not used. You could just write `case Cast(Cast(c1, dataType1), dataType2)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by guowei2 <gi...@git.apache.org>.
Github user guowei2 commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-73662470
@chenghao-intel
can we cast int or long to Date?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by guowei2 <gi...@git.apache.org>.
Github user guowei2 commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-73665511
@chenghao-intel i think i should restrict it in numeric.
```
object CombineCasts extends Rule[LogicalPlan] {
def apply(plan: LogicalPlan): LogicalPlan = plan transformAllExpressions {
case Cast(Cast(c1, dataType1: NumericType), dataType2: NumericType) =>
Cast(c1, dataType2)
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-74957611
Agreed. If there are concrete proposals for eliminating redundant casts then we should discuss on JIRA. However as is this could change the answer and thus is an invalid optimization. So, we should close this issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-73667464
Numeric casts can lose precision, and as a result, I don't think the cast works.
e.g.
long => int => long yields different results from long => long
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by guowei2 <gi...@git.apache.org>.
Github user guowei2 commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-73668250
@rxin
For example.
create table test(a decimal(3,1));
explain select * from test where a*2-1>1;
== Physical Plan ==
Filter (CAST(CAST((CAST(CAST((CAST(a#5, DecimalType()) * 2), DecimalType(21,1)), DecimalType()) - 1), DecimalType(22,1)), DecimalType()) > 1)
HiveTableScan a#5, (MetastoreRelation default, test, None), None
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-73667493
Did you see a query that needs this optimization?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-74858454
@guowei2 can this change be limited to merging adjacent casts that are definitely redundant? it sounds like this change introduces bugs as-is, but there may still be some opportunities for optimization. It'd be good to clarify whether this is proceeding or not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-73654741
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/4497#issuecomment-73657894
This is probably work for some cases, not for all of the cases. For example:
Int => Data => String is not equals to Int => String
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-5710][SQL] Combines two adjacent Cast e...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/4497
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org