RussellSpitzer commented on code in PR #18252:
URL: https://github.com/apache/iceberg/pull/18252#discussion_r4458562238
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -1155,6 +1155,136 @@ private Map<Integer, NestedField> lazyFieldsById() {
}
}
+ public static final class FileType extends NestedType {
+ public static final String NAME = "file";
+ public static final int NUM_NESTED_FIELDS = 6;
+
+ private static final String URI = "uri";
+ private static final String OFFSET = "offset";
+ private static final String SIZE = "size";
+ private static final String CONTENT_TYPE = "content_type";
+ private static final String CHECKSUM = "checksum";
+ private static final String INLINE = "inline";
+
+ public static FileType of(int baseId) {
Review Comment:
This is part of why FileType is "kinda a nested type" and "kinda a
primitive" is complicated. All of my decisions here are leaning towards "nested
type." The ID here is shorthand for filling in the nested fields, the same way
`ListType.ofOptional(elementId, ...)` and `MapType.ofOptional(keyId, valueId,
...)` take IDs for nested fields the caller doesn't really own.
That also covers the `addColumn` case: `UpdateSchema` already reassigns a
nested type's field IDs when it's added, so `addColumn("photo",
FileType.of(0))` works like adding a list with a placeholder element ID. You're
right that it costs some file-specific handling in ID assignment
(`applyAssignedId`).
An ID-free `FileType.get()` moves the type from a "nested" form toward a
"primitive" one, and IMHO that brings three odd behaviors:
1. Equality. I think there are 3 ways we could handle this but each is ...
odd:
- Unbound equals every bound file. Bound types = if they match in field
id. This breaks transitive equality.
- All file types are equal, like primitives: schema comparison still
works because `NestedField` compares IDs, but anything keyed by `Type` breaks.
For example, `TypeToSchema.WithTypeToName` caches Avro records by type, so two
file columns would share one cached record and its nested field IDs.
- Compare `baseId`, with unbound as its own value: this is safe, but
`field.type().equals(FileType.get())` is always false for a real column, which
is odd for something that looks like a singleton. We could probably put in a
static rule to forbid writing this code with a warning to use "isFileType()"? A
little of a foot-gun, but this is what I would pick if we want to go this way.
2. `fields()`. An unbound file type can't return nested fields with IDs, so
`fields()` would have to throw until the type is bound to a field. Structs,
lists, and maps always have their nested fields from construction. We'd have
"bound" and "unbound" file types that behave differently, which makes the type
stateful in a way I'd like to avoid.
3. Constructors change the type. `NestedField.of(5, "photo",
FileType.get()).type()` would return a different type than the one passed in.
No other type is rewritten when a field is built, so that's a trap for future
code.
If we're OK with those three behaviors, the workable version is the last
equality option (compare `baseId`, unbound is distinct), and I think we can
switch to it. I'd need to check the rest of the stack, but I expect that
version is safe.
For what it's worth, Claude tells me "the risks are concentrated, and
simplifications are wide" for what that's worth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
