feat: support reading OPTIONAL column in parquet_derive #5717
Conversation
double-free
changed the title
double-free
force-pushed
the
yy/nullable-field
branch
from
345991c
to
7caee7c
Compare
There was a problem hiding this comment.
Looks good to me -- thank you @double-free and sorry for the delay in review
I had one small request to update the comments. Let me know what you think
| @@ -248,7 +248,13 @@ impl Field { | |||
| let write_batch_expr = quote! { | |||
| let mut vals = Vec::new(); | |||
| if let #column_reader(mut typed) = column_reader { | |||
| typed.read_records(num_records, None, None, &mut vals)?; | |||
| let mut definition_levels = Vec::new(); | |||
There was a problem hiding this comment.
Can we please also update the documentation above
/// `Option` types and references not supported
To reflect this change?
There was a problem hiding this comment.
Sure, I can make it explicit, but to align our understanding, I want to note that this change does NOT mean it supports Option<T> in the struct to be decoded.
It is to support fields that are marked as "optional" in schema, but in fact all values are non-null. This is very common when parsing parquet files generated by pyarrow, since by default all columns are optional. I reproduced it in unit test, by adding Option<T> in the struct to be encoded.
So this comment is still true, but I can make it explicit for the case I mentioned above:
/// `Option` types and references not supported, but the column itself can be nullable (i.e., def_level==1), as long as the values are all valid.
How do you think? To update the comment or keep it as it is?
Thanks for your response! Please check my reply and see if we do need to update the code comment. |
|
Thanks again @double-free |
Which issue does this PR close?
Closes #5716.
Rationale for this change
See details in the issue.
What changes are included in this PR?
Add definition level in macro code.
Are there any user-facing changes?
No API change, extend parquet_derive to handle OPTIONAL columns.