[SPARK-8432] [SQL] fix hashCode() and equals() of BinaryType in Row #6876

davies · 2015-06-18T08:02:24Z

Also added more tests in LiteralExpressionSuite

SparkQA · 2015-06-18T08:10:32Z

Test build #35112 has finished for PR 6876 at commit 53c38b1.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2015-06-18T09:58:49Z

sql/catalyst/src/main/java/org/apache/spark/sql/BaseRow.java

+    for (int i = 0; i < arr.length; i++) {
+      hash = hash * 37 + (int)arr[i];
+    }
+    return hash;


Is it same with java.util.Arrays.hashCode?

Good idea, we should use that.

SparkQA · 2015-06-18T10:06:07Z

Test build #35114 has finished for PR 6876 at commit 0fff25d.

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

SparkQA · 2015-06-18T17:41:45Z

Test build #35144 has finished for PR 6876 at commit 5819d33.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-18T19:44:01Z

Test build #35146 has finished for PR 6876 at commit 6ad2a90.

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

davies · 2015-06-18T20:51:22Z

@marmbrus Could you help to review this one?

marmbrus · 2015-06-18T22:45:43Z

sql/catalyst/src/main/java/org/apache/spark/sql/BaseRow.java

-  /**
-   * A generic version of Row.equals(Row), which is used for tests.
-   */
-  @Override


Existing: can you add some javadoc to this class to explain what its used for and why its in Java?

Because Row is a trait, UnsafeRow and SpecificRow are both in Java, they can not inherit some default implementations from Row, so created BaseRow in Java for them. Right now, we have InternalRow, will be clean these in another PR.

marmbrus · 2015-06-18T22:48:12Z

@davies, thanks for working on this! I'm okay with this approach, but did you consider the alternative, where we instead change the internal type of BytesType to be a wrapper class instead of Array[Byte], and define a good equality function for that wrapper?

davies · 2015-06-18T23:40:04Z

@marmbrus We're working to have more efficient representation in catalyst, putting Array[Byte] inside a wrapper sounds not in the same direction. I'd like to go this approach.

marmbrus · 2015-06-18T23:58:51Z

I think using a wrapper might be necessary for efficiency. For example, we will want to reuse the same byte array when reading from something like parquet, instead of needing to allocate one of the exact size each time (think java.nio.ByteBuffer). However, I'm fine merging this first.

SparkQA · 2015-06-19T02:43:17Z

Test build #35189 timed out for PR 6876 at commit d96929b after a configured wait of 175m.

cloud-fan · 2015-06-19T03:32:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/InternalRow.scala

+  override def copy(): InternalRow = this
+
+  override def equals(o: Any): Boolean = {
+    if (!o.isInstanceOf[Row]) {


Will we change it to isInstanceOf[InternalRow] after #6869?

rxin · 2015-06-19T06:35:19Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralExpressionSuite.scala

@@ -31,8 +45,16 @@ class LiteralExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
  }

  test("int literals") {
-    checkEvaluation(Literal(1), 1)
-    checkEvaluation(Literal(0L), 0L)
+    List(0, 1, Int.MinValue, Int.MaxValue).foreach {


this is a pretty weird way of indenting. you can do

List(0, 1, Int.MinValue, Int.MaxValue).foreach { d => ... }

or

for (d <- List(0, 1, Int.MinValue, Int.MaxValue)) { ... }

SparkQA · 2015-06-19T06:50:38Z

Test build #35225 has finished for PR 6876 at commit 41caec6.

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

SparkQA · 2015-06-19T18:05:59Z

Test build #35283 has finished for PR 6876 at commit bd20780.

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

SparkQA · 2015-06-19T18:17:01Z

Test build #937 has finished for PR 6876 at commit a0626ed.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-19T18:49:20Z

Test build #35284 has finished for PR 6876 at commit a0626ed.

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

marmbrus · 2015-06-19T18:56:58Z

test this please

SparkQA · 2015-06-19T20:28:22Z

Test build #939 has finished for PR 6876 at commit a0626ed.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class SerializableConfiguration(@transient var value: Configuration) extends Serializable
- class SerializableJobConf(@transient var value: JobConf) extends Serializable

SparkQA · 2015-06-19T20:42:59Z

Test build #35307 has finished for PR 6876 at commit a0626ed.

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

SparkQA · 2015-06-19T23:25:46Z

Test build #35322 has finished for PR 6876 at commit 32d9811.

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

cloud-fan · 2015-06-20T14:07:12Z

...st/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateProjection.scala

@@ -127,6 +127,7 @@ object GenerateProjection extends CodeGenerator[Seq[Expression], Projection] {
        case FloatType => s"Float.floatToIntBits($col)"
        case DoubleType =>
            s"(int)(Double.doubleToLongBits($col) ^ (Double.doubleToLongBits($col) >>> 32))"
+        case BinaryType => s"java.util.Arrays.hashCode($col)"


Should we also update equals for generated code?

That's already done

Ah I see, genEqual has already handled BinaryType.

SparkQA · 2015-06-21T00:30:30Z

Test build #946 has finished for PR 6876 at commit 32d9811.

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

Conflicts: unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

SparkQA · 2015-06-22T22:36:17Z

Test build #35483 has finished for PR 6876 at commit 429c2c0.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class UnresolvedAlias(child: Expression) extends NamedExpression
- abstract class ExtractValueWithStruct extends ExtractValue

marmbrus · 2015-06-23T18:55:52Z

Thanks, merging to master.

fix hashCode() and equals() of BinaryType in Row

53c38b1

davies mentioned this pull request Jun 18, 2015

[WIP] remove expensive api from InternalRow #6869

Closed

fix style

0fff25d

cloud-fan reviewed Jun 18, 2015
View reviewed changes

unify equals() and hashCode()

5819d33

fix style

6ad2a90

davies mentioned this pull request Jun 18, 2015

[SPARK-8446] [SQL] Add helper functions for testing SparkPlan physical operators #6885

Closed

marmbrus reviewed Jun 18, 2015
View reviewed changes

address comment

d96929b

cloud-fan reviewed Jun 19, 2015
View reviewed changes

change for to while

41caec6

rxin reviewed Jun 19, 2015
View reviewed changes

Davies Liu added 3 commits June 19, 2015 09:58

check with catalyst types

bd20780

fix style

89c2432

Merge branch 'master' of github.com:apache/spark into fix_hashcode

a0626ed

fix test

32d9811

cloud-fan reviewed Jun 20, 2015
View reviewed changes

Merge branch 'master' of github.com:apache/spark into fix_hashcode

429c2c0

Conflicts: unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

asfgit closed this in 6f4cadf Jun 23, 2015

davies mentioned this pull request Jun 23, 2015

[SPARK-8371][SQL] improve unit test for MaxOf and MinOf and fix bugs #6825

Closed

[SPARK-8432] [SQL] fix hashCode() and equals() of BinaryType in Row #6876

[SPARK-8432] [SQL] fix hashCode() and equals() of BinaryType in Row #6876

Uh oh!

Conversation

davies commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

davies commented Jun 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Jun 18, 2015

Uh oh!

davies commented Jun 18, 2015

Uh oh!

marmbrus commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 19, 2015

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 Jun 19, 2015

Uh oh!

SparkQA commented Jun 19, 2015

Uh oh!

SparkQA commented Jun 19, 2015

Uh oh!

SparkQA commented Jun 19, 2015

Uh oh!

marmbrus commented Jun 19, 2015

Uh oh!

SparkQA commented Jun 19, 2015

Uh oh!

SparkQA commented Jun 19, 2015

Uh oh!

SparkQA commented Jun 19, 2015

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 Jun 21, 2015

Uh oh!

SparkQA commented Jun 22, 2015

Uh oh!

marmbrus commented Jun 23, 2015

Uh oh!

Uh oh!