Re: [PR] Spark: Fix changelog cancellation for binary values [iceberg]

["RussellSpitzer (via GitHub)" <gi...@apache.org>]

RussellSpitzer commented on PR #18209:
URL: https://github.com/apache/iceberg/pull/18209#issuecomment-6023979954

   I don't think adding a new class that redefines equality is the safest approach here. It also adds map equality handling, which we don't need: map columns can't be sorted, so they never reach these iterators.
   
   I sketched out a few alternatives, and I'd recommend one of these two:
   
   1. Add a small nested comparison inside `ChangelogIterator` that does what our current check does (`Objects.equals`), but applies it recursively through arrays and structs so binary values are compared by content at any depth.
   
   ```java
     protected boolean isDifferentValue(Row currentRow, Row nextRow, int idx) {
       return !valuesEqual(currentRow.get(idx), nextRow.get(idx));
     }
     /**
      * Compares values the way {@link Objects#equals} does, except that binary values are compared by
      * content at any depth within arrays and structs.
      */
     private static boolean valuesEqual(Object left, Object right) {
       if (left instanceof byte[] leftBytes && right instanceof byte[] rightBytes) {
         return Arrays.equals(leftBytes, rightBytes);
       } else if (left instanceof Seq<?> leftSeq && right instanceof Seq<?> rightSeq) {
         return seqsEqual(leftSeq, rightSeq);
       } else if (left instanceof Row leftRow && right instanceof Row rightRow) {
         return rowsEqual(leftRow, rightRow);
       }
       return Objects.equals(left, right);
     }
     private static boolean seqsEqual(Seq<?> left, Seq<?> right) {
       if (left.size() != right.size()) {
         return false;
       }
       Iterator<?> leftValues = CollectionConverters.asJava(left).iterator();
       Iterator<?> rightValues = CollectionConverters.asJava(right).iterator();
       while (leftValues.hasNext()) {
         if (!valuesEqual(leftValues.next(), rightValues.next())) {
           return false;
         }
       }
       return true;
     }
     private static boolean rowsEqual(Row left, Row right) {
       if (left.size() != right.size()) {
         return false;
       }
       for (int index = 0; index < left.size(); index++) {
         if (!valuesEqual(left.get(index), right.get(index))) {
           return false;
         }
       }
       return true;
     }
   ```
   This keeps the change local to `ChangelogIterator`, leaves the subclasses and the procedure untouched, and passes the binary tests in this PR, minus the map cases.
   
   2. Rework this code to run on Spark's `InternalRow` directly and compare with Spark's own ordering. That would give us Spark's comparison semantics without defining any equality of our own, and it skips converting every row to an external `Row`, so it should be cheaper than what we have today. It's a broader refactor, though, and it changes the `ChangelogIterator` signatures. I have a prototype, but I think it should be a follow-up if we do it at all.
   I'd suggest going with option 1 here and doing option 2 potentially later.
   
   I'm also open for any other approaches but let's try to keep them contained and try not to require any additional allocation or definitions of equality. 


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org