manuzhang opened a new pull request, #1108: URL: https://github.com/apache/iceberg-cpp/pull/1108
## Summary Follow-up to #1044, fixing issues found while reviewing how Parquet files without field IDs are read. - **Thread safety:** `NameMapping` and `MappedFields` built their lookup indexes the first time `Find` was called, by writing to `mutable` members without synchronization. Every reader of a scan shares one mapping, so readers opening migrated files on different threads raced on those writes. An empty mapping never filled its index, so it rewrote it on every lookup. The indexes are now built in the constructors, as Java's `NameMapping` does, and `MappedFields` can no longer be copied (its indexes point into its own fields). - **No field IDs and no name mapping:** this used to fail with `InvalidSchema`. The reader now builds a mapping from the read schema's names, as Java's Avro (`ProjectionDatumReader`) and ORC (`OrcIterable`) readers do with `MappingUtil.create(expectedSchema)`. Java's Parquet reader assigns IDs by column position instead; matching by name also covers nested fields and can't read a column into the wrong field when the column order differs. Files with no columns are covered by this too. The `ReaderOptions::name_mapping` doc comment now describes the fallback. - **List/map children without IDs:** a list element, map key or map value without an ID made the whole file fail with `InvalidSchema`, which can happen with a partial name mapping. As with Java's `PruneColumns`, the container is now read as if it were missing from the file: its initial default, null if optional, or an error if required. - **Aliases mapping two columns to one ID:** a file failed with `Duplicate field id` when it physically contained two columns whose names are aliases of the same mapped field. Like Java's reader, the last such column now keeps the ID. - **Cleanup:** the mutually recursive `ApplyNameMappingToField`/`ApplyNameMappingToChild` pair is replaced by a small `NameMappingApplier` class. It builds the dot-joined lookup path incrementally instead of joining a fresh `std::vector<std::string>` for every Parquet node, and uses `ListType::kElementName`, `MapType::kKeyName` and `MapType::kValueName` instead of string literals. `JoinByDot` no longer goes through a `std::stringstream`. - **Test:** `ReadNestedFieldsWithNameMapping` named its list element `item`, but Arrow's writer stores it as `element` by default, so the test never covered non-standard names. The test helper now writes with `disable_compliant_nested_types()`, and the test checks that the file keeps `item`. ### Not changed - **Names containing dots:** `NameMapping::Find` joins the path with `.`, so a top-level `a.b` and a nested `a` → `b` look up the same key. Java's `NameMapping.find` does the same (`DOT.join(names)`), so this is left as is. - **Arrow types restored from `ARROW:schema`** (`large_string`, `string_view`, dictionary): these don't depend on name mapping. Files with field IDs written by pyarrow-based writers hit the same projection error, so they're better handled separately. - **Footer metrics and row-group pruning for mapped files:** IDs from a mapping are still only attached to the reader's Arrow `SchemaManifest`. `ParquetMetrics::GetMetrics` is only called by the writer, so no current path computes metrics for these files. `ParquetMetricsRowGroupFilter` finds no IDs for them and keeps every row group, which is safe but means no pruning. ## Testing - Updated `ReadWithoutFieldIdsOrNameMapping`, which now reads by name, and added `ReadNestedFieldsWithoutFieldIdsOrNameMapping`, `ReadListWithUnmappedElementAsMissing` and `ReadWithAliasMappedToTwoColumns` to `parquet_test`. - Added `ApplyNameMappingKeepsLastAliasedSibling` and `ProjectContainersWithUnmappedChildrenAsMissing` to `parquet_schema_test`. - Added `NameMappingTest.ConcurrentFind` to `schema_test`; it is meant to run under ThreadSanitizer. - Local macOS Release build (Arrow vendored, REST off): all 19 `ctest` suites pass, and clang-tidy 22.1.8 (the CI version) reports nothing on the changed files. 🤖 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]
