PDGGK opened a new pull request, #9249:
URL: https://github.com/apache/paimon/pull/9249
### Purpose
`collect(distinct)` and `merge_map` compare their elements and keys with
`Object.equals`. When those are `BINARY` or `VARBINARY` the value is a
`byte[]`, which inherits identity equality, so nothing with the same content is
ever recognised as the same thing.
Measured on `master` (`91ce4d6`), before any change:
| aggregator | input | result | expected |
|---|---|---|---|
| `collect(distinct)` on `ARRAY<BYTES>` | `[0x0102]` merged with `[0x0102]`
| **2 elements** | 1 |
| `merge_map` on `MAP<BYTES, INT>` | `{0x0102: 1}` merged with `{0x0102: 2}`
| **2 entries** | 1 |
| `merge_map` retract | `{0x0102: 1}` retract `{0x0102: 1}` | **1 entry** |
0 |
The middle row is the worst of the three: the aggregator hands back a
`GenericMap` containing the same logical key twice, which is not a well-formed
map at all. The third means a retraction on such a table never removes anything.
### Why it happens
**`FieldCollectAgg`** builds a generated `RecordEqualiser` and uses it
instead of `Object.equals`, but only for constructed element types:
```java
if (distinct
&& dataType.getElementType().getTypeRoot().getFamilies()
.contains(DataTypeFamily.CONSTRUCTED)) {
```
`BINARY` and `VARBINARY` are `(PREDEFINED, BINARY_STRING)` — not
`CONSTRUCTED` — so `equaliser` stays null, `agg` falls into the `new
HashSet<>()` branch, and the private `equals(a, b)` that `retract` uses
degrades to `a.equals(b)` on two `byte[]`.
**`FieldMergeMapAgg`** has no equaliser at all: it keys a `HashMap` directly
with `keyGetter.getElementOrNull(...)`, and its `retract` probes a `HashSet` of
the same raw keys.
### What changes
**`FieldCollectAgg`** — the element type is routed to the existing equaliser
path when it is binary as well as when it is constructed. The generated
equaliser already compares `VARBINARY` by value; I checked that directly rather
than assuming:
```java
RecordEqualiser eq =
newRecordEqualiser(singletonList(DataTypes.VARBINARY(10)));
eq.equals(GenericRow.of(new byte[] {1, 2}), GenericRow.of(new byte[] {1,
2})) // true
eq.equals(GenericRow.of(new byte[] {1, 2}), GenericRow.of(new byte[] {1,
3})) // false
```
The gate on `distinct` stays. It has to: `agg` treats a non-null equaliser
as "deduplicate", so building one unconditionally would make plain `collect`
start dropping duplicates.
**`FieldMergeMapAgg`** — binary keys are held in `ByteArrayKey`
(`paimon-common`, already used by `PartitionDictionary`,
`ManifestEntryRunMergeEntry` and `DataEvolutionRowIdAssignmentPlanner`) while
they sit in the hash collections, and unwrapped when the `GenericMap` is built.
`ByteArrayKey.bytes()` becomes `public` for that; it was package-private and
the class exists for nothing else.
### Blast radius
Nothing changes for a non-binary key or element. `binaryKey` is false for
every other type, so `hashKey`/`originalKey` are the identity and
`toGenericMap` returns `new GenericMap(map)` — the same object construction as
before. In `FieldCollectAgg` the added branch is reachable only for
`BINARY`/`VARBINARY`, which is exactly the case that is broken today. All 100
pre-existing cases in `FieldAggregatorTest` pass unchanged.
### Tests
Four, in `FieldAggregatorTest`: distinct collect and its retraction on
`ARRAY<VARBINARY>`, merge and retraction on `MAP<VARBINARY, INT>`. Each
mutation is isolated — reverting one fix fails only its own two:
| reverted | failures |
|---|---|
| `needsEqualiser` back to `CONSTRUCTED` only |
`testFieldCollectAggWithDistinctBinary`,
`testFieldCollectAggRetractWithDistinctBinary` |
| `hashKey` back to the identity | `testFieldMergeMapAggWithBinaryKey`,
`testFieldMergeMapAggRetractWithBinaryKey` |
`FieldAggregatorTest` (100), `FieldAggregatorRetractNullTest` (19) and
`AggregateMergeFunctionTest` (3): 122 tests, 0 failures. `spotless:apply` and
`checkstyle:check` on `paimon-common` and `paimon-core` are clean.
### Not in this PR
`FieldCollectAgg.retract` uses `equals(a, b)`, which is the raw
`Object.equals` whenever `distinct` is false — so retraction of a non-distinct
`collect` still compares constructed and binary elements by identity. That gap
predates this change and applies to `ROW`/`ARRAY`/`MAP` elements too, so fixing
it means giving `retract` an equality function of its own rather than borrowing
the one `agg` uses for deduplication. Happy to do that separately if you want
it.
### API and Format
No change to any option, on-disk format or public signature, other than
widening `ByteArrayKey.bytes()` from package-private to public.
--
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]
