[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

      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

      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 <[email protected]> Closes #18571 from kiszk/SPARK-21344. (cherry picked from commit ac5d5d7) Signed-off-by: gatorsmile <[email protected]>

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 <[email protected]> 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 <[email protected]> Closes apache#18571 from kiszk/SPARK-21344. (cherry picked from commit ac5d5d7) Signed-off-by: gatorsmile <[email protected]>

…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 <[email protected]> Closes apache#18571 from kiszk/SPARK-21344. (cherry picked from commit ac5d5d7) Signed-off-by: gatorsmile <[email protected]>

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

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

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

Uh oh!

Conversation

kiszk commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 8, 2017

Uh oh!

srowen commented Jul 8, 2017

Uh oh!

kiszk commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Jul 9, 2017

Uh oh!

kiszk commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 10, 2017

Uh oh!

SparkQA commented Jul 10, 2017

Uh oh!

maropu commented Jul 10, 2017

Uh oh!

SparkQA commented Jul 10, 2017

Uh oh!

kiszk commented Jul 10, 2017

Uh oh!

gatorsmile commented Jul 13, 2017

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 14, 2017

Uh oh!

kiszk commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 14, 2017

Uh oh!

gatorsmile commented Jul 14, 2017

Uh oh!

viirya commented Jul 14, 2017

Uh oh!

gatorsmile commented Jul 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

kiszk commented Jul 8, 2017 •

edited

Loading

kiszk commented Jul 8, 2017 •

edited

Loading