[GitHub] [spark] MaxGekk commented on a change in pull request #34848: [SPARK-37582][SPARK-37583][SQL] CONTAINS, STARTSWITH, ENDSWITH should support binary type

[GitBox <gi...@apache.org>]

MaxGekk commented on a change in pull request #34848:
URL: https://github.com/apache/spark/pull/34848#discussion_r782200442



##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/array/ByteArrayMethods.java
##########
@@ -91,4 +91,64 @@ public static boolean arrayEquals(
     }
     return true;
   }
+
+  public static boolean contains(byte[] array, byte[] target) {
+    if (target.length == 0) {
+      return true;
+    } else if (array.length == 0) {
+      return false;
+    } else if (target.length > array.length) {
+      return false;
+    } else {
+      return indexOf(array, target) >= 0;
+    }
+  }
+
+  public static boolean startsWith(byte[] array, byte[] target) {
+    if (target.length == 0) {
+      return true;
+    } else if (array.length == 0) {
+      return false;
+    } else if (target.length > array.length) {
+      return false;
+    } else {
+      return indexOf(array, target) == 0;
+    }
+  }
+
+  public static boolean endsWith(byte[] array, byte[] target) {
+    if (target.length == 0) {
+      return true;
+    } else if (array.length == 0) {
+      return false;
+    } else if (target.length > array.length) {
+      return false;
+    } else {
+      int endPos = array.length - target.length;
+      for (int i = 0; i < target.length; ++i) {
+        if (array[endPos + i] != target[i]) {
+          return false;
+        }
+      }
+      return true;
+    }
+  }

Review comment:
       Is it possible to use optimized method `arrayEquals` in `startsWith()` and `endsWith()` as UTF8String methods do? I guess it is faster than your `indexOf()`.

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
##########
@@ -450,16 +451,32 @@ case class Lower(child: Expression)
   override protected def withNewChildInternal(newChild: Expression): Lower = copy(child = newChild)
 }
 
-/** A base trait for functions that compare two strings, returning a boolean. */
-abstract class StringPredicate extends BinaryExpression
+/** A base trait for functions that compare two strings or binaries, returning a boolean. */
+abstract class BinaryPredicate extends BinaryExpression
   with Predicate with ImplicitCastInputTypes with NullIntolerant {
 
-  def compare(l: UTF8String, r: UTF8String): Boolean
+  def compare(l: Array[Byte], r: Array[Byte]): Boolean
 
-  override def inputTypes: Seq[DataType] = Seq(StringType, StringType)
+  override def inputTypes: Seq[AbstractDataType] =
+    Seq(TypeCollection(StringType, BinaryType), TypeCollection(StringType, BinaryType))
+
+  def convertData(dataType: DataType): Any => Array[Byte] = dataType match {
+    case BinaryType => (input: Any) => input.asInstanceOf[Array[Byte]]
+    case StringType => (input: Any) => input.asInstanceOf[UTF8String].getBytes

Review comment:
       Not sure that handling of the string and binary types in `BinaryPredicate` is good approach. @cloud-fan @HyukjinKwon WDYT?

##########
File path: common/unsafe/src/main/java/org/apache/spark/unsafe/array/ByteArrayMethods.java
##########
@@ -91,4 +91,64 @@ public static boolean arrayEquals(
     }
     return true;
   }
+
+  public static boolean contains(byte[] array, byte[] target) {
+    if (target.length == 0) {
+      return true;
+    } else if (array.length == 0) {
+      return false;
+    } else if (target.length > array.length) {
+      return false;
+    } else {
+      return indexOf(array, target) >= 0;
+    }
+  }
+
+  public static boolean startsWith(byte[] array, byte[] target) {
+    if (target.length == 0) {
+      return true;
+    } else if (array.length == 0) {
+      return false;
+    } else if (target.length > array.length) {
+      return false;
+    } else {
+      return indexOf(array, target) == 0;
+    }
+  }
+
+  public static boolean endsWith(byte[] array, byte[] target) {
+    if (target.length == 0) {
+      return true;
+    } else if (array.length == 0) {
+      return false;
+    } else if (target.length > array.length) {
+      return false;
+    } else {
+      int endPos = array.length - target.length;
+      for (int i = 0; i < target.length; ++i) {
+        if (array[endPos + i] != target[i]) {
+          return false;
+        }
+      }
+      return true;
+    }
+  }

Review comment:
       Also we need to check that there is no perf regression in UTF8String methods. Please, run string benchmarks.

##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala
##########
@@ -1028,4 +1028,21 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
     checkEvaluation(Contains(Literal("Spark SQL"), Literal("SQL")), true)
     checkEvaluation(Contains(Literal("Spark SQL"), Literal("k S")), true)
   }
+
+  test("SPARK-37582, SPARK-37583: CONTAINS, STARTSWITH, ENDSWITH should support binary type") {
+    checkEvaluation(Contains(Literal("Spark SQL"), Literal("Spark")), true)
+    checkEvaluation(Contains(Literal("Spark SQL".getBytes), Literal("Spark")), true)
+    checkEvaluation(Contains(Literal("Spark SQL"), Literal("Spark".getBytes)), true)

Review comment:
       What is the purpose of those checkEvaluation()? How do they check your changes?
   
   I think need more tests for your new functions `contains`, `startsWith`, `endsWith`, and especially `indexOf` to check corner cases.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org