[GitHub] spark pull request #20736: [SPARK-23586][SQL] Add interpreted execution to U...

[hvanhovell]

Github user hvanhovell commented on a diff in the pull request:

https://github.com/apache/spark/pull/20736#discussion_r172194194
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -382,8 +382,14 @@ case class UnwrapOption(
 
   override def inputTypes: Seq[AbstractDataType] = ObjectType :: Nil
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported")
+  override def eval(input: InternalRow): Any = {
+val inputObject = child.eval(input)
--- End diff --

I am fine either way. This is going to be slow anyway. Let's leave as it is.


---

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

[GitHub] spark pull request #20736: [SPARK-23586][SQL] Add interpreted execution to U...

[mgaido91]

Github user mgaido91 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20736#discussion_r172192400
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -382,8 +382,14 @@ case class UnwrapOption(
 
   override def inputTypes: Seq[AbstractDataType] = ObjectType :: Nil
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported")
+  override def eval(input: InternalRow): Any = {
+val inputObject = child.eval(input)
--- End diff --

thanks for your comment. I haven't used pattern matching because it is a 
bit slower than using if...else and I'd prefer the fastest way here. What do 
you think?


---

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

[GitHub] spark pull request #20736: [SPARK-23586][SQL] Add interpreted execution to U...

[hvanhovell]

Github user hvanhovell commented on a diff in the pull request:

https://github.com/apache/spark/pull/20736#discussion_r172191654
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
 ---
@@ -66,4 +66,16 @@ class ObjectExpressionsSuite extends SparkFunSuite with 
ExpressionEvalHelper {
 checkEvalutionWithUnsafeProjection(
   mapEncoder.serializer.head, mapExpected, mapInputRow)
   }
+
+  test("SPARK-23586: UnwrapOption should support interpreted execution") {
--- End diff --

I just found out that there are literally no unit tests for this expression 
:(...

Can you use `checkEvaluation`? This will make sure both code gen and 
interpreted mode are tested?


---

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

[GitHub] spark pull request #20736: [SPARK-23586][SQL] Add interpreted execution to U...

[hvanhovell]

Github user hvanhovell commented on a diff in the pull request:

https://github.com/apache/spark/pull/20736#discussion_r172188241
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
 ---
@@ -382,8 +382,14 @@ case class UnwrapOption(
 
   override def inputTypes: Seq[AbstractDataType] = ObjectType :: Nil
 
-  override def eval(input: InternalRow): Any =
-throw new UnsupportedOperationException("Only code-generated 
evaluation is supported")
+  override def eval(input: InternalRow): Any = {
+val inputObject = child.eval(input)
--- End diff --

NIT: you can also use pattern matching here. That saves you some typing:
```scala
child.eval(input) match {
  case Some(value) => value
  case _ => null
}
```


---

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

[GitHub] spark pull request #20736: [SPARK-23586][SQL] Add interpreted execution to U...

[mgaido91]

GitHub user mgaido91 opened a pull request:

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

[SPARK-23586][SQL] Add interpreted execution to UnwrapOption

## What changes were proposed in this pull request?

The PR adds interpreted execution to UnwrapOption.

## How was this patch tested?

added UT


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

$ git pull https://github.com/mgaido91/spark SPARK-23586

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

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


commit 804394b44e65142c0af3eb3dc19bab29ed13875d
Author: Marco Gaido 
Date:   2018-03-05T13:15:06Z

[SPARK-23586][SQL] Add interpreted execution to UnwrapOption




---

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