rshdhere commented on code in PR #18252:
URL: https://github.com/apache/iceberg/pull/18252#discussion_r4364511414
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -639,11 +659,47 @@ private static int estimateSize(Type type) {
/** Interface for passing a function that assigns column IDs. */
public interface NextID {
int get();
+
+ default int get(int numReserved) {
+ int id = get();
+ for (int offset = 1; offset <= numReserved; offset += 1) {
+ int reserved = get();
+ Preconditions.checkState(
+ reserved == id + offset,
+ "Failed to assign consecutive IDs %s-%s, requested %s and got %s",
+ id + 1,
+ id + numReserved,
+ id + offset,
+ reserved);
+ }
+
+ return id;
+ }
}
/** Interface for passing a function that assigns column IDs from the
previous Id. */
public interface GetID {
int get(int oldId);
+
+ /**
+ * Assigns a new ID and reserves the IDs that immediately follow it.
+ *
+ * <p>Implementations must override this method to assign IDs for types
with derived field IDs.
+ *
+ * @param oldId an existing field ID
+ * @param numReserved number of IDs after the new ID that must not be
assigned
+ * @return a new field ID
+ */
+ default int get(int oldId, int numReserved) {
Review Comment:
`Schema.reassignIds` wraps the caller's `GetID` in a lambda that only
implements `get(int)`. So a file column always reaches this default, even when
the caller passes `reassignConflictingIds`, which this PR teaches to reserve
IDs:
```java
new Schema(
ImmutableList.of(optional(2, "f", Types.FileType.of(2))),
TypeUtil.reassignConflictingIds(ImmutableSet.of(2), ImmutableSet.of(1,
2, 3)));
// UnsupportedOperationException: Cannot reserve 6 IDs for field 2:
reserving IDs is not supported
```
Calling `TypeUtil.assignIds` directly with the same `GetID` works and gives
`4: f: optional file`. #17821 forwarded `get(oldId, numReserved)` in that
wrapper. The only caller today is `BaseSparkScanBuilder`, with partition
metadata fields, so nothing breaks yet.
##########
api/src/main/java/org/apache/iceberg/Schema.java:
##########
@@ -67,7 +67,8 @@ public class Schema implements Serializable {
Type.TypeID.VARIANT, 3,
Type.TypeID.UNKNOWN, 3,
Type.TypeID.GEOMETRY, 3,
- Type.TypeID.GEOGRAPHY, 3);
+ Type.TypeID.GEOGRAPHY, 3,
+ Type.TypeID.FILE, 4);
Review Comment:
`Schema` accepts a file whose base ID doesn't match the field that holds it:
```java
new Schema(
optional(2, "f", Types.FileType.of(9)),
required(3, "x", Types.LongType.get()));
```
This builds fine. `f`'s sub-fields come out as 10–15, and `x` takes ID 3
inside the block the spec reserves for `f` ("reserves the root field's ID plus
six consecutive IDs"). The same happens for a list element, e.g.
`ListType.ofOptional(2, FileType.of(20))`.
#17821 rejected this in `validateFileFields` with `Invalid file column %s:
nested field IDs are derived from %s, not %s`. Should that check come back?
Overlaps with the derived IDs are already caught by `IndexById`, so only the
mismatch check seems to be missing.
--
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]
