[I] Equality delete on a fixed column deletes almost nothing: StructLikeSet collapses to one key [iceberg]

["Neuw84 (via GitHub)" <gi...@apache.org>]

Neuw84 opened a new issue, #18618:
URL: https://github.com/apache/iceberg/issues/18618

   ### Apache Iceberg version
   
   1.12.0 (iceberg-spark-runtime-4.1_2.13, Spark 4.1.3)
   
   ### Query engine
   
   Spark (also affects any engine that loads equality deletes through `BaseDeleteLoader` with the generic Parquet reader)
   
   ### Please describe the bug 🐞
   
   An equality delete file keyed on a `fixed(L)` column deletes almost nothing on read: N distinct keys behave like a single key.
   
   **Repro**
   
   1. v2 table `(id long, f fixed(4))`, `write.delete.mode=merge-on-read`; insert 1000 rows with a distinct `f` per row (e.g. the 4 big-endian bytes of `id`).
   2. Write one Parquet equality delete file on `f` with 143 distinct keys (`id` = 0, 7, 14, ..., 994) using `GenericFileWriterFactory` (or `SparkFileWriterFactory`), commit it with `newRowDelta().addDeletes(...)`.
   3. `SELECT id FROM t`: 999 rows remain instead of 857. Same with the vectorized and the row-based Spark reader.
   
   Loading the delete file directly with `BaseDeleteLoader.loadEqualityDeletes(...)` returns a `StructLikeSet` of **size 1**, and `set.contains(<one of the real keys>)` is `false`.
   
   **Root cause**
   
   - `org.apache.iceberg.data.parquet.GenericParquetReaders$FixedReader.read(byte[] reuse)` fills and returns the caller's reuse buffer (`column.nextBinary().toByteBuffer().duplicate().get(reuse); return reuse;`), so every record's fixed value is the same `byte[]` instance, mutated as the file is iterated.
   - `BaseDeleteLoader.readEqDeletes` passes the records (through a flyweight `InternalRecordWrapper`) to `StructLikeSet.add`, which goes through `StructLikeWrapper.copyFor(s)` -> `set(s)`, which keeps only the `StructLike` reference and does not copy field values.
   
   All N delete records end up aliasing one array, so after the file is read they all compare equal to the last value and the set collapses to one element. `int`/`long`/`string` keys are not affected because their values are fresh immutable objects per row; `fixed` (and any reused-buffer binary value) is.
   
   **Possible fix**
   
   Return a fresh array in `FixedReader.read` (or do not reuse for delete loading), or deep-copy field values when building the equality-delete `StructLikeSet` in `readEqDeletes`.
   
   Found while building a native (vectorized) Iceberg scan for Spark that loads equality deletes through Iceberg's own loader; we now read the delete file ourselves and copy each key as it is read, which deletes the spec-correct rows.
   
   ### Willingness to contribute
   
   - [x] I can contribute a fix for this bug independently
   


-- 
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: issues-unsubscribe@iceberg.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [I] Equality delete on a fixed column deletes almost nothing: StructLikeSet collapses to one key [iceberg]

["Neuw84 (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

Re: [I] Equality delete on a fixed column deletes almost nothing: StructLikeSet collapses to one key [iceberg]

["Neuw84 (via GitHub)" <gi...@apache.org>]

Neuw84 commented on issue #18618:
URL: https://github.com/apache/iceberg/issues/18618#issuecomment-6358233637

   Opened #18621 with the fix: `FixedReader.read` now returns a fresh array unconditionally (dropping the reuse-buffer aliasing branch), plus a regression test in `TestGenericData` that reads distinct `fixed(4)` values with `reuseContainers()` and asserts no two retained values share a `byte[]` instance.


-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org