[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...

[petermaxlee]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...

[cloud-fan]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...

[petermaxlee]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...

[petermaxlee]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...

[cloud-fan]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...

[cloud-fan]

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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #14103: [SPARK-16436][SQL] checkEvaluation support NaN an...

[petermaxlee]

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 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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