manuzhang opened a new pull request, #1109: URL: https://github.com/apache/iceberg-cpp/pull/1109
## Summary `HasIdVisitor::HasNoIds()` checked `total_fields_ == 0`, so it was only true for a schema with no fields at all. For any Avro file whose fields have no IDs, `AvroReader` skipped the name-mapping branch and failed with `Not all fields in the Avro file schema have field IDs`, even when a name mapping was passed. Because that branch was unreachable, nothing tested the mapping path end to end, and it had more bugs that this PR also fixes. - **`HasNoIds()`** now returns true when no field has an ID. Its doc comment, copied from `AllHaveIds()`, is corrected too. - **No name mapping:** the reader builds a mapping from the read schema's names, as Java's `ProjectionDatumReader` does with `MappingUtil.create(expectedSchema)`. This matches the Parquet fallback in #1108. - **Fields the mapping doesn't cover:** `MakeAvroNodeWithFieldIds` failed with `Field '...' not found in name mapping` for any file field missing from the mapping. With the fallback, that would be every column outside the projection. Like Java's `ApplyNameMapping`, such fields, and fields mapped without an ID, are now kept without an ID. - **Projection:** like Java's `PruneColumns`, record fields without an ID are skipped. A list or map whose element, key or value has no ID is read as if it were missing from the file: its initial default, null if optional, or an error if required. - **Mapped maps:** `MakeMapNodeWithFieldIds` added key and value leaves to a `NodeMap`, which already has the implicit string key, so the map ended up with three leaves. It also put `value-id` in a second attribute set, while projection reads both IDs from the first. Mapped maps are now built the way `ToAvroNodeVisitor` builds them for writing. - **Mapped array-backed maps** keep the `map` logical type. Without it, `ProjectMap` rejected them. - **Direct decoder:** the null branch of a union is now consumed with `decodeNull()`. The binary decoder doesn't need this, but a mapped schema differs from the file's schema, so avro-cpp reads through a resolving decoder. That decoder failed on the next field with `Schema requires: Null, got: Long`. ### Not changed - **Files where only some fields have IDs** still fail with `Not all fields in the Avro file schema have field IDs`. Java reads them, taking any missing IDs from the name mapping when there is one. That's a separate behavior change, left for its own discussion. ## Testing - Added `ReadWithoutFieldIdsOrNameMapping`, `ReadWithNameMappingWithoutFieldIds`, `ReadNestedFieldsWithoutFieldIds` and `ReadContainersWithUnmappedChildrenAsMissing` to `avro_test`. They write Avro files without IDs through avro-cpp directly and run against both the direct-decoder and GenericDatum backends. - Updated `HasIdVisitorTest.ArrayWithoutElementId` and added `RecordWithoutFieldIds`. - `ApplyNameMappingToMap` now checks that the result has the writer's layout and can be projected. Added `ApplyNameMappingToArrayBackedMap`, and replaced `MissingFieldIdError` with `UnmappedFieldsHaveNoFieldIds`. - Local macOS Release build (Arrow vendored, REST off): all 19 `ctest` suites pass, and clang-tidy 22.1.8 (the CI version) reports nothing new on the changed files. Its one warning, at `schema_util.h:38`, also appears on `main`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
