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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
