You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by petermaxlee <gi...@git.apache.org> on 2016/07/08 03:54:58 UTC
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...
GitHub user petermaxlee opened a pull request:
https://github.com/apache/spark/pull/14103
[SPARK-16436][SQL] checkEvaluation support NaN and RuntimeReplaceable
## What changes were proposed in this pull request?
This small patch modifies ExpressionEvalHelper. checkEvaluation to support comparing NaN values for floating point comparisons as well as testing RuntimeReplaceable expressions.
## How was this patch tested?
This is a test harness change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/petermaxlee/spark SPARK-16436
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14103.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 #14103
----
commit 0278dea13085bdeaf0689a2c33a0805a0af2593a
Author: petermaxlee <pe...@gmail.com>
Date: 2016-07-08T03:53:34Z
[SPARK-16436][SQL] checkEvaluation should support NaN and RuntimeReplaceable
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation should support...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/14103
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...
Posted by petermaxlee <gi...@git.apache.org>.
Github user petermaxlee commented on a diff in the pull request:
https://github.com/apache/spark/pull/14103#discussion_r70025887
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -41,7 +41,12 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ originalExpr: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ val expression = originalExpr match {
+ case replaceable: RuntimeReplaceable => replaceable.replaced
--- End diff --
I don't see any test for testing harness in Spark though, but I can do it in a separate pull request if it is needed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation support NaN and Runti...
Posted by petermaxlee <gi...@git.apache.org>.
Github user petermaxlee commented on the issue:
https://github.com/apache/spark/pull/14103
cc @cloud-fan
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...
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/14103#discussion_r70024354
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -41,7 +41,12 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ originalExpr: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ val expression = originalExpr match {
+ case replaceable: RuntimeReplaceable => replaceable.replaced
--- End diff --
can you write a test for this?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation support NaN and Runti...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14103
Merged build finished. Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...
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/14103#discussion_r70029140
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -41,7 +41,12 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ originalExpr: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ val expression = originalExpr match {
+ case replaceable: RuntimeReplaceable => replaceable.replaced
--- End diff --
I mean, do we have a test that pass in `RuntimeReplaceable` to `checkEvaluation`? Or how can we know this works?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation should support NaN
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14103
Merged build finished. Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation should support...
Posted by petermaxlee <gi...@git.apache.org>.
Github user petermaxlee commented on a diff in the pull request:
https://github.com/apache/spark/pull/14103#discussion_r70030804
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -41,7 +41,12 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ originalExpr: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ val expression = originalExpr match {
+ case replaceable: RuntimeReplaceable => replaceable.replaced
--- End diff --
Either way, I removed RuntimeReplaceable here.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation support NaN and Runti...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14103
**[Test build #61955 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61955/consoleFull)** for PR 14103 at commit [`0278dea`](https://github.com/apache/spark/commit/0278dea13085bdeaf0689a2c33a0805a0af2593a).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation should support NaN
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14103
**[Test build #61966 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61966/consoleFull)** for PR 14103 at commit [`ea0e91b`](https://github.com/apache/spark/commit/ea0e91b87202d2a4ae8de997c40039b9fe9bfcd9).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation should support NaN
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14103
**[Test build #61966 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61966/consoleFull)** for PR 14103 at commit [`ea0e91b`](https://github.com/apache/spark/commit/ea0e91b87202d2a4ae8de997c40039b9fe9bfcd9).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation should support NaN
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/14103
thanks, merging to master!
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...
Posted by petermaxlee <gi...@git.apache.org>.
Github user petermaxlee commented on a diff in the pull request:
https://github.com/apache/spark/pull/14103#discussion_r70025935
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -63,6 +68,10 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
expected.asInstanceOf[Spread[Double]].isWithin(result)
case (result: MapData, expected: MapData) =>
result.keyArray() == expected.keyArray() && result.valueArray() == expected.valueArray()
+ case (result: Double, expected: Double) =>
--- End diff --
```
scala> Double.PositiveInfinity == Double.PositiveInfinity
res2: Boolean = true
```
it already works
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation support NaN and Runti...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14103
**[Test build #61955 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/61955/consoleFull)** for PR 14103 at commit [`0278dea`](https://github.com/apache/spark/commit/0278dea13085bdeaf0689a2c33a0805a0af2593a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...
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/14103#discussion_r70024346
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -63,6 +68,10 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
expected.asInstanceOf[Spread[Double]].isWithin(result)
case (result: MapData, expected: MapData) =>
result.keyArray() == expected.keyArray() && result.valueArray() == expected.valueArray()
+ case (result: Double, expected: Double) =>
--- End diff --
shoud we also handle `isInfinite`
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation should support...
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/14103#discussion_r70030535
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -41,7 +41,12 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ originalExpr: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ val expression = originalExpr match {
+ case replaceable: RuntimeReplaceable => replaceable.replaced
--- End diff --
> when we add one that uses this it will fail if it doesn't work
Yea, I also checked all the implementations of `RuntimeReplaceable` and they don't have unit test. It will be good if we can test them and verify the `checkEvaluation` can work with `RuntimeReplaceable` after your change.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation should support...
Posted by petermaxlee <gi...@git.apache.org>.
Github user petermaxlee commented on a diff in the pull request:
https://github.com/apache/spark/pull/14103#discussion_r70030786
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -41,7 +41,12 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ originalExpr: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ val expression = originalExpr match {
+ case replaceable: RuntimeReplaceable => replaceable.replaced
--- End diff --
They are already tested in the end-to-end tests. I'm not sure if expression level unit tests are useful, given they are replaced in the optimizer.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...
Posted by petermaxlee <gi...@git.apache.org>.
Github user petermaxlee commented on a diff in the pull request:
https://github.com/apache/spark/pull/14103#discussion_r70029441
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala ---
@@ -41,7 +41,12 @@ trait ExpressionEvalHelper extends GeneratorDrivenPropertyChecks {
}
protected def checkEvaluation(
- expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ originalExpr: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
+ val expression = originalExpr match {
+ case replaceable: RuntimeReplaceable => replaceable.replaced
--- End diff --
Because when we add one that uses this it will fail if it doesn't work?
I can also remove this if you think it's better.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation support NaN and Runti...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14103
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/61955/
Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #14103: [SPARK-16436][SQL] checkEvaluation should support NaN
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14103
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/61966/
Test 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org