Parquet: Implement defaults for generic data #11785
Conversation
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Small nits, but it looks good. Great to see this being added 👍
| public void testMissingRequiredWithoutDefault() { | ||
| Schema writeSchema = | ||
| new Schema( | ||
| required(1, "id", Types.LongType.get()), |
There was a problem hiding this comment.
For this test, I would drop fields {1,2} to ensure that it only focuses on being unable to read default fields without an initialDefault.
There was a problem hiding this comment.
I'll drop the data field, but I think it's good to have an ID field so that we get unique records and can compare the lists easily when debugging.
| } else if (id == MetadataColumns.IS_DELETED.fieldId()) { | ||
| reorderedFields.add(ParquetValueReaders.constant(false)); | ||
| types.add(null); | ||
| } else if (reader != null) { |
There was a problem hiding this comment.
I really miss Python's := operator here :)
| NestedField.optional("missing_inner_float") | ||
| .withId(5) | ||
| .ofType(Types.FloatType.get()) | ||
| .withInitialDefault(-0.0F) |
There was a problem hiding this comment.
Should we add an example with another complex default type (map, list)?
There was a problem hiding this comment.
In addition to this, I'm not sure if this is the right place to test it but it would be good to make sure there test coverage that covers. The following spec requirements:
- When a new field is added to a struct with a default value, updating the struct's default is optional
- If a field value is missing from a struct's initial-default, the field's initial-default must be used for the field
There was a problem hiding this comment.
Yes, good call out. I'll add those cases.
There was a problem hiding this comment.
@emkornfield, right now we have only added support for primitive field defaults so those cases aren't being addressed yet.
|
Merging this. Thanks for the review, @Fokko! |
3 participants
This adds default value support when reading Parquet files into Iceberg's generic data model.