PDGGK commented on PR #9249:
URL: https://github.com/apache/paimon/pull/9249#issuecomment-5308033423
Good catch — it does, and the consequence there is worse than in
`FieldMergeMapAgg`. Extended in `769f164`.
`mergeInputMap` uses the key for two lookups, not just for insertion:
```java
if (newRow == null) {
resultMap.remove(key); // tombstone: never matches, so nothing is
removed
continue;
}
...
Object existingValue = resultMap.get(key); // always null for a byte[] key
if (existingValue == null) {
resultMap.put(key, newRow); // so this branch is always the
one taken
} else {
... newTs.compareTo(existingTs) > 0 ... // the timestamp comparison
never runs
}
```
So for a binary key the aggregator does not merely duplicate entries — the
whole last-write-wins-by-timestamp behaviour it exists for is bypassed, and an
out-of-order older row is appended rather than discarded.
The added test walks exactly that:
| step | expected | on `master` |
|---|---|---|
| `{0x0102: (A, ts=100)}` | 1 entry, `A` | 1 entry, `A` |
| merge `{0x0102: (A1, ts=200)}` | 1 entry, `A1` | **2 entries** |
| merge `{0x0102: (A0, ts=050)}` — older | 1 entry, `A1` | **3 entries** |
| merge `{0x0102: null}` — tombstone | 0 entries | **4 entries** |
Since two aggregators now need the same treatment I moved the wrapping into
one place, `BinaryMapKeys`, rather than repeating the three methods;
`FieldMergeMapAgg` now calls through it too. It stays a static helper so
nothing new has to be serialisable.
Reverting `hashKey` to the identity in `FieldMergeMapWithKeyTimeAgg` fails
only `testFieldMergeMapWithKeyTimeAggWithBinaryKey`, and each of the other two
fixes is likewise isolated to its own tests.
`FieldAggregatorTest` (101), `FieldAggregatorRetractNullTest` (19),
`AggregateMergeFunctionTest` (3): 123 tests, 0 failures. `spotless:apply` and
`checkstyle:check` clean on `paimon-common` and `paimon-core`.
--
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]
