[GitHub] [spark] mazeboard commented on a change in pull request #24367: [SPARK-27457][SQL] modify bean encoder to support avro objects

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

mazeboard commented on a change in pull request #24367: [SPARK-27457][SQL] modify bean encoder to support avro objects
URL: https://github.com/apache/spark/pull/24367#discussion_r276998740
 
 

 ##########
 File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
 ##########
 @@ -308,6 +309,26 @@ class ExpressionEncoderSuite extends CodegenInterpretedPlanTest with AnalysisTes
   encodeDecodeTest(Option("abc"), "option of string")
   encodeDecodeTest(Option.empty[String], "empty option of string")
 
+  encodeDecodeTest(
 
 Review comment:
   The issue here is how the `input` and the `convertedBack` objects are compared
   
   if we replace the check, in line ExpressionEncoderSuite.scala:442,
   
   `left.asInstanceOf[Comparable[Any]].compareTo(right) == 0`
   By
   `left.asInstanceOf[Comparable[Any]].equals(right) == 0`
   
   the test for Avro encoder passes, but unfortunately other tests fail
   
   Equality of objects is tricky.
   
   The GenericData `compare` function 
   ```
     /** Comparison implementation.  When equals is true, only checks for equality,
      * not for order. */
     @SuppressWarnings(value="unchecked")
     protected int compare(Object o1, Object o2, Schema s, boolean equals) {
   ```
   fails to compare Maps when the parameter `equals` is `false`
   
   
   I propose the following
   
   replace ExpressionEncoderSuite:434:444 lines
   
   ```
         val isCorrect = (input, convertedBack) match {
           case (b1: Array[Byte], b2: Array[Byte]) => Arrays.equals(b1, b2)
           case (b1: Array[Int], b2: Array[Int]) => Arrays.equals(b1, b2)
           case (b1: Array[Array[_]], b2: Array[Array[_]]) =>
             Arrays.deepEquals(b1.asInstanceOf[Array[AnyRef]], b2.asInstanceOf[Array[AnyRef]])
           case (b1: Array[_], b2: Array[_]) =>
             Arrays.equals(b1.asInstanceOf[Array[AnyRef]], b2.asInstanceOf[Array[AnyRef]])
           case (left: Comparable[_], right: Comparable[_]) =>
             left.asInstanceOf[Comparable[Any]].compareTo(right) == 0
           case _ => input == convertedBack
         }
   ```
   
   by 
   
   ```
         val convertedBackRow = encoder.toRow(convertedBack)
         val isCorrect = row == convertedBackRow
   ```
   
   With the proposed modification all the tests in ExpressionEncoderSuite passes
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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