Neuw84 opened a new pull request, #18621:
URL: https://github.com/apache/iceberg/pull/18621
Closes #18618.
## Problem
`GenericParquetReaders.FixedReader.read(byte[] reuse)` fills and returns the
caller's reuse buffer when it is non-null:
```java
public byte[] read(byte[] reuse) {
if (reuse != null) {
column.nextBinary().toByteBuffer().duplicate().get(reuse);
return reuse; // same array instance every row
} else {
return column.nextBinary().getBytes();
}
}
```
Under container reuse the `fixed` slot holds a non-null `byte[]`, so every
row returns the **same** array, overwritten in place as the file is iterated.
Any caller that retains the value past the next `read()` call ends up with N
references to one mutated array.
This surfaces as a correctness bug in equality-delete loading on a
`fixed(L)` key. `BaseDeleteLoader.readEqDeletes` copies each record
(`Record::copy`), but that copy is shallow over the field-value references —
and because the reader handed every row the same array, the copy faithfully
preserves N references to it. The `StructLikeSet` built from those records
therefore collapses to a single element, and the delete matches almost nothing.
`int`/`long`/`string` keys are unaffected because they produce fresh immutable
values per row.
Minimal repro from the issue: a v2 table `(id long, f fixed(4))` with an
equality delete on `f` carrying 143 distinct keys deletes only 1 row instead of
143; `BaseDeleteLoader.loadEqualityDeletes(...)` returns a `StructLikeSet` of
size 1.
## Fix
Return a fresh array from `FixedReader.read` unconditionally. `getBytes()`
already produced a fresh array on the `reuse == null` path, so this just drops
the aliasing branch. Fixing it at the reader (rather than deep-copying in
`readEqDeletes`) removes the footgun for **every** consumer of the generic
reader that reasonably assumes distinct per-row values, not just delete loading.
The per-row allocation the reuse branch avoided is negligible for a
`fixed(L)` column (typically a small key/id), and the previous behavior was
already unsafe for any retaining caller.
## Test
Adds `TestGenericData.testFixedReaderDoesNotAliasAcrossRowsWithReuse`:
writes distinct `fixed(4)` values, reads them back with `reuseContainers()`
(the path that triggered the aliasing), retains each value as a delete loader
would, and asserts no two retained values are the same array instance and each
still equals what was written. The test fails against the old reader (all
values alias the last row) and passes with this change.
## Build / test note
I was unable to run the Gradle suite locally — this environment only has JDK
25 and the Iceberg build requires JDK 17 or 21 (`This build must be run with
JDK 17 or 21`). The change is small and the fix path (`getBytes()`) is the one
the method already used when `reuse == null`; CI will exercise the new test
under a supported JDK.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
