API, CORE, PARQUET: Implicit struct file type - #17808
Conversation
Persist the type as "file" and expand it to a closed nested struct whose field IDs are derived from the enclosing field. Generated-by: Cursor Grok 4.6
ReassignDoc rebuilt every struct it visited, so reassigning docs turned a file column into a plain struct that no longer serializes as "file" or honors the format version gate. Return the file type unchanged there and in ReassignIds, matching the other assigners. The new two-argument GetID overload ignored the reservation request, so an implementation that did not override it could hand out IDs inside a file's derived block and produce duplicate field IDs with no error. Fail when the reservation cannot be honored. ReassignConflictingIds moved a field only when its own ID conflicted, so a file column kept an ID whose derived block overlapped IDs already in use. Move the column when any of its reserved IDs is unavailable. Also consolidate the helper that rebuilds a file type from a newly assigned ID into TypeUtil.assignedType, and drop the test prefix from the schema evolution tests added for this type. Generated-by: Cursor Claude Opus 5
Derived ID validation in the schema parser was only covered for struct fields and list elements. Add the map key and map value cases, along with a round trip for a file used as a map key. Add Parquet conversions for a required file column and for a file used as a list element and as a map value, plus a data round trip for a file inside a list. Record that reassigning a file column tracks only the enclosing ID, because the nested IDs are derived from it, and split the combined rename and delete test into one test per operation. Generated-by: Cursor Claude Opus 5
The file type had its own constant because the map was keyed by type ID and the file type reports STRUCT, so a STRUCT key would have gated every struct. Key the map by class instead, which identifies a logical type even when two of them share a type ID, and drop the separate constant so all minimum versions are declared in one place. Make the file type final so the class key is exact. Generated-by: Cursor Claude Opus 5
Passing zero reserved IDs used the argument as a sentinel for "do not reserve", which hid the fact that the overload exists only for types whose nested field IDs are derived. Branch on the type so the plain overload is used for everything else. Generated-by: Cursor Claude Opus 5
Tests for name resolution, Java serialization, format version gating, readability, projection, doc reassignment, accessors, and expression binding now live beside the code they exercise, so a change to those utilities surfaces the file type expectations. TestFileType keeps the type contract and the reserved ID block, which no existing class owns. Add coverage for selecting and filtering a file subfield. Drop tests that only re-exercised generic behavior: rejecting defaults applies to every nested type, and the list round trip is already covered by the list schema conversion plus the file round trip. Generated-by: Cursor
FileType.fieldId() returned the ID of the field that holds the type, not an ID of the type itself, which read as though it mirrored NestedField.fieldId(). Rename it to enclosingId() to match the name the parser already used for the same value. Report the short type name when a file and a struct are not interchangeable instead of formatting a whole struct into the error. Generated-by: Cursor
RussellSpitzer
changed the title
| return existingId; | ||
| } | ||
|
|
||
| return type.isFileType() ? nextId.get(Types.FileType.NUM_NESTED_FIELDS) : nextId.get(); |
There was a problem hiding this comment.
When we assign ID's we have to burn ID's equal to the number of fields within the File type.
| for (int i = 0; i < length; i += 1) { | ||
| newIds.add(idFor(name(fields.get(i).fieldId()))); | ||
| Types.NestedField field = fields.get(i); | ||
| newIds.add(idFor(name(field.fieldId()), field.type())); |
There was a problem hiding this comment.
We have to start plumbing "Type" through again to handle "file" types
|
|
||
| @VisibleForTesting | ||
| static final Map<Type.TypeID, Integer> MIN_FORMAT_VERSIONS = | ||
| static final Map<Class<? extends Type>, Integer> MIN_FORMAT_VERSIONS = |
There was a problem hiding this comment.
I really didn't want to have to do this, but since File type shares an ID with Struct Type we can't match on that. Matching on class is a little less clean but the other option is to add a new type for file ID and then change our code everywhere that checks whether something is a Struct by typeId
|
Hi @RussellSpitzer, hope it's okay to jump in here! I went through the PR and wanted to share some thoughts on the design. While making This leads to a classic Liskov Substitution Principle (LSP) violation—it compiles, but quietly breaks substitutability. We can already see the interest on this technical debt building up in the PR through:
These aren't features of An alternative approach: This keeps the exact representation you want while eliminating the Wdyt? |
2 participants
Adds in an File Type to Iceberg using a "Struct on Read" approach. The File type is defined as a single leaf node in the Schema which implicitly defines it's sub-fields. On Read, the library treats this nested node as a Struct with all the expected behaviors of a struct. The only exceptions are that the fields within this file-struct cannot be modified or rearranged in any way.
The key design here is to modify many of the visitors we have that deal with ID's to burn a certain number of id's when making new File fields.