You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by ueshin <gi...@git.apache.org> on 2018/08/23 10:55:36 UTC

[GitHub] spark pull request #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for ...

GitHub user ueshin opened a pull request:

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

    [SPARK-25208][SQL] Loosen Cast.forceNullable for DecimalType.

    ## What changes were proposed in this pull request?
    
    Casting to `DecimalType` is not always needed to force nullable.
    If the decimal type to cast is wider than original type, or only truncating or precision loss, the casted value won't be `null`.
    
    ## How was this patch tested?
    
    Added and modified tests.


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

    $ git pull https://github.com/ueshin/apache-spark issues/SPARK-25208/cast_nullable_decimal

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

    https://github.com/apache/spark/pull/22200.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 #22200
    
----
commit e134258738c0dfa43533422d88bd16ce90e92743
Author: Takuya UESHIN <ue...@...>
Date:   2018-08-23T10:05:26Z

    Loosen `Cast.forceNullable` for DecimalType.

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95150/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2483/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for ...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

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


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    LGTM, merging to master!


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for ...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22200#discussion_r232608144
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala ---
    @@ -154,6 +154,15 @@ object Cast {
         fromPrecedence >= 0 && fromPrecedence < toPrecedence
       }
     
    +  def canNullSafeCastToDecimal(from: DataType, to: DecimalType): Boolean = from match {
    +    case from: BooleanType if to.isWiderThan(DecimalType.BooleanDecimal) => true
    +    case from: NumericType if to.isWiderThan(from) => true
    +    case from: DecimalType =>
    +      // truncating or precision lose
    +      (to.precision - to.scale) > (from.precision - from.scale)
    --- End diff --
    
    In this case, we need rounding, then we need an extra precision to avoid overflow.
    E.g., cast 99.95 of Decimal(4, 2) to Decimal(3, 1) will be 100.0, but it’s an overflow and ends up to null. We need Decimal(4, 1) to be null-safe.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    cc @gatorsmile @cloud-fan 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    **[Test build #95150 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95150/testReport)** for PR 22200 at commit [`e134258`](https://github.com/apache/spark/commit/e134258738c0dfa43533422d88bd16ce90e92743).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark pull request #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for ...

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/22200#discussion_r232572693
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala ---
    @@ -154,6 +154,15 @@ object Cast {
         fromPrecedence >= 0 && fromPrecedence < toPrecedence
       }
     
    +  def canNullSafeCastToDecimal(from: DataType, to: DecimalType): Boolean = from match {
    +    case from: BooleanType if to.isWiderThan(DecimalType.BooleanDecimal) => true
    +    case from: NumericType if to.isWiderThan(from) => true
    +    case from: DecimalType =>
    +      // truncating or precision lose
    +      (to.precision - to.scale) > (from.precision - from.scale)
    --- End diff --
    
    why it's not `>=` but `>`?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #22200: [SPARK-25208][SQL] Loosen Cast.forceNullable for Decimal...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22200
  
    **[Test build #95150 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95150/testReport)** for PR 22200 at commit [`e134258`](https://github.com/apache/spark/commit/e134258738c0dfa43533422d88bd16ce90e92743).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org