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 2014/05/08 05:28:10 UTC
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
GitHub user ueshin opened a pull request:
https://github.com/apache/spark/pull/689
[SPARK-1754] [SQL] Add missing arithmetic DSL operations.
Add missing arithmetic DSL operations: `unary_-`, `%`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ueshin/apache-spark issues/SPARK-1754
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/689.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 #689
----
commit e09c5b8d38738f1dfffab77bac1ac217949e6607
Author: Takuya UESHIN <ue...@happy-camper.st>
Date: 2014-05-08T03:14:17Z
Add missing arithmetic DSL operations.
commit 5b3f087ba9109cdc7bafd0e65e9c3b934b0e5fb4
Author: Takuya UESHIN <ue...@happy-camper.st>
Date: 2014-05-08T03:16:52Z
Add tests relating DSL operations.
----
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42515916
Thanks for doing this! I think we are also missing ! (not).
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42512925
Merged build finished. All automated tests passed.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42520521
Merged build finished. All automated tests passed.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:
https://github.com/apache/spark/pull/689#discussion_r12416177
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvaluationSuite.scala ---
@@ -381,6 +381,30 @@ class ExpressionEvaluationSuite extends FunSuite {
checkEvaluation(Add(c1, Literal(null, IntegerType)), null, row)
checkEvaluation(Add(Literal(null, IntegerType), c2), null, row)
checkEvaluation(Add(Literal(null, IntegerType), Literal(null, IntegerType)), null, row)
+
+ checkEvaluation(-c1, -1, row)
+ checkEvaluation(c1 + c2, 3, row)
+ checkEvaluation(c1 - c2, -1, row)
+ checkEvaluation(c1 * c2, 2, row)
+ checkEvaluation(c1 / c2, 0, row)
+ checkEvaluation(c1 % c2, 1, row)
+ }
+
+ test("BinaryPredicate") {
--- End diff --
Ah, I see, you are right.
I'll remove the test.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42516176
Oops, I'll add it.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42516536
Merged build started.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/689#discussion_r12415957
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvaluationSuite.scala ---
@@ -381,6 +381,30 @@ class ExpressionEvaluationSuite extends FunSuite {
checkEvaluation(Add(c1, Literal(null, IntegerType)), null, row)
checkEvaluation(Add(Literal(null, IntegerType), c2), null, row)
checkEvaluation(Add(Literal(null, IntegerType), Literal(null, IntegerType)), null, row)
+
+ checkEvaluation(-c1, -1, row)
+ checkEvaluation(c1 + c2, 3, row)
+ checkEvaluation(c1 - c2, -1, row)
+ checkEvaluation(c1 * c2, 2, row)
+ checkEvaluation(c1 / c2, 0, row)
+ checkEvaluation(c1 % c2, 1, row)
+ }
+
+ test("BinaryPredicate") {
--- End diff --
I think these predicate tests are redundant with the [tests above](https://github.com/ueshin/apache-spark/blob/issues/SPARK-1754/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvaluationSuite.scala#L70).
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42510321
Merged build started.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42512927
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14803/
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/689
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/689#discussion_r12416135
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvaluationSuite.scala ---
@@ -381,6 +381,30 @@ class ExpressionEvaluationSuite extends FunSuite {
checkEvaluation(Add(c1, Literal(null, IntegerType)), null, row)
checkEvaluation(Add(Literal(null, IntegerType), c2), null, row)
checkEvaluation(Add(Literal(null, IntegerType), Literal(null, IntegerType)), null, row)
+
+ checkEvaluation(-c1, -1, row)
+ checkEvaluation(c1 + c2, 3, row)
+ checkEvaluation(c1 - c2, -1, row)
+ checkEvaluation(c1 * c2, 2, row)
+ checkEvaluation(c1 / c2, 0, row)
+ checkEvaluation(c1 % c2, 1, row)
+ }
+
+ test("BinaryPredicate") {
--- End diff --
The tests above use the DSL too and test the whole truth table for each operation.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42510314
Merged build triggered.
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:
https://github.com/apache/spark/pull/689#discussion_r12416099
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvaluationSuite.scala ---
@@ -381,6 +381,30 @@ class ExpressionEvaluationSuite extends FunSuite {
checkEvaluation(Add(c1, Literal(null, IntegerType)), null, row)
checkEvaluation(Add(Literal(null, IntegerType), c2), null, row)
checkEvaluation(Add(Literal(null, IntegerType), Literal(null, IntegerType)), null, row)
+
+ checkEvaluation(-c1, -1, row)
+ checkEvaluation(c1 + c2, 3, row)
+ checkEvaluation(c1 - c2, -1, row)
+ checkEvaluation(c1 * c2, 2, row)
+ checkEvaluation(c1 / c2, 0, row)
+ checkEvaluation(c1 % c2, 1, row)
+ }
+
+ test("BinaryPredicate") {
--- End diff --
Thank you for your comment.
I just wanted to check if the DSL operations work or not here.
Don't we need them?
---
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.
---
[GitHub] spark pull request: [SPARK-1754] [SQL] Add missing arithmetic DSL ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/689#issuecomment-42520522
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14806/
---
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.
---