You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by ze...@apache.org on 2026/07/27 20:30:07 UTC
(iceberg-go) branch main updated: fix(table): reject reserved metadata field IDs before reassignment (#1548)
This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 36bd848da fix(table): reject reserved metadata field IDs before reassignment (#1548)
36bd848da is described below
commit 36bd848da31c0e401f14d38caf70a90f3205fb8b
Author: Tanmay Rauth <t_...@apple.com>
AuthorDate: Mon Jul 27 13:30:01 2026 -0700
fix(table): reject reserved metadata field IDs before reassignment (#1548)
NewMetadata reassigns schema IDs before the compatibility check runs, so
a user-supplied reserved ID (e.g. _row_id) was silently remapped to a
fresh value and later resolved to the wrong physical column against
Java-written files. Validate the user schema against the full reserved
range before reassignIDs. The AddSchema check stays narrow so internal
position-delete schemas (file_path/pos) still pass.
Fixes: #1107
---
metadata_columns.go | 18 +++-
table/metadata.go | 10 ++
table/metadata_builder_internal_test.go | 156 +++++++++++++++++++++++++++++---
table/metadata_schema_compatibility.go | 48 ++++++++++
table/scanner_internal_test.go | 55 +++++++----
5 files changed, 254 insertions(+), 33 deletions(-)
diff --git a/metadata_columns.go b/metadata_columns.go
index 6ab1daceb..dc592a835 100644
--- a/metadata_columns.go
+++ b/metadata_columns.go
@@ -17,7 +17,23 @@
package iceberg
-import "slices"
+import (
+ "math"
+ "slices"
+)
+
+// MaxStructFieldID is the largest field ID a user-supplied schema may assign.
+// Field IDs greater than this are reserved by the spec for metadata columns
+// (_row_id, _last_updated_sequence_number, _file, _pos, _deleted, _spec_id,
+// _partition, ...). Java: TypeUtil.MAX_STRUCT_FIELD_ID (Integer.MAX_VALUE - 200).
+const MaxStructFieldID = math.MaxInt32 - 200
+
+// IsReservedFieldID reports whether fieldID falls in the range the spec reserves
+// for metadata columns. User schemas must not assign IDs in this range; table
+// schema field IDs must not exceed MaxStructFieldID.
+func IsReservedFieldID(fieldID int) bool {
+ return fieldID > MaxStructFieldID
+}
// Row lineage metadata column field IDs (v3+). Reserved IDs are Integer.MAX_VALUE - 107
// and Integer.MAX_VALUE - 108 per the Iceberg spec (Metadata Columns / Row Lineage).
diff --git a/table/metadata.go b/table/metadata.go
index ce4ec84e4..fd9e8896a 100644
--- a/table/metadata.go
+++ b/table/metadata.go
@@ -2704,6 +2704,16 @@ func NewMetadataWithUUID(sc *iceberg.Schema, partitions *iceberg.PartitionSpec,
}
}
+ // Reject reserved metadata column IDs on the user-supplied schema before
+ // reassignIDs runs: reassignment overwrites every field ID with a fresh,
+ // non-reserved value, so a reserved ID (e.g. _row_id) would otherwise be
+ // silently remapped and later resolve to the wrong physical column against
+ // Java-written files (see #1107). This is stricter than Java, which relies
+ // on assignFreshIds to remap them; rejecting is the safer contract here.
+ if err := validateNoReservedFieldIDs(sc); err != nil {
+ return nil, err
+ }
+
reassignedIds, err := reassignIDs(sc, partitions, sortOrder)
if err != nil {
return nil, err
diff --git a/table/metadata_builder_internal_test.go b/table/metadata_builder_internal_test.go
index 4bec99478..3b6baee5b 100644
--- a/table/metadata_builder_internal_test.go
+++ b/table/metadata_builder_internal_test.go
@@ -23,6 +23,7 @@ import (
"math"
"reflect"
"slices"
+ "strconv"
"strings"
"testing"
"time"
@@ -2072,29 +2073,154 @@ func TestUnknownTypeValidation(t *testing.T) {
require.ErrorContains(t, err, "must be optional")
})
- t.Run("ReservedFieldIDRowID", func(t *testing.T) {
+ t.Run("InvalidUnknownMapValue", func(t *testing.T) {
+ invalidSchema := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 2, Name: "invalid_map", Type: &iceberg.MapType{KeyID: 3, KeyType: iceberg.StringType{}, ValueID: 4, ValueType: iceberg.UnknownType{}, ValueRequired: true}, Required: false},
+ )
+ err := checkSchemaCompatibility(invalidSchema, 3)
+ require.Error(t, err, "should error when unknown type is used as map value")
+ require.ErrorContains(t, err, "must be optional")
+ })
+}
+
+// reservedNonLineageFieldID is a spec-reserved metadata column ID (_file) that
+// is not one of the row-lineage columns, exercising the full reserved range
+// rather than just iceberg.IsMetadataColumn.
+const reservedNonLineageFieldID = 2147483646
+
+func TestValidateNoReservedFieldIDs(t *testing.T) {
+ tests := []struct {
+ name string
+ schema *iceberg.Schema
+ wantColumn string // schema path the error must name
+ wantID int
+ }{
+ {
+ name: "top-level row-lineage id with non-canonical name",
+ schema: iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true},
+ iceberg.NestedField{ID: iceberg.RowIDFieldID, Name: "user_id", Type: iceberg.PrimitiveTypes.Int64},
+ ),
+ wantColumn: "user_id",
+ wantID: iceberg.RowIDFieldID,
+ },
+ {
+ name: "top-level non-lineage reserved id",
+ schema: iceberg.NewSchema(1,
+ iceberg.NestedField{ID: reservedNonLineageFieldID, Name: "bad", Type: iceberg.PrimitiveTypes.Int64},
+ ),
+ wantColumn: "bad",
+ wantID: reservedNonLineageFieldID,
+ },
+ {
+ name: "reserved id inside struct child",
+ schema: iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "outer", Required: true, Type: &iceberg.StructType{
+ FieldList: []iceberg.NestedField{
+ {ID: reservedNonLineageFieldID, Name: "inner", Type: iceberg.PrimitiveTypes.Int64},
+ },
+ }},
+ ),
+ wantColumn: "outer.inner",
+ wantID: reservedNonLineageFieldID,
+ },
+ {
+ name: "reserved id on list element",
+ schema: iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "arr", Required: true, Type: &iceberg.ListType{
+ ElementID: reservedNonLineageFieldID, Element: iceberg.PrimitiveTypes.Int64, ElementRequired: false,
+ }},
+ ),
+ wantColumn: "arr.element",
+ wantID: reservedNonLineageFieldID,
+ },
+ {
+ name: "reserved id on map key",
+ schema: iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "m", Required: true, Type: &iceberg.MapType{
+ KeyID: reservedNonLineageFieldID, KeyType: iceberg.PrimitiveTypes.String,
+ ValueID: 2, ValueType: iceberg.PrimitiveTypes.Int64, ValueRequired: false,
+ }},
+ ),
+ wantColumn: "m.key",
+ wantID: reservedNonLineageFieldID,
+ },
+ {
+ name: "reserved id on map value",
+ schema: iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "m", Required: true, Type: &iceberg.MapType{
+ KeyID: 2, KeyType: iceberg.PrimitiveTypes.String,
+ ValueID: reservedNonLineageFieldID, ValueType: iceberg.PrimitiveTypes.Int64, ValueRequired: false,
+ }},
+ ),
+ wantColumn: "m.value",
+ wantID: reservedNonLineageFieldID,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ err := validateNoReservedFieldIDs(tt.schema)
+ require.Error(t, err)
+ assert.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ assert.Contains(t, err.Error(), "reserved metadata column ID")
+ assert.Contains(t, err.Error(), tt.wantColumn, "error must report the schema path")
+ assert.Contains(t, err.Error(), strconv.Itoa(tt.wantID))
+ })
+ }
+
+ t.Run("boundary MaxStructFieldID is allowed", func(t *testing.T) {
schema := iceberg.NewSchema(1,
- iceberg.NestedField{ID: iceberg.RowIDFieldID, Name: "bad_field", Type: iceberg.PrimitiveTypes.Int64},
+ iceberg.NestedField{ID: iceberg.MaxStructFieldID, Name: "ok", Type: iceberg.PrimitiveTypes.Int64},
)
- err := checkSchemaCompatibility(schema, 3)
- require.Error(t, err)
- require.ErrorContains(t, err, "reserved metadata column ID")
+ require.NoError(t, validateNoReservedFieldIDs(schema))
})
- t.Run("ReservedFieldIDLastUpdatedSeqNum", func(t *testing.T) {
+}
+
+// TestNewMetadataRejectsReservedFieldIDsBeforeReassignment guards the #1107
+// regression: reassignIDs would silently remap a user-supplied reserved ID to a
+// fresh value, so NewMetadata must reject it up front instead.
+func TestNewMetadataRejectsReservedFieldIDsBeforeReassignment(t *testing.T) {
+ for _, tc := range []struct {
+ name string
+ field iceberg.NestedField
+ }{
+ {"row_id", iceberg.NestedField{ID: iceberg.RowIDFieldID, Name: "user_id", Type: iceberg.PrimitiveTypes.Int64}},
+ {"last_updated_seq", iceberg.NestedField{ID: iceberg.LastUpdatedSequenceNumberFieldID, Name: "seq", Type: iceberg.PrimitiveTypes.Int64}},
+ {"non_lineage_reserved", iceberg.NestedField{ID: reservedNonLineageFieldID, Name: "f", Type: iceberg.PrimitiveTypes.Int64}},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ schema := iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true},
+ tc.field,
+ )
+
+ _, err := NewMetadata(schema, iceberg.UnpartitionedSpec, UnsortedSortOrder,
+ "s3://bucket/table", iceberg.Properties{"format-version": "3"})
+ require.Error(t, err)
+ assert.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ assert.Contains(t, err.Error(), "reserved metadata column ID")
+ })
+ }
+}
+
+// TestCheckSchemaCompatibilityReservedIDs locks in the deliberate asymmetry of
+// the AddSchema gate: it rejects row-lineage metadata columns, but must permit
+// the position-delete reserved IDs (file_path/pos), which internal writers add
+// via AddSchema. Broadening it to the full reserved range breaks those writers.
+func TestCheckSchemaCompatibilityReservedIDs(t *testing.T) {
+ t.Run("rejects row-lineage column", func(t *testing.T) {
schema := iceberg.NewSchema(1,
- iceberg.NestedField{ID: iceberg.LastUpdatedSequenceNumberFieldID, Name: "bad_field", Type: iceberg.PrimitiveTypes.Int64},
+ iceberg.NestedField{ID: iceberg.RowIDFieldID, Name: "bad", Type: iceberg.PrimitiveTypes.Int64},
)
err := checkSchemaCompatibility(schema, 3)
require.Error(t, err)
- require.ErrorContains(t, err, "reserved metadata column ID")
+ assert.ErrorContains(t, err, "reserved metadata column ID")
})
- t.Run("InvalidUnknownMapValue", func(t *testing.T) {
- invalidSchema := iceberg.NewSchema(1,
- iceberg.NestedField{ID: 2, Name: "invalid_map", Type: &iceberg.MapType{KeyID: 3, KeyType: iceberg.StringType{}, ValueID: 4, ValueType: iceberg.UnknownType{}, ValueRequired: true}, Required: false},
- )
- err := checkSchemaCompatibility(invalidSchema, 3)
- require.Error(t, err, "should error when unknown type is used as map value")
- require.ErrorContains(t, err, "must be optional")
+
+ t.Run("permits position-delete reserved IDs", func(t *testing.T) {
+ schema := iceberg.NewSchema(0, iceberg.PositionalDeleteSchema.Fields()...)
+ require.NoError(t, checkSchemaCompatibility(schema, 2))
})
}
diff --git a/table/metadata_schema_compatibility.go b/table/metadata_schema_compatibility.go
index af13ede5b..e10ed0aa8 100644
--- a/table/metadata_schema_compatibility.go
+++ b/table/metadata_schema_compatibility.go
@@ -133,6 +133,13 @@ func checkSchemaCompatibility(sc *iceberg.Schema, formatVersion int) error {
panic("invalid schema: field with id " + strconv.Itoa(field.ID) + " not found, this is a bug, please report.")
}
+ // Reject row-lineage metadata columns (_row_id,
+ // _last_updated_sequence_number) if a caller adds them to a stored
+ // schema during evolution. Other spec-reserved IDs (e.g. position-delete
+ // file_path/pos) are intentionally permitted: internal writers build
+ // throwaway metadata from those schemas via AddSchema. User schemas are
+ // validated against the full reserved range separately, before
+ // reassignIDs, in NewMetadataWithUUID.
if iceberg.IsMetadataColumn(field.ID) {
return fmt.Errorf("%w: field '%s' uses reserved metadata column ID %d",
iceberg.ErrInvalidSchema, colName, field.ID)
@@ -174,6 +181,47 @@ func checkSchemaCompatibility(sc *iceberg.Schema, formatVersion int) error {
return nil
}
+// validateNoReservedFieldIDs rejects user-supplied schemas that assign field IDs
+// in the range the spec reserves for metadata columns (_row_id,
+// _last_updated_sequence_number, _file, _pos, _deleted, ...). Field IDs must not
+// exceed iceberg.MaxStructFieldID. The walk is recursive: FlatFields yields every
+// leaf and nested field, so a reserved ID buried in a struct, list element, or
+// map key/value is caught too.
+//
+// This guards the table-creation path only. NewMetadataWithUUID calls it on the
+// user schema before reassignIDs, because reassignment overwrites every ID with
+// a fresh, non-reserved value and would otherwise mask a reserved ID the caller
+// supplied (see #1107). It intentionally covers the full reserved range, unlike
+// checkSchemaCompatibility, which permits internal writers to add position-delete
+// schemas (file_path/pos) via AddSchema.
+func validateNoReservedFieldIDs(sc *iceberg.Schema) error {
+ fieldsIt, err := sc.FlatFields()
+ if err != nil {
+ return fmt.Errorf("failed to enumerate schema fields: %w", err)
+ }
+
+ // Sort by ID so the reported field is deterministic when several are reserved.
+ for _, field := range slices.SortedFunc(fieldsIt, func(a, b iceberg.NestedField) int {
+ return cmp.Compare(a.ID, b.ID)
+ }) {
+ if !iceberg.IsReservedFieldID(field.ID) {
+ continue
+ }
+
+ // Report the schema's own name/path for the field, not the canonical
+ // metadata column name, so the caller can locate the offending field.
+ name, ok := sc.FindColumnName(field.ID)
+ if !ok {
+ name = field.Name
+ }
+
+ return fmt.Errorf("%w: field '%s' uses reserved metadata column ID %d",
+ iceberg.ErrInvalidSchema, name, field.ID)
+ }
+
+ return nil
+}
+
// minFormatVersionForType returns the minimum table format version required
// for the given type. Returns 1 for types supported in all versions, or a higher
// version number for types that require newer format versions.
diff --git a/table/scanner_internal_test.go b/table/scanner_internal_test.go
index 0ca010b2e..3f2ed9eee 100644
--- a/table/scanner_internal_test.go
+++ b/table/scanner_internal_test.go
@@ -1085,24 +1085,44 @@ func TestProjectionWithRowLineageRequiresV3(t *testing.T) {
assert.ErrorContains(t, err, "row lineage")
}
-// TestProjectionV3SchemaAlreadyHasRowID covers the case where the user schema
-// already declares _row_id (a reserved field id, but legal in v3). The
-// projection helper must be idempotent and not panic on the duplicate ID.
+// TestProjectionV3SchemaAlreadyHasRowID covers a v3 table whose stored schema
+// already declares _row_id at its reserved field ID. NewMetadata now rejects
+// such user schemas, but tables loaded from a catalog (e.g. written by Java) can
+// legitimately carry the reserved ID, so the metadata is parsed directly here to
+// bypass the creation-time validator. The end-to-end projection must be
+// idempotent and not duplicate the reserved column.
func TestProjectionV3SchemaAlreadyHasRowID(t *testing.T) {
- schema := iceberg.NewSchema(
- 1,
- iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true},
- iceberg.NestedField{ID: 2, Name: "payload", Type: iceberg.PrimitiveTypes.String, Required: false},
- iceberg.RowID(),
- )
-
- metadata, err := NewMetadata(
- schema,
- iceberg.UnpartitionedSpec,
- UnsortedSortOrder,
- "s3://test-bucket/test_table",
- iceberg.Properties{"format-version": "3"},
- )
+ metadataJSON := `{
+ "format-version": 3,
+ "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1",
+ "location": "s3://test-bucket/test_table",
+ "last-sequence-number": 0,
+ "last-updated-ms": 1602638573590,
+ "last-column-id": 2,
+ "next-row-id": 0,
+ "current-schema-id": 0,
+ "schemas": [
+ {
+ "type": "struct",
+ "schema-id": 0,
+ "fields": [
+ {"id": 1, "name": "id", "required": true, "type": "long"},
+ {"id": 2, "name": "payload", "required": false, "type": "string"},
+ {"id": 2147483540, "name": "_row_id", "required": false, "type": "long"}
+ ]
+ }
+ ],
+ "default-spec-id": 0,
+ "partition-specs": [{"spec-id": 0, "fields": []}],
+ "last-partition-id": 999,
+ "default-sort-order-id": 0,
+ "sort-orders": [{"order-id": 0, "fields": []}],
+ "properties": {},
+ "current-snapshot-id": -1,
+ "snapshots": []
+ }`
+
+ metadata, err := ParseMetadataString(metadataJSON)
require.NoError(t, err)
assert.Equal(t, 3, metadata.Version(), "sanity: must be v3")
@@ -1125,5 +1145,6 @@ func TestProjectionV3SchemaAlreadyHasRowID(t *testing.T) {
}
seen[f.ID] = f.Name
}
+ assert.Contains(t, seen, iceberg.RowIDFieldID, "_row_id must survive projection")
})
}