linliu-code opened a new issue, #19901:
URL: https://github.com/apache/hudi/issues/19901
Two related design choices make ordering-value bugs representable, and both
are upstream of
#19899 / #19900.
**1. "No ordering value" is encoded as a value that is also legal, and whose
type differs from
most ordering columns.**
`HoodieRecord.DEFAULT_ORDERING_VALUE` is `public static final int
DEFAULT_ORDERING_VALUE = 0`, and
`OrderingValues.getDefault()` hands out its boxed `Integer`. So "absent" and
"the ordering column
holds 0" are the same object, and `OrderingValues.isDefault(x)` cannot tell
them apart. Code that
wants to know whether a record carries an ordering value has to guess.
The type is the sharper half. Every ordering column that is not an `int`, so
`bigint`, `string`,
`timestamp`, produces a value whose class differs from the sentinel, and the
two meet in
comparisons.
**2. Ordering values are compared with a raw `compareTo` across two
independently typed
`Comparable`s.**
`BufferedRecordMergerFactory.shouldKeepNewerRecord`:
```java
private static <T> boolean shouldKeepNewerRecord(BufferedRecord<T>
oldRecord, BufferedRecord<T> newRecord) {
if (newRecord.isCommitTimeOrderingDelete() ||
oldRecord.isCommitTimeOrderingDelete()) {
// handle records coming from DELETE statements
// The orderingVal is constant 0 (int) and not guaranteed to match the
type of the old or new record's ordering value.
return true;
}
return
newRecord.getOrderingValue().compareTo(oldRecord.getOrderingValue()) >= 0;
}
```
The comment on the delete branch states the hazard precisely, then the
non-delete branch below it
does the unguarded comparison anyway. Nothing in the type system or in a
runtime check stops a
sentinel `Integer` meeting a `Long`.
**Consequences observed.** #19899 is one instance: a record whose ordering
value falls back to the
sentinel is compared against a `Long` read from storage and throws
`ClassCastException` on every
merged row. The same defect on an `int` ordering column throws nothing: the
comparison succeeds,
`0.compareTo(anyPositive)` is negative, and the update is silently
discarded. That silent variant
is the reason this is worth fixing at the representation rather than per
call site.
**Possible directions**, in increasing order of disruption:
- Guard the comparison. `OrderingValues.isSameClass` already exists and is
used in
`deltaMergeDeleteRecord`; applying the same check on the non-delete path
turns a
`ClassCastException` into a defined outcome, and makes the silent `int`
case detectable.
- Make the sentinel distinguishable, for example a dedicated singleton type
that is never equal to
a user value and compares as lowest, so `isDefault` is exact rather than
value-based.
- Represent absence outside the value, for example `Option`, so the question
"does this record
carry an ordering value" is answered by the type rather than by comparing
against 0.
The first is a contained change. The second and third touch
`BufferedRecord`, the mergers and the
delete block payload, so they need agreement before anyone writes code,
which is why this is an
issue rather than a PR.
Related: #19899, #19900.
--
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]
