dramaticlly commented on code in PR #18252:
URL: https://github.com/apache/iceberg/pull/18252#discussion_r4340574222
##########
api/src/main/java/org/apache/iceberg/types/AssignIds.java:
##########
@@ -31,8 +31,8 @@ class AssignIds extends
TypeUtil.CustomOrderSchemaVisitor<Type> {
this.getID = getID;
}
- private int idFor(int id) {
- return getID.get(id);
+ private int idFor(int id, Type type) {
+ return type.isFileType() ? getID.get(id, Types.FileType.NUM_NESTED_FIELDS)
: getID.get(id);
Review Comment:
looks like there's constant need to figure out the reserved id space of
given type, 6 for file and 0 otherwise, maybe we can move it to TypeUtils so
this can be simply
```suggestion
return getID.get(id, TypeUtil.numReservedIds(type));
```
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -460,6 +461,25 @@ public static Type assignIds(Type type, GetID getId) {
return TypeUtil.visit(type, new AssignIds(getId));
}
+ /**
+ * Returns the type to use for a field that was assigned {@code newId}.
+ *
+ * <p>A file's nested field IDs are derived from the ID of the field that
holds it, so a file is
+ * rebuilt from the new ID rather than reassigned field by field.
+ *
+ * @param original the field's type before IDs were assigned
+ * @param newId the ID assigned to the field
+ * @param assignedType the original type with IDs assigned
+ * @return the type the field should have
+ */
+ static Type applyAssignedId(Type original, int newId, Type assignedType) {
Review Comment:
can we rely only on visitor output assignedType?
##########
api/src/main/java/org/apache/iceberg/types/AssignFreshIds.java:
##########
@@ -48,15 +48,31 @@ class AssignFreshIds extends
TypeUtil.CustomOrderSchemaVisitor<Type> {
this.nextId = nextId;
}
- private int idFor(String fullName) {
+ private int idFor(int currentId, Type type) {
+ Types.NestedField existingField = baseField(name(currentId));
+ if (canReuseBaseId(existingField, type)) {
+ return existingField.fieldId();
+ }
+
+ return type.isFileType() ? nextId.get(Types.FileType.NUM_NESTED_FIELDS) :
nextId.get();
Review Comment:
looks like the logic is find the filed by id and look up in the schema, then
we can make type based decision on whether to reuse the base id. we can
potentially inline the logic of canReuseBaseId and baseField? Something like
```java
private int idFor(int currentId, Type type) {
String fullName = name(currentId);
if (baseSchema != null && fullName != null) {
Types.NestedField baseField = baseSchema.findField(fullName);
// a file derives its nested field IDs from its own ID, so reusing a
base ID is only safe when
// the base field was already a file and had reserved that block
if (baseField != null && (!type.isFileType() ||
baseField.type().isFileType())) {
return baseField.fieldId();
}
}
return nextId.get(TypeUtil.numReservedIds(type));
}
```
--
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]
