apache / apache/hudi

Ordering values: untyped 0 sentinel and unguarded cross-type compareTo make merge bugs representable

Open
#19,901 1 comment 0 reactions 1 assignee Claimed by @linliu-code View on GitHub
area:core priority:high
Dominant language
Java
Stars
6.2k
Forks
2.5k
Avg merge
2d 8h
Merged PRs (30d)
111

Description

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 boolean shouldKeepNewerRecord(BufferedRecord oldRecord, BufferedRecord 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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.