Repository navigation
feat(parquet): apply name mapping to files without field IDs - #1044
fairytale0828 wants to merge 1 commit into
Conversation
814bbf4 to
c794884
Compare
|
|
||
| if (!HasFieldIds(metadata->schema()->schema_root())) { | ||
| if (name_mapping == nullptr) { | ||
| return InvalidSchema( |
There was a problem hiding this comment.
Java does not error here - ReadConf.java:85-88 falls back to addFallbackIds and assigns positional ids, so this file still reads there. Not a regression, but is closing that gap out of scope?
There was a problem hiding this comment.
Good catch. I'd like to keep this PR scoped to name mapping. The no-mapping case currently matches the C++ Avro reader, which also returns InvalidSchema. I've opened #1046 to add the positional fallback to match Java. Does that work for you?
There was a problem hiding this comment.
I don't think we should add positional fallback. IIRC, that fallback was added for legacy use case.
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
The scope of this PR looks good. Please resolve conflict. Feel free to create followup PRs for known limitations in the description.
c794884 to
e10f48e
Compare
Thanks! Rebased and force-pushed to resolve the conflicts.The known limitations will be followed up in separate PRs. |
The CI failures are caused by mozilla-actions/sccache-action being blocked by the org policy, which also affects other PRs. Could you re-run the failed jobs? Thanks! |
|
Could you please rebase again to incorporate the fix on ci failure? |
This branch has not been deployed
3 participants
Summary
Parquet files without field IDs (for example, files added by table migration) currently fail with
NotImplementedinBuildProjection. This PR resolves the TODO by applying the name mapping fromReaderOptions::name_mapping, which the Avro reader already supports.ApplyNameMappingwalks theSchemaManifestand addsPARQUET:field_idmetadata to each field found in the mapping. The existing projection then runs unchanged.element, and map entries usekeyandvalue, regardless of the physical names in the file. Bothlistandlarge_listare handled.InvalidSchema, consistent with Avro.Known limitations, not changed in this PR:
NameMappingbuilds its lookup indexes lazily on first use, so concurrent readers sharing one mapping can race on that first lookup. This already affects Avro and can be fixed separately..cannot be told apart from nested paths, becauseNameMapping::Findjoins path segments with..Test Plan
ParquetNameMappingTestinparquet_schema_test.cc: assigning IDs by column name, matching an alias, nested struct/list/map fields with non-standard physical names (item,key_value), and unmapped fields or fields mapped without an ID.ParquetReaderTestinparquet_test.cc:ReadWithoutFieldIdsOrNameMapping: returnsInvalidSchema.ReadWithNameMapping: alias, column reordering, type promotion, and a missing optional column.ReadWithFieldIdsAndNameMapping: file field IDs win over a conflicting mapping.ReadNestedFieldsWithNameMapping: end-to-end read of struct,list<struct>, andmap<string, struct>, including null and empty values, with bothlistandlarge_list.pre-commit runpasses on the changed files.ctest -R parquet_test: 140 passed, 1 skipped.