Introduce implementation for FileType - #3608
Conversation
| } else if (!LogicalTypeAnnotation.FileLogicalTypeAnnotation.OPTIONAL_FIELD_NAMES.contains(fieldName)) { | ||
| throw new IllegalArgumentException( | ||
| "FILE type group '" + name + "' contains unrecognized field '" + fieldName | ||
| + "'. Valid fields are: path, size, offset, etag"); |
There was a problem hiding this comment.
nit: stringify FileLogicalTypeAnnotation.OPTIONAL_FIELD_NAMES and use it in the error to avoid drift if we add new fields?
There was a problem hiding this comment.
We should add tests for validity of size/offset fields, both their values (>= 0) and valid/invalid combinations
There was a problem hiding this comment.
where can we add value checks during reads and writes? Can you provide any code references please?
|
I had a look at this against the revised self-reference semantics in apache/parquet-format#603 and ended up writing some of it, so rather than leave it as review comments I opened it as a PR against this branch: brkyvz#1 (delta is just two commits on top of The main piece is the writer-side API you'd need anyway: Three things worth flagging regardless of whether you take the PR:
One thing I deliberately did not change: I'd tightened schema validation to require Also worth noting for sequencing: this can't go green until a parquet-format release carries Happy to fold any of it in differently, or split it up, if that's easier for you. |
Rationale for this change
Introduces the reference implementation for the new LogicalTypeAnnotation File as discussed in this design document
What changes are included in this PR?
Introduces a new LogicalTypeAnnotation called FILE. It enforces that a group with this type annotation have the following fields by name:
Are these changes tested?
Yes, unit tested
Are there any user-facing changes?
Introduces a new LogicalTypeAnnotation called FILE