fix(parquet): add reader option for unknown physical/logical type combos - #11017
fix(parquet): add reader option for unknown physical/logical type combos#11017M-Tesla wants to merge 2 commits into
Conversation
parquet-format apacheGH-607 requires readers to treat unrecognized physical and logical type combinations as an unknown logical type. Keep the historical error as the default, and add a reader option that exposes the physical type and ignores column statistics.
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Thanks @M-Tesla, but I think this might want some more discussion on the issue before diving into an implementation. I myself am not entirely certain what should be done in this case. I'm not particularly keen on returning None for the column statistics (and I think this change has no impact on stats in the column index). Perhaps changing the logical type to _Unknown and ensuring the sort order comes back as UNDEFINED would suffice. Consumers who care could then ignore the stats, or use them anyway if they have out-of-band info about the real type.
| } | ||
|
|
||
| fn parquet_file_with_int32_uuid_logical_type() -> Vec<u8> { | ||
| let field = Type::primitive_type_builder("id", PhysicalType::INT32) |
There was a problem hiding this comment.
We could instead use the file added in apache/parquet-testing#122
There was a problem hiding this comment.
Thank you, that is a better fixture. I will switch the test to int32_with_uuid_logical_type.parquet from that parquet-testing PR.
|
Thank you for the review, @etseidl. I really appreciate you taking the time, especially while this behavior is still being settled. I'll follow the approach you outlined:
I'll keep the reader option with the current default (error), matching what @alamb suggested on the issue, and I will post a short note there as well so the discussion lives with the spec question. Thank you again for the guidance. I'll update the PR along these lines. |
Keep column statistics, switch tests to the parquet-testing INT32+UUID file, and compute sort order from _Unknown instead of a SIGNED placeholder.
2 participants
Which issue does this PR close?
Rationale for this change
parquet-format GH-607 says readers must handle unknown physical and logical type combinations (for example INT32 annotated as UUID) by treating them as an unknown logical type.
This crate currently errors in
Typeconstruction (Cannot annotate Uuid from INT32). @alamb asked for a reader option rather than changing the default.What changes are included in this PR?
with_coerce_incompatible_logical_types(true)onArrowReaderOptions,ReadOptionsBuilder, andParquetMetaDataOptions._Unknownwith sort orderUNDEFINED. Column statistics are retained so callers who care can ignore them or use them with out-of-band type info.Type::build()still rejects the combination. Round-trip of the invalid annotation is out of scope, as noted in the issue.Are these changes tested?
_Unknown/UNDEFINEDwith the option. INT64+UUID and BYTE_ARRAY+UUID also getUNDEFINEDrather than UNSIGNED or SIGNED. UUID FLBA(15) and List-on-primitive still error with the option.int32_with_uuid_logical_type.parquetfrom parquet-testing. Default reader errors. With the option, the column reads as ArrowInt32values 0 through 9, sort order isUNDEFINED, and column statistics are present.cargo test -p parquet --lib --tests --bins.cargo clippy -p parquet --all-targets -- -D warnings.Are there any user-facing changes?
Additive reader option. Default remains an error. No breaking API change.
AI Disclosure
Assisted with the option plumbing, GH-607 coerce path, and regression tests. Default stays an error per the issue discussion. Reviewed and verified with the checks above.