address comment

Davies Liu · Davies Liu · commit c8e7cd2ab883 · 2015-06-09T16:39:53.000-07:00
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/BaseRow.java b/sql/catalyst/src/main/scala/org/apache/spark/sql/BaseRow.java
@@ -148,6 +148,9 @@ public int fieldIndex(String name) {
     throw new UnsupportedOperationException();
   }
 
+  /**
+   * A generic version of Row.equals(Row), which is used for tests.
+   */
   @Override
   public boolean equals(Object other) {
     if (other instanceof Row) {
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateOrdering.scala
@@ -61,12 +61,7 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[Row]] wit
             {
               byte[] x = ${if (asc) evalA.primitive else evalB.primitive};
               byte[] y = ${if (!asc) evalB.primitive else evalA.primitive};
-              int j = 0;
-              while (j < x.length && j < y.length) {
-                if (x[j] != y[j]) return x[j] - y[j];
-                j = j + 1;
-              }
-              int d = x.length - y.length;
+              int d = org.apache.spark.sql.catalyst.util.TypeUtils.compareBinary(x, y);
               if (d != 0) {
                 return d;
               }
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
@@ -267,19 +267,8 @@ abstract class BinaryComparison extends BinaryExpression with Predicate {
           if (!${ev.isNull}) {
             ${eval2.code}
             if (!${eval2.isNull}) {
-              boolean done = false;
-              for (int j = 0; j < ${eval1.primitive}.length && j < ${eval2.primitive}.length; j++) {
-                byte a = ${eval1.primitive}[j];
-                byte b = ${eval2.primitive}[j];
-                if (a != b) {
-                  ${ev.primitive} = a $symbol b;
-                  done = true;
-                  break;
-                }
-              }
-              if (!done) {
-                ${ev.primitive} = ${eval1.primitive}.length $symbol ${eval2.primitive}.length;
-              }
+              ${ev.primitive} = org.apache.spark.sql.catalyst.util.TypeUtils.compareBinary(
+                ${eval1.primitive}, ${eval2.primitive}) $symbol 0;
             } else {
               ${ev.isNull} = true;
             }
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TypeUtils.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TypeUtils.scala
@@ -53,4 +53,12 @@ object TypeUtils {
 
   def getOrdering(t: DataType): Ordering[Any] =
     t.asInstanceOf[AtomicType].ordering.asInstanceOf[Ordering[Any]]
+
+  def compareBinary(x: Array[Byte], y: Array[Byte]): Int = {
+    for (i <- 0 until x.length; if i < y.length) {
+      val res = x(i).compareTo(y(i))
+      if (res != 0) return res
+    }
+    x.length - y.length
+  }
 }
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/types/BinaryType.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/types/BinaryType.scala
@@ -22,6 +22,7 @@ import scala.reflect.runtime.universe.typeTag
 
 import org.apache.spark.annotation.DeveloperApi
 import org.apache.spark.sql.catalyst.ScalaReflectionLock
+import org.apache.spark.sql.catalyst.util.TypeUtils
 
 
 /**
@@ -43,11 +44,7 @@ class BinaryType private() extends AtomicType {
 
   private[sql] val ordering = new Ordering[InternalType] {
     def compare(x: Array[Byte], y: Array[Byte]): Int = {
-      for (i <- 0 until x.length; if i < y.length) {
-        val res = x(i).compareTo(y(i))
-        if (res != 0) return res
-      }
-      x.length - y.length
+      TypeUtils.compareBinary(x, y)
     }
   }
 

Original file line number	Diff line number	Diff line change
`@@ -148,6 +148,9 @@ public int fieldIndex(String name) {`
`148`	`148`	`throw new UnsupportedOperationException();`
`149`	`149`	`}`
`150`	`150`
	`151`	`+ /**`
	`152`	`+ * A generic version of Row.equals(Row), which is used for tests.`
	`153`	`+ */`
`151`	`154`	`@Override`
`152`	`155`	`public boolean equals(Object other) {`
`153`	`156`	`if (other instanceof Row) {`
Original file line number	Diff line number	Diff line change
`@@ -61,12 +61,7 @@ object GenerateOrdering extends CodeGenerator[Seq[SortOrder], Ordering[Row]] wit`
`61`	`61`	`{`
`62`	`62`	`byte[] x = ${if (asc) evalA.primitive else evalB.primitive};`
`63`	`63`	`byte[] y = ${if (!asc) evalB.primitive else evalA.primitive};`
`64`		`- int j = 0;`
`65`		`- while (j < x.length && j < y.length) {`
`66`		`- if (x[j] != y[j]) return x[j] - y[j];`
`67`		`- j = j + 1;`
`68`		`- }`
`69`		`- int d = x.length - y.length;`
	`64`	`+ int d = org.apache.spark.sql.catalyst.util.TypeUtils.compareBinary(x, y);`
`70`	`65`	`if (d != 0) {`
`71`	`66`	`return d;`
`72`	`67`	`}`