You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by priyankagargnitk <gi...@git.apache.org> on 2016/10/24 06:36:28 UTC

[GitHub] spark pull request #15609: [SPARK-18048][SQL] To make behaviour of If consis...

GitHub user priyankagargnitk opened a pull request:

    https://github.com/apache/spark/pull/15609

    [SPARK-18048][SQL] To make behaviour of If consistent, in case of true expression and false expression are of compatible data types.

    ## What changes were proposed in this pull request?
    This change adds a type conversion from false value datatype to true value\u2019s datatype.
    
    Earlier, the expression If(Literal.create(true, BooleanType), Literal.create(identity(1), DateType), Literal.create(identity(2L), TimestampType))  was throwing Exception while the expression If(Literal.create(true, BooleanType), Literal.create(identity(1L), TimestampType), Literal.create(identity(2), DateType)) was working fine. So , if we interchange the true and false expressions, behaviour of the IF expression changes.
    
    ## How was this patch tested?
    Added test case in ConditionalExpressionSuite.scala
    
    jira entry for detail: https://issues.apache.org/jira/browse/SPARK-18048

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/priyankagargnitk/spark SPARK-18048

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/15609.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 #15609
    
----
commit 9a8ef85c9e8924a44b4e7d5639f259c950d03a9c
Author: prigarg <pr...@adobe.com>
Date:   2016-10-24T06:34:58Z

    [SPARK-18048][SQL] To make behaviour of If consistent, in case of true expression and false expression are of compatible data types.
    
    ## What changes were proposed in this pull request?
    This change adds a type conversion from false value datatype to true value\u2019s datatype.
    
    Earlier, the expression If(Literal.create(true, BooleanType), Literal.create(identity(1), DateType), Literal.create(identity(2L), TimestampType))  was throwing Exception while the expression If(Literal.create(true, BooleanType), Literal.create(identity(1L), TimestampType), Literal.create(identity(2), DateType)) was working fine. So , if we interchange the true and false expressions, behaviour of the IF expression changes.
    
    ## How was this patch tested?
    Added test case in ConditionalExpressionSuite.scala
    
    jira entry for detail: https://issues.apache.org/jira/browse/SPARK-18048

----


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by hvanhovell <gi...@git.apache.org>.

Github user hvanhovell commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    @priyankagargnitk this should be covered in type widening. The `trueValue` and `falseValue` expressions should have the same type by the time we reach the execution phase. The `IfCoercion` rule should have injected casts if the types were not compatible. Could you please take a look there?


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by priyankagargnitk <gi...@git.apache.org>.

Github user priyankagargnitk commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    Sure.. I am closing it... If still face any issues... will raise another.. Thanks for the help.


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by priyankagargnitk <gi...@git.apache.org>.

Github user priyankagargnitk commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    Hmm... I think that makes sense.. i am trying a couple of more things... Thanks


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    Then, wouldn't the responsibility of type coercion be your own layer? I guess this PR virtually mean every expression handled in `TypeCoercion` should be fixed too. For example, I guess 
    
    ```scala
    Greatest(Seq(Literal(10, IntegerType), Literal("a", StringType))).eval()
    ```
    
    wouldn't work too.


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    @priyankagargnitk Could I please ask some codes which I can reproduce this issue? It seems fine the sql below:
    
    ```sql
    spark-sql> SELECT if(true, cast('2014-01-01' as date), cast('2014-12-12' as timestamp));
    2014-01-01 00:00:00
    ```
    
    I just checked and it seems this is covered in [TypeCoercion.scala#L596-L601](https://github.com/apache/spark/blob/39e2bad6a866d27c3ca594d15e574a1da3ee84cc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala#L596-L601). It seems the cases here are unreachable to end users. 
    
    Also, it seems it can't just cast it. For example, if we add a test with `IntegerType` and `StringType` here, i guess it'd be failed due to the cast.


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by priyankagargnitk <gi...@git.apache.org>.

Github user priyankagargnitk commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    Actually our use case is little different, we are not invoking it with the Select queries.. We are using JS to let user type expressions and then we have created out own layer on top of spark that converts this JS to Spark expressions.. So if user writes an expression that returns date in true branch and timestamp in false branch.. Then the IF expression fails... because it internally getting mapped to If(Literal.create(true, BooleanType), Literal.create(identity(1), DateType), Literal.create(identity(2L), TimestampType)) , Which fails.


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    @priyankagargnitk Could maybe we then close this?


---
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 issue #15609: [SPARK-18048][SQL] To make behaviour of If consistent, i...

Posted by HyukjinKwon <gi...@git.apache.org>.

Github user HyukjinKwon commented on the issue:

    https://github.com/apache/spark/pull/15609
  
    Hm, does this cause a actual problem? I thought this is covered in type-widening.


---
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