You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rednaxelafx <gi...@git.apache.org> on 2018/06/26 11:08:44 UTC
[GitHub] spark pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
GitHub user rednaxelafx opened a pull request:
https://github.com/apache/spark/pull/21643
[SPARK-24659][SQL] GenericArrayData.equals should respect element type differences
## What changes were proposed in this pull request?
Fix `GenericArrayData.equals`, so that it respects the actual types of the elements.
e.g. an instance that represents an `array<int>` and another instance that represents an `array<long>` should be considered incompatible, and thus should return false for `equals`.
`GenericArrayData` doesn't keep any schema information by itself, and rather relies on the Java objects referenced by its `array` field's elements to keep track of their own object types. So, the most straightforward way to respect their types is to call `equals` on the elements, instead of using Scala's `==` operator, which can have semantics that are not always desirable:
```
new java.lang.Integer(123) == new java.lang.Long(123L) // true in Scala
new java.lang.Integer(123).equals(new java.lang.Long(123L)) // false in Scala
```
## How was this patch tested?
Added unit test in `ComplexDataSuite`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rednaxelafx/apache-spark fix-genericarraydata-equals
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21643.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 #21643
----
commit d91b44accbe40b9879cda259912e3ca38759d716
Author: Kris Mok <kr...@...>
Date: 2018-06-26T11:02:25Z
SPARK-24659: GenericArrayData.equals should respect element type differences
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21643
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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-unified/479/
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198268375
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala ---
@@ -104,4 +104,38 @@ class ComplexDataSuite extends SparkFunSuite {
// The copied data should not be changed externally.
assert(copied.getStruct(0, 1).getUTF8String(0).toString == "a")
}
+
+ test("SPARK-24659: GenericArrayData.equals should respect element type differences") {
+ import scala.reflect.ClassTag
+
+ // Expected positive cases
+ def arraysShouldEqual[T: ClassTag](element: T*): Unit = {
+ val array1 = new GenericArrayData(Array[T](element: _*))
+ val array2 = new GenericArrayData(Array[T](element: _*))
+ assert(array1.equals(array2))
+ }
+ arraysShouldEqual(true, false) // Boolean
+ arraysShouldEqual(0.toByte, 123.toByte, (-123).toByte) // Byte
+ arraysShouldEqual(0.toShort, 123.toShort, (-256).toShort) // Short
+ arraysShouldEqual(0, 123, -65536) // Int
+ arraysShouldEqual(0L, 123L, -65536L) // Long
--- End diff --
It is not important but if you are checking corner cases, probably, it makes sense to pass values like `Long.MinValue` and `Double.MaxValue`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21643
LGTM except for one minor comment.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21643
**[Test build #92353 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92353/testReport)** for PR 21643 at commit [`a30de22`](https://github.com/apache/spark/commit/a30de22183cab3cde90fce6029cd03dfc5b5758a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21643
**[Test build #92352 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92352/testReport)** for PR 21643 at commit [`083673c`](https://github.com/apache/spark/commit/083673c9a9f851fc40dc53b42754776fba5f100a).
* 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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21643
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21643
**[Test build #92352 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92352/testReport)** for PR 21643 at commit [`083673c`](https://github.com/apache/spark/commit/083673c9a9f851fc40dc53b42754776fba5f100a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198118072
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala ---
@@ -104,4 +104,13 @@ class ComplexDataSuite extends SparkFunSuite {
// The copied data should not be changed externally.
assert(copied.getStruct(0, 1).getUTF8String(0).toString == "a")
}
+
+ test("SPARK-24659: GenericArrayData.equals should respect element type differences") {
+ // Spark SQL considers array<int> and array<long> to be incompatible,
+ // so an underlying implementation of array type should return false in this case.
+ val array1 = new GenericArrayData(Array[Int](123))
+ val array2 = new GenericArrayData(Array[Long](123L))
+
+ assert(!array1.equals(array2))
--- End diff --
Good catch! Can you test short and byte, too?
```
// GenericArrayData
scala> val arrayByte= new GenericArrayData(Array[Short](123.toByte))
scala> val arrayShort = new GenericArrayData(Array[Short](123.toShort))
scala> val arrayInt = new GenericArrayData(Array[Int](123))
scala> val arrayLong = new GenericArrayData(Array[Long](123L))
scala> arrayByte.equals(arrayLong)
res8: Boolean = true
scala> arrayByte.equals(arrayInt)
res9: Boolean = true
scala> arrayShort.equals(arrayInt)
res10: Boolean = true
scala> arrayShort.equals(arrayLong)
res11: Boolean = true
// UnsafeArrayData
scala> val unsafeByte = ExpressionEncoder[Array[Byte]].resolveAndBind().toRow(arrayByte).getArray(0)
scala> val unsafeShort = ExpressionEncoder[Array[Short]].resolveAndBind().toRow(arrayShort).getArray(0)
scala> val unsafeInt = ExpressionEncoder[Array[Int]].resolveAndBind().toRow(arrayInt).getArray(0)
scala> val unsafeLong = ExpressionEncoder[Array[Long]].resolveAndBind().toRow(arrayLong).getArray(0)
scala> arrayByte.equals(arrayLong)
res12: Boolean = false
scala> arrayByte.equals(arrayInt)
res13: Boolean = false
scala> arrayShort.equals(arrayInt)
res14: Boolean = false
scala> arrayShort.equals(arrayLong)
res15: Boolean = false
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
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/21643#discussion_r198186283
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/GenericArrayData.scala ---
@@ -122,7 +122,7 @@ class GenericArrayData(val array: Array[Any]) extends ArrayData {
if (!o2.isInstanceOf[Double] || ! java.lang.Double.isNaN(o2.asInstanceOf[Double])) {
return false
}
- case _ => if (o1 != o2) {
+ case _ => if (!o1.equals(o2)) {
--- End diff --
in java `byte[]` or other primitive arrays doesn't have a proper `equals` implementation.
```
scala> Array(1) == Array(1)
res0: Boolean = false
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21643
**[Test build #92335 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92335/testReport)** for PR 21643 at commit [`d91b44a`](https://github.com/apache/spark/commit/d91b44accbe40b9879cda259912e3ca38759d716).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92352/
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by rednaxelafx <gi...@git.apache.org>.
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198370477
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala ---
@@ -104,4 +104,40 @@ class ComplexDataSuite extends SparkFunSuite {
// The copied data should not be changed externally.
assert(copied.getStruct(0, 1).getUTF8String(0).toString == "a")
}
+
+ test("SPARK-24659: GenericArrayData.equals should respect element type differences") {
+ import scala.reflect.ClassTag
--- End diff --
Thanks for your suggestion! I'm used to making one-off imports inside a function when an import is only used within that function, so that the scope is as narrow as possible without being disturbing.
Are there any Spark coding style guidelines that suggest otherwise? If so I'll follow the guideline and always import at the beginning of the file.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by rednaxelafx <gi...@git.apache.org>.
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21643
https://github.com/apache/spark/pull/21643#pullrequestreview-131977512
For all practical purposes, no, this change shouldn't break any use cases that we support; there could be use cases that we didn't intend to support that might break.
In Spark SQL, SQL/DataFrame/Dataset operations are all type-checked, and the analyzer will strictly reject operations that involve incompatible types.
Array types in Spark SQL are neither covariant nor contravariant, so e.g. `array<int>` and `array<long>` would be considered incompatible, and the analyzer would have rejected equality comparison between them in the first place.
In all other cases, `GenericArrayData` is a Spark SQL internal type that shouldn't be used on the outside, and I don't think we ever intended to support direct access of this type from the outside.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198339961
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala ---
@@ -104,4 +104,40 @@ class ComplexDataSuite extends SparkFunSuite {
// The copied data should not be changed externally.
assert(copied.getStruct(0, 1).getUTF8String(0).toString == "a")
}
+
+ test("SPARK-24659: GenericArrayData.equals should respect element type differences") {
+ import scala.reflect.ClassTag
--- End diff --
nit: you can move this import to the head of this file.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198109320
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/GenericArrayData.scala ---
@@ -122,7 +122,7 @@ class GenericArrayData(val array: Array[Any]) extends ArrayData {
if (!o2.isInstanceOf[Double] || ! java.lang.Double.isNaN(o2.asInstanceOf[Double])) {
return false
}
- case _ => if (o1 != o2) {
+ case _ => if (!o1.equals(o2)) {
--- End diff --
Are there any needs to handle `Array[Byte]` separately above?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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-unified/492/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21643
**[Test build #92335 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92335/testReport)** for PR 21643 at commit [`d91b44a`](https://github.com/apache/spark/commit/d91b44accbe40b9879cda259912e3ca38759d716).
* 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 pull request #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
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/21643#discussion_r198189030
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala ---
@@ -104,4 +104,13 @@ class ComplexDataSuite extends SparkFunSuite {
// The copied data should not be changed externally.
assert(copied.getStruct(0, 1).getUTF8String(0).toString == "a")
}
+
+ test("SPARK-24659: GenericArrayData.equals should respect element type differences") {
+ // Spark SQL considers array<int> and array<long> to be incompatible,
+ // so an underlying implementation of array type should return false in this case.
+ val array1 = new GenericArrayData(Array[Int](123))
+ val array2 = new GenericArrayData(Array[Long](123L))
+
+ assert(!array1.equals(array2))
--- End diff --
without schema, Spark can never compare a generic and an unsafe array. This fix is not a real bug fix, but makes the `GenericArrayData` more clear about equals semantic, which is good to have, and might be useful when writing tests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21643
**[Test build #92353 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/92353/testReport)** for PR 21643 at commit [`a30de22`](https://github.com/apache/spark/commit/a30de22183cab3cde90fce6029cd03dfc5b5758a).
* 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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by rednaxelafx <gi...@git.apache.org>.
Github user rednaxelafx commented on the issue:
https://github.com/apache/spark/pull/21643
Thanks for your comments, @MaxGekk @maropu @cloud-fan , I've tweaked the unit test case to address your comments. Please check it out again.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92335/
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by MaxGekk <gi...@git.apache.org>.
Github user MaxGekk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198106640
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala ---
@@ -104,4 +104,13 @@ class ComplexDataSuite extends SparkFunSuite {
// The copied data should not be changed externally.
assert(copied.getStruct(0, 1).getUTF8String(0).toString == "a")
}
+
+ test("SPARK-24659: GenericArrayData.equals should respect element type differences") {
+ // Spark SQL considers array<int> and array<long> to be incompatible,
+ // so an underlying implementation of array type should return false in this case.
+ val array1 = new GenericArrayData(Array[Int](123))
+ val array2 = new GenericArrayData(Array[Long](123L))
+
+ assert(!array1.equals(array2))
--- End diff --
Can you check positive case when two arrays have the same element type?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/92353/
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 #21643: [SPARK-24659][SQL] GenericArrayData.equals should...
Posted by rednaxelafx <gi...@git.apache.org>.
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/21643#discussion_r198268691
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ComplexDataSuite.scala ---
@@ -104,4 +104,38 @@ class ComplexDataSuite extends SparkFunSuite {
// The copied data should not be changed externally.
assert(copied.getStruct(0, 1).getUTF8String(0).toString == "a")
}
+
+ test("SPARK-24659: GenericArrayData.equals should respect element type differences") {
+ import scala.reflect.ClassTag
+
+ // Expected positive cases
+ def arraysShouldEqual[T: ClassTag](element: T*): Unit = {
+ val array1 = new GenericArrayData(Array[T](element: _*))
+ val array2 = new GenericArrayData(Array[T](element: _*))
+ assert(array1.equals(array2))
+ }
+ arraysShouldEqual(true, false) // Boolean
+ arraysShouldEqual(0.toByte, 123.toByte, (-123).toByte) // Byte
+ arraysShouldEqual(0.toShort, 123.toShort, (-256).toShort) // Short
+ arraysShouldEqual(0, 123, -65536) // Int
+ arraysShouldEqual(0L, 123L, -65536L) // Long
--- End diff --
That's a good one. I can do that (and NaNs/Infinity for floating point types too)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21643: [SPARK-24659][SQL] GenericArrayData.equals should respec...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21643
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-unified/493/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org