Neuw84 commented on issue #18618:
URL: https://github.com/apache/iceberg/issues/18618#issuecomment-6358191298
Reviewed against `apache/iceberg@main`. The bug is real and reproduces as
described; one correction to the root-cause attribution so a fix targets the
right line.
### `FixedReader` aliasing: confirmed
`GenericParquetReaders.FixedReader.read` only returns a fresh array when
`reuse == null`:
```java
public byte[] read(byte[] reuse) {
if (reuse != null) {
column.nextBinary().toByteBuffer().duplicate().get(reuse);
return reuse; // same array instance every row, mutated in place
} else {
return column.nextBinary().getBytes(); // fresh array
}
}
```
Under record reuse the `fixed` slot holds a non-null `byte[]`, so every row
returns the **same** array reference, overwritten as the file is iterated.
`int`/`long`/`string` are immune because they produce fresh immutable values
per row. Agreed on this part.
### Correction: `readEqDeletes` already copies — the failure is that the
copy is shallow over an aliased array
The issue says `readEqDeletes` passes records straight through
`InternalRecordWrapper` to `StructLikeSet.add` with no copy. Current `main`
actually does copy each record first:
```java
private Iterable<StructLike> readEqDeletes(DeleteFile deleteFile, Schema
projection) {
CloseableIterable<Record> deletes = openDeletes(deleteFile, projection);
CloseableIterable<Record> copiedDeletes =
CloseableIterable.transform(deletes, Record::copy); // per-record
copy
CloseableIterable<StructLike> copiedDeletesAsStructs =
toStructs(copiedDeletes, projection);
return materialize(copiedDeletesAsStructs);
}
```
So the defensive copy exists — but `GenericRecord.copy()` copies field-value
**references**, and since `FixedReader` handed every row the *same* `byte[]`,
`copy()` faithfully preserves N references to one mutated array. The copy that
should have saved us is defeated upstream by the reader's aliasing. The bug
therefore originates in `FixedReader`, not in `readEqDeletes`.
### Recommended fix: `FixedReader.read` (your option A)
Returning a fresh array at the reader is the correct, localized fix:
- it is where the aliasing originates, so it makes the existing
`Record::copy` sound rather than patching one call site;
- it fixes **every** consumer of the generic reader that reasonably assumes
distinct per-row values, not just equality-delete loading.
Simplest form is to drop the reuse branch and always return
`column.nextBinary().getBytes()`. If the per-row allocation that the reuse path
was designed to avoid is a concern for hot generic scans, the surgical form is
`return reuse.clone();` after filling `reuse`. For a `fixed(L)` column (almost
always a key/ID) the allocation delta is negligible, so unconditional-fresh is
the clean choice.
Deep-copying inside `readEqDeletes` (option B) is not preferred: it
duplicates the copy that already exists and leaves the reader footgun in place
for other callers.
I'll open a PR with the `FixedReader` fix plus a failing test that asserts
the loaded `StructLikeSet` has the right cardinality for a `fixed`-keyed
equality delete.
--
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]
