[PARQUET] Allow UNKNOWN logical type annotation on any physical type#9855
Conversation
|
Keeping as draft while awaiting a test file (see issue discussion). |
|
Received confirmation that this fixes the downstream issue so marking as ready. @cdelmonte-zg one thing this fix doesn't do is preserve the physical type upon write...it will revert to |
Thanks @etseidl, no follow-up needed for the delta-kernel-rs side. The kernel strips void columns from the physical write schema before handing data to the parquet writer (it's a deliberate design choice from the Delta protocol, void columns are never materialized in data files). So kernel never writes a Null/UNKNOWN column; it only needs to read what other writers (Spark, in the DAT cases) produced. Read-side correctness is all that #9855 needs to provide. |
| // specification. Since this can only annotate an entirely null column, the data type | ||
| // used for the NullArrayReader should be irrelevant. It's just needed to read the | ||
| // repetition and definition level data. | ||
| if matches!(arrow_type, Some(DataType::Null)) { |
There was a problem hiding this comment.
pulling this up out of INT32 type makes sense to me
| let batch = reader.next().unwrap().unwrap(); | ||
|
|
||
| for col in batch.columns() { | ||
| assert_eq!(col.len(), 4); |
There was a problem hiding this comment.
I think we should probably also assert the DataType of each column read in. In this case, I think they should all be DataType::Null
| unit | ||
| )), | ||
| }, | ||
| // https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#unknown-always-null |
There was a problem hiding this comment.
The reson this is removed is that it is now handled higher up in from_parquet 👍
|
Thanks for the review @alamb. |
apache#9855) # Which issue does this PR close? - Closes apache#9844. # Rationale for this change Parquet writers for the most part only annotate `INT32` columns with the `UNKNOWN` logical type annotation. This annotation is used to denote a column which contains only null values for which the actual physical type cannot be deduced. This crate assumes _only_ INT32 columns can be so annotated, but there is no such requirement in the Parquet [specification](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#unknown-always-null). # What changes are included in this PR? Modifies Parquet schema parsing to allow `UNKNOWN` on any type, and ensures that when encountered, an arrow array reader for the `Null` datatype is used. # Are these changes tested? Yes, unit test is added. # Are there any user-facing changes? No API changes, just a behavior change.
3 participants
Which issue does this PR close?
Rationale for this change
Parquet writers for the most part only annotate
INT32columns with theUNKNOWNlogical type annotation. This annotation is used to denote a column which contains only null values for which the actual physical type cannot be deduced. This crate assumes only INT32 columns can be so annotated, but there is no such requirement in the Parquet specification.What changes are included in this PR?
Modifies Parquet schema parsing to allow
UNKNOWNon any type, and ensures that when encountered, an arrow array reader for theNulldatatype is used.Are these changes tested?
Yes, unit test is added.
Are there any user-facing changes?
No API changes, just a behavior change.