linliu-code commented on PR #19908:
URL: https://github.com/apache/hudi/pull/19908#issuecomment-5650999365
One thing I would like a second opinion on, since this change makes it
reachable.
`BufferedRecordMergerFactory.shouldKeepNewerRecord` short-circuits when
either side is a commit time ordered delete, and its own comment gives the
reason: the default ordering value is a constant `0` (an `Integer`) and is "not
guaranteed to match the type of the old or new record's ordering value". Past
that early return it does a bare `compareTo`:
```java
private static <T> boolean shouldKeepNewerRecord(BufferedRecord<T>
oldRecord, BufferedRecord<T> newRecord) {
if (newRecord.isCommitTimeOrderingDelete() ||
oldRecord.isCommitTimeOrderingDelete()) {
return true;
}
return
newRecord.getOrderingValue().compareTo(oldRecord.getOrderingValue()) >= 0;
}
```
The sibling `deltaMergeDeleteRecord` guards the same comparison with
`OrderingValues.isSameClass` before calling `compareTo`.
`shouldKeepNewerRecord` does not.
Before this change, every delete on the widened path carried the default and
took the early return, so it never reached that line. Now those deletes carry a
real ordering value and fall through to it. The case I could not rule out is an
ordering column written as `int` in an older base file and later evolved to
`long`: the stored value is an `Integer`, the incoming delete's is a `Long`,
and `Long.compareTo(Integer)` would throw.
I could not build a repro. On the payload path both sides come from the same
field through the same accessor, and `HoodieMergeHelper` composes a schema
evolution transformer for the base file reader, either of which may close it.
The missing guard is pre-existing and not introduced here; what changed is that
deletes can now reach it. Does anyone know whether schema evolution can produce
mismatched ordering value classes on that path, or is it already closed?
--
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]
