Angelia-Wang opened a new pull request, #934:
URL: https://github.com/apache/iceberg-cpp/pull/934

   ## What
   `AppendIntList`, `AppendIntMap`, and `AppendBinaryMap` in 
`src/iceberg/arrow_row_builder.cc` always call `ArrowArrayFinishElement()` 
after appending entries, even when the input container is empty. As a result, 
whenever one of DataFile's optional list/map fields is unset, the C++ writer 
encodes an **empty-but-non-null** list/map element in the Avro manifest instead 
of a **null** element.
   
   These three helpers back all of DataFile's optional list/map fields in 
`manifest_adapter.cc`:
   - `split_offsets`, `equality_ids` (optional list)
   - `column_sizes`, `value_counts`, `null_value_counts`, `nan_value_counts`, 
`lower_bounds`, `upper_bounds` (optional map)
   
   ## Why
   The Java reference implementation treats "unset" as null for these same 
fields: `BaseFile` initializes `columnSizes`, `valueCounts`, `nullValueCounts`, 
`nanValueCounts`, `lowerBounds`, `upperBounds`, `splitOffsets`, and 
`equalityIds` to `null` (not empty containers), and its `get(pos, ...)` 
accessor — used by the Avro manifest writer — returns `null` for an unset field 
(https://github.com/apache/iceberg/blob/1.1.x/core/src/main/java/org/apache/iceberg/BaseFile.java).
   So manifests written by iceberg-cpp currently diverge from manifests written 
by iceberg-java whenever any of these 8 fields is unset.
   
   This divergence is not cosmetic — it causes **silent data loss when the 
table is read back with iceberg-java**. We verified this end-to-end: after 
iceberg-cpp commits a snapshot, reading the table with 
`IcebergGenerics.read(table)` (iceberg 1.1.0) returns **0 rows** instead of the 
committed rows:
   
   1. `split_offsets` unset → encoded as empty Avro array `[]` instead of 
`null`.
   2. `BaseFile.splitOffsets()` converts the empty array into a non-null 
**empty** `List<Long>` 
(https://github.com/apache/iceberg/blob/1.1.x/core/src/main/java/org/apache/iceberg/BaseFile.java).
   3. `BaseContentScanTask.split()` selects 
`OffsetsAwareSplitScanTaskIterator`, because `file.splitOffsets() != null && 
OFFSET_ORDERING.isOrdered(...)` holds for an empty list 
(https://github.com/apache/iceberg/blob/1.1.x/core/src/main/java/org/apache/iceberg/BaseContentScanTask.java).
   4. `OffsetsAwareSplitScanTaskIterator` produces **zero split tasks** for an 
empty offset list: `splitSizes` stays empty and `hasNext()` always returns 
false 
(https://github.com/apache/iceberg/blob/1.1.x/core/src/main/java/org/apache/iceberg/OffsetsAwareSplitScanTaskIterator.java).
   5. The scan silently reads nothing — no exception, no warning.
   
   With this change (null instead of empty list), the same end-to-end test 
reads back the expected rows: a `null` `split_offsets` makes 
`BaseContentScanTask.split()` fall back to `FixedSizeSplitScanTaskIterator` and 
the file is read normally.
   
   ## How
   Add an empty check to each of the three helpers that delegates to the 
existing `AppendNull()` instead of finishing an empty element. This covers all 
8 optional list/map fields with one consistent change; `manifest_adapter.cc` is 
untouched. `AppendStringMap` (used for required properties-style maps) is 
intentionally left unchanged: an empty-but-non-null map is the expected 
encoding for required maps (covered by the existing 
`ArrowRowBuilderTest.BuildsRowsWithTypedValues`).
   
   ## Testing
   - New unit tests in `arrow_row_builder_test.cc` assert that 
`AppendIntList`/`AppendIntMap`/`AppendBinaryMap` write a null element (not an 
empty one) for empty input, alongside existing non-empty coverage.
   - Existing manifest round-trip tests are unaffected: the C++ read path 
already normalizes both null and empty to empty containers.
   - End-to-end verification (C++ writer → snapshot commit → Java 
`IcebergGenerics.read` reader): without the fix, 0 rows are read back; with the 
fix, all committed rows are read back.
   


-- 
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]

Reply via email to