[SPARK-21344][SQL] BinaryType comparison does signed byte array comparison #18571

kiszk · 2017-07-08T09:29:51Z

What changes were proposed in this pull request?

This PR fixes a wrong comparison for BinaryType. This PR enables unsigned comparison and unsigned prefix generation for an array for BinaryType. Previous implementations uses signed operations.

How was this patch tested?

Added a test suite in OrderingSuite.

SparkQA · 2017-07-08T11:01:12Z

Test build #79381 has finished for PR 18571 at commit ac86eed.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2017-07-08T11:21:04Z

Aren't byte comparisons signed though elsewhere? as well as other integer type comparison? this would become inconsistent. Maybe I'm missing the reason this should be different.

kiszk · 2017-07-08T12:02:30Z

IIUC, BinaryType should be handled as unsigned byte while Java and Scala have only signed byte array.
That is why we have to do unsigned comparison of BinaryType.
The places where byte comparison is used rather than for BinaryType, I think that we do not change comparisons.

What do you think?
cc: @gatorsmile @maropu

maropu · 2017-07-09T02:23:03Z

I checked other dbms (dbms-like) systems handled binary as unassigned;

postgres=# SELECT E'\\x00'::BYTEA < E'\\xff'::BYTEA;
 ?column? 
----------
 t
(1 row)

hive> SELECT unhex('00') < unhex('ff');
OK
true

mysql> SELECT x'00' < x'ff';
+---------------+
| x'00' < x'ff' |
+---------------+
|             1 |
+---------------+

In some parts of the Spark implementation, they also handle binary as unassigned (https://github.com/apache/spark/blob/master/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java#L36). I feel, based on the principle of least astonishment, we'd be better to handle binary as unassigned in user-facing pieces.

kiszk · 2017-07-09T15:19:32Z

@maropu, thank you for your comment.

dongjoon-hyun · 2017-07-09T20:43:05Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala

+
+  test("SPARK-21344: BinaryType comparison does signed byte array comparison") {
+    val b1 = Array[Byte](1)   // 0x01
+    val b2 = Array[Byte](-1)  // 0xff


Hi, @kiszk
Can we have another test case with more than one byte for code coverage?

Thank you for your comment, done.

Also, it looks better to check the boundary values like -128 and 127.

SparkQA · 2017-07-09T21:04:38Z

Test build #79425 has finished for PR 18571 at commit 0fea784.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2017-07-10T04:55:21Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/ByteArray.java

@@ -44,7 +44,7 @@ public static long getPrefix(byte[] bytes) {
      final int minLen = Math.min(bytes.length, 8);
      long p = 0;
      for (int i = 0; i < minLen; ++i) {
-        p |= (128L + Platform.getByte(bytes, Platform.BYTE_ARRAY_OFFSET + i))
+        p |= ((long)Platform.getByte(bytes, Platform.BYTE_ARRAY_OFFSET + i) & 0xff)


Nit: maybe we need a single space like ((long) Platform....

good catch. done.

SparkQA · 2017-07-10T07:04:55Z

Test build #79436 has finished for PR 18571 at commit 814d4c4.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-07-10T07:04:56Z

Test build #79439 has finished for PR 18571 at commit a84776b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2017-07-10T07:12:37Z

Jenkins, retest this please.

SparkQA · 2017-07-10T10:11:54Z

Test build #79450 has finished for PR 18571 at commit a84776b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2017-07-10T10:24:16Z

@gatorsmile could you please review this?

gatorsmile · 2017-07-13T23:16:25Z

SELECT x'00' < x'0f'
SELECT x'00' < x'ff'

Could you add the above queries to a new comparator.sql file?

gatorsmile

LGTM

dongjoon-hyun

+1, LGTM.

SparkQA · 2017-07-14T06:30:38Z

Test build #79600 has finished for PR 18571 at commit e33ec8e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2017-07-14T06:54:11Z

@gatorsmile thank you for your comment, done

viirya · 2017-07-14T07:03:07Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala

+      (Array[Byte](1, 1, 1, 1, 1, 1, 1, 1, 1), Array[Byte](1, 1, 1, 1, 1, 1, 1, 1, -1))
+      )
+    data.foreach { case (b1, b2) =>
+      val rowOrdering = InterpretedOrdering.forSchema(Seq(BinaryType, BinaryType))


hmmm, don't you just have one binary field for each row? Why you specify two fields to compare?

Good catch, updated.

viirya · 2017-07-14T07:04:44Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala

+          BoundReference(1, BinaryType, nullable = true).asc :: Nil)
+      val rowType = StructType(
+        StructField("b1", BinaryType, nullable = true) ::
+          StructField("b2", BinaryType, nullable = true) :: Nil)


You specify two binary fields as row schema. But actually your input just has one binary field (i.e., Row(b1)).

I misunderstood, updated.

viirya · 2017-07-14T07:46:38Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/ByteArray.java

@@ -44,7 +44,7 @@ public static long getPrefix(byte[] bytes) {
      final int minLen = Math.min(bytes.length, 8);
      long p = 0;
      for (int i = 0; i < minLen; ++i) {
-        p |= (128L + Platform.getByte(bytes, Platform.BYTE_ARRAY_OFFSET + i))
+        p |= ((long) Platform.getByte(bytes, Platform.BYTE_ARRAY_OFFSET + i) & 0xff)


Seems we don't have unit test for this. Shall we add a ByteArraySuite?

We have the unit test for this. I added some cases.

SparkQA · 2017-07-14T19:30:29Z

Test build #79612 has finished for PR 18571 at commit 08776e1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-07-14T20:56:11Z

It sounds like all the comments from @viirya have been resolved. Will merge it tonight. Thanks!

viirya · 2017-07-14T23:18:09Z

LGTM

…rison ## What changes were proposed in this pull request? This PR fixes a wrong comparison for `BinaryType`. This PR enables unsigned comparison and unsigned prefix generation for an array for `BinaryType`. Previous implementations uses signed operations. ## How was this patch tested? Added a test suite in `OrderingSuite`. Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes #18571 from kiszk/SPARK-21344. (cherry picked from commit ac5d5d7) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

gatorsmile · 2017-07-15T03:17:40Z

Thanks! Merging to master/2.2/2.1/2.0

… representation ## What changes were proposed in this pull request? SPARK 2.0 does not support hex literal. Thus, the test case failed after backporting #18571 ## How was this patch tested? N/A Author: gatorsmile <gatorsmile@gmail.com> Closes #18643 from gatorsmile/fixTestFailure2.0.

…rison ## What changes were proposed in this pull request? This PR fixes a wrong comparison for `BinaryType`. This PR enables unsigned comparison and unsigned prefix generation for an array for `BinaryType`. Previous implementations uses signed operations. ## How was this patch tested? Added a test suite in `OrderingSuite`. Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes apache#18571 from kiszk/SPARK-21344. (cherry picked from commit ac5d5d7) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

…rison This PR fixes a wrong comparison for `BinaryType`. This PR enables unsigned comparison and unsigned prefix generation for an array for `BinaryType`. Previous implementations uses signed operations. Added a test suite in `OrderingSuite`. Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes apache#18571 from kiszk/SPARK-21344. (cherry picked from commit ac5d5d7) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

initial commit

ac86eed

Use unsigned byte value to generate prefix

0fea784

dongjoon-hyun reviewed Jul 9, 2017

View reviewed changes

kiszk added 2 commits July 10, 2017 13:53

fix signed value comparison in test suite

bb01063

address review comment

814d4c4

maropu reviewed Jul 10, 2017

View reviewed changes

address review comment

a84776b

gatorsmile approved these changes Jul 13, 2017

View reviewed changes

newly add comparator.sql

e33ec8e

dongjoon-hyun approved these changes Jul 14, 2017

View reviewed changes

viirya reviewed Jul 14, 2017

View reviewed changes

address review comments

08776e1

asfgit closed this in ac5d5d7 Jul 15, 2017

gatorsmile mentioned this pull request Jul 16, 2017

[SPARK-21426] [2.0] [SQL] [TEST] Fix test failure due to missing literal representation #18643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21344][SQL] BinaryType comparison does signed byte array comparison #18571

[SPARK-21344][SQL] BinaryType comparison does signed byte array comparison #18571

kiszk commented Jul 8, 2017 •

edited

Loading

SparkQA commented Jul 8, 2017

srowen commented Jul 8, 2017

kiszk commented Jul 8, 2017 •

edited

Loading

maropu commented Jul 9, 2017

kiszk commented Jul 9, 2017

dongjoon-hyun Jul 9, 2017

kiszk Jul 10, 2017

maropu Jul 10, 2017

SparkQA commented Jul 9, 2017

maropu Jul 10, 2017

kiszk Jul 10, 2017

SparkQA commented Jul 10, 2017

SparkQA commented Jul 10, 2017

maropu commented Jul 10, 2017

SparkQA commented Jul 10, 2017

kiszk commented Jul 10, 2017

gatorsmile commented Jul 13, 2017

gatorsmile left a comment

dongjoon-hyun left a comment

SparkQA commented Jul 14, 2017

kiszk commented Jul 14, 2017

viirya Jul 14, 2017

kiszk Jul 14, 2017

viirya Jul 14, 2017

kiszk Jul 14, 2017

viirya Jul 14, 2017

kiszk Jul 14, 2017

SparkQA commented Jul 14, 2017

gatorsmile commented Jul 14, 2017

viirya commented Jul 14, 2017

gatorsmile commented Jul 15, 2017

[SPARK-21344][SQL] BinaryType comparison does signed byte array comparison #18571

[SPARK-21344][SQL] BinaryType comparison does signed byte array comparison #18571

Conversation

kiszk commented Jul 8, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jul 8, 2017

srowen commented Jul 8, 2017

kiszk commented Jul 8, 2017 • edited Loading

maropu commented Jul 9, 2017

kiszk commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 10, 2017

SparkQA commented Jul 10, 2017

maropu commented Jul 10, 2017

SparkQA commented Jul 10, 2017

kiszk commented Jul 10, 2017

gatorsmile commented Jul 13, 2017

gatorsmile left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Jul 14, 2017

kiszk commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 14, 2017

gatorsmile commented Jul 14, 2017

viirya commented Jul 14, 2017

gatorsmile commented Jul 15, 2017

kiszk commented Jul 8, 2017 •

edited

Loading

kiszk commented Jul 8, 2017 •

edited

Loading