[GitHub] spark pull request #22898: [SPARK-25746][SQL][followup] do not add unnecessa...

2018-10-31 Thread asfgit 
Github user asfgit closed the pull request at:

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


---

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

[GitHub] spark pull request #22898: [SPARK-25746][SQL][followup] do not add unnecessa...

2018-10-31 Thread cloud-fan 
Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/22898#discussion_r229667653
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala
 ---
@@ -124,14 +124,9 @@ object ExpressionEncoder {
 s"`GetColumnByOrdinal`, but there are ${getColExprs.size}")
 
   val input = GetStructField(GetColumnByOrdinal(0, schema), index)
-  val newDeserializer = enc.objDeserializer.transformUp {
+  enc.objDeserializer.transformUp {
 case GetColumnByOrdinal(0, _) => input
   }
-  if (schema(index).nullable) {
-If(IsNull(input), Literal.create(null, newDeserializer.dataType), 
newDeserializer)
--- End diff --

good catch!


---

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

[GitHub] spark pull request #22898: [SPARK-25746][SQL][followup] do not add unnecessa...

2018-10-31 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/22898#discussion_r229631397
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala
 ---
@@ -124,14 +124,9 @@ object ExpressionEncoder {
 s"`GetColumnByOrdinal`, but there are ${getColExprs.size}")
 
   val input = GetStructField(GetColumnByOrdinal(0, schema), index)
-  val newDeserializer = enc.objDeserializer.transformUp {
+  enc.objDeserializer.transformUp {
 case GetColumnByOrdinal(0, _) => input
   }
-  if (schema(index).nullable) {
-If(IsNull(input), Literal.create(null, newDeserializer.dataType), 
newDeserializer)
--- End diff --

oh, I see. If the child deserializer is a tuple deserializer, it is just

```scala
val deserializer =
  NewInstance(cls, childrenDeserializers, ObjectType(cls), 
propagateNull = false)
```

So it misses the `If(IsNull(..), null, ...)` pattern. We should wrap the 
`NewInstance` with `If(IsNull(..), null)` at L139.


---

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

[GitHub] spark pull request #22898: [SPARK-25746][SQL][followup] do not add unnecessa...

2018-10-31 Thread viirya 
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/22898#discussion_r229619306
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala
 ---
@@ -124,14 +124,9 @@ object ExpressionEncoder {
 s"`GetColumnByOrdinal`, but there are ${getColExprs.size}")
 
   val input = GetStructField(GetColumnByOrdinal(0, schema), index)
-  val newDeserializer = enc.objDeserializer.transformUp {
+  enc.objDeserializer.transformUp {
 case GetColumnByOrdinal(0, _) => input
   }
-  if (schema(index).nullable) {
-If(IsNull(input), Literal.create(null, newDeserializer.dataType), 
newDeserializer)
-  } else {
-newDeserializer
-  }
--- End diff --

Good catch!


---

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

[GitHub] spark pull request #22898: [SPARK-25746][SQL][followup] do not add unnecessa...

2018-10-30 Thread cloud-fan 
GitHub user cloud-fan opened a pull request:

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

[SPARK-25746][SQL][followup] do not add unnecessary If expression

## What changes were proposed in this pull request?

a followup of https://github.com/apache/spark/pull/22749.

When we construct the new serializer in `ExpressionEncoder.tuple`, we don't 
need to add `if(isnull ...)` check for each field. They are either simple 
expressions that can propagate null correctly(e.g. 
`GetStructField(GetColumnByOrdinal(0, schema), index)`), or complex expression 
that already have the isnull check.

## How was this patch tested?

existing tests

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

$ git pull https://github.com/cloud-fan/spark minor

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

https://github.com/apache/spark/pull/22898.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 #22898


commit 82664439318b72d8446230515abb882b89767bb9
Author: Wenchen Fan 
Date:   2018-10-31T05:44:44Z

do not add unnecessary If expression




---

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