You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mgaido91 <gi...@git.apache.org> on 2018/03/05 13:23:56 UTC
[GitHub] spark pull request #20736: [SPARK-23586][SQL] Add interpreted execution to U...
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 <ma...@...>
Date: 2018-03-05T13:15:06Z
[SPARK-23586][SQL] Add interpreted execution to UnwrapOption
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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 #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/20736
@hvanhovell thanks for your comments, let's say this is a good opportunity to improve test coverage too for it
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20736
**[Test build #87957 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87957/testReport)** for PR 20736 at commit [`804394b`](https://github.com/apache/spark/commit/804394b44e65142c0af3eb3dc19bab29ed13875d).
* 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 issue #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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 #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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/1277/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20736
**[Test build #87963 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87963/testReport)** for PR 20736 at commit [`fc202c6`](https://github.com/apache/spark/commit/fc202c6fc5846b65e39d2fce6f4cdd13e6cd927a).
* 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 issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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 #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20736
**[Test build #87957 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87957/testReport)** for PR 20736 at commit [`804394b`](https://github.com/apache/spark/commit/804394b44e65142c0af3eb3dc19bab29ed13875d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20736
**[Test build #87961 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87961/testReport)** for PR 20736 at commit [`d899f44`](https://github.com/apache/spark/commit/d899f44b213da4e3e819a308a2442a9202901ad3).
* 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 issue #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/20736
@mgaido91 you picked this up phenomenally fast! It looks pretty good, let two minor comments.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87961/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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 #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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/1273/
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 #20736: [SPARK-23586][SQL] Add interpreted execution to U...
Posted by mgaido91 <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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 #20736: [SPARK-23585][SQL] Add interpreted execution to U...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20736
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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 #20736: [SPARK-23586][SQL] Add interpreted execution to U...
Posted by hvanhovell <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20736
**[Test build #87963 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87963/testReport)** for PR 20736 at commit [`fc202c6`](https://github.com/apache/spark/commit/fc202c6fc5846b65e39d2fce6f4cdd13e6cd927a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
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/1279/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/20736
Merging to master. Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/20736
LGTM - pending jenkins
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87963/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23585][SQL] Add interpreted execution to UnwrapOp...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20736
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87957/
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 #20736: [SPARK-23586][SQL] Add interpreted execution to U...
Posted by hvanhovell <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20736: [SPARK-23586][SQL] Add interpreted execution to U...
Posted by hvanhovell <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20736: [SPARK-23586][SQL] Add interpreted execution to UnwrapOp...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20736
**[Test build #87961 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87961/testReport)** for PR 20736 at commit [`d899f44`](https://github.com/apache/spark/commit/d899f44b213da4e3e819a308a2442a9202901ad3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org