This is an automated email from the ASF dual-hosted git repository.
laskoviymishka 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 ded986b42 refactor(table): deduplicate schema default validation
(#2298)
ded986b42 is described below
commit ded986b4233a6c548655973d851b9765bdf5cdfe
Author: Minh Vu <[email protected]>
AuthorDate: Sun Oct 25 10:10:22 2026 +0100
refactor(table): deduplicate schema default validation (#2298)
---
table/metadata_schema_compatibility.go | 179 ++++++++++--------------
table/metadata_schema_compatibility_test.go | 208 ++++++++++++++++++++++++++++
2 files changed, 281 insertions(+), 106 deletions(-)
diff --git a/table/metadata_schema_compatibility.go
b/table/metadata_schema_compatibility.go
index e10ed0aa8..62599575e 100644
--- a/table/metadata_schema_compatibility.go
+++ b/table/metadata_schema_compatibility.go
@@ -270,106 +270,75 @@ func typeRequiresNullDefaults(t iceberg.Type) bool {
}
}
+// isRequiredUnknownField identifies unknown fields that violate the
optionality
+// rule, regardless of whether they appear in a struct, list, or map.
+func isRequiredUnknownField(field iceberg.NestedField) bool {
+ _, isUnknown := field.Type.(iceberg.UnknownType)
+
+ return isUnknown && field.Required
+}
+
+// validateUnknownFieldDefaults keeps the null-default rule for unknown and
+// variant types in one place for all schema container types.
+func validateUnknownFieldDefaults(field iceberg.NestedField) error {
+ if !typeRequiresNullDefaults(field.Type) {
+ return nil
+ }
+ if field.InitialDefault != nil {
+ return fmt.Errorf("%s type field '%s' (id: %d) must have null
initial-default, but got: %v",
+ field.Type, field.Name, field.ID, field.InitialDefault)
+ }
+ if field.WriteDefault != nil {
+ return fmt.Errorf("%s type field '%s' (id: %d) must have null
write-default, but got: %v",
+ field.Type, field.Name, field.ID, field.WriteDefault)
+ }
+
+ return nil
+}
+
+// Nested list and map fields have historically used slightly different
+// wording from struct fields when reporting a required unknown type.
+func validateNestedUnknownField(field iceberg.NestedField) error {
+ if isRequiredUnknownField(field) {
+ return fmt.Errorf("unknown type field '%s' (id: %d) must be
optional, but was marked required",
+ field.Name, field.ID)
+ }
+
+ return validateUnknownFieldDefaults(field)
+}
+
func (v *unknownTypeValidator) Field(field iceberg.NestedField, fieldResult
error) error {
if fieldResult != nil {
return fieldResult
}
- // Optionality: unknown must be optional per spec; variant has no such
constraint.
- // Both require null defaults (enforced below via
typeRequiresNullDefaults).
- if _, isUnknown := field.Type.(iceberg.UnknownType); isUnknown {
- if field.Required {
- return fmt.Errorf("unknown type field '%s' (id: %d)
must be optional, but was marked as required",
- field.Name, field.ID)
- }
- }
- if typeRequiresNullDefaults(field.Type) {
- if field.InitialDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null initial-default, but got: %v",
- field.Type, field.Name, field.ID,
field.InitialDefault)
- }
- if field.WriteDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null write-default, but got: %v",
- field.Type, field.Name, field.ID,
field.WriteDefault)
- }
+ if isRequiredUnknownField(field) {
+ return fmt.Errorf("unknown type field '%s' (id: %d) must be
optional, but was marked as required",
+ field.Name, field.ID)
}
- return nil
+ return validateUnknownFieldDefaults(field)
}
func (v *unknownTypeValidator) List(list iceberg.ListType, elemResult error)
error {
if elemResult != nil {
return elemResult
}
- elem := list.ElementField()
-
- if _, isUnknown := elem.Type.(iceberg.UnknownType); isUnknown {
- if elem.Required {
- return fmt.Errorf("unknown type field '%s' (id: %d)
must be optional, but was marked required",
- elem.Name, elem.ID)
- }
- }
- if typeRequiresNullDefaults(elem.Type) {
- if elem.InitialDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null initial-default, but got: %v",
- elem.Type, elem.Name, elem.ID,
elem.InitialDefault)
- }
- if elem.WriteDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null write-default, but got: %v",
- elem.Type, elem.Name, elem.ID,
elem.WriteDefault)
- }
- }
- return nil
+ return validateNestedUnknownField(list.ElementField())
}
func (v *unknownTypeValidator) Map(mapType iceberg.MapType, keyResult,
valueResult error) error {
if keyResult != nil {
return keyResult
}
-
if valueResult != nil {
return valueResult
}
-
- key := mapType.KeyField()
-
- if _, isKeyUnknown := key.Type.(iceberg.UnknownType); isKeyUnknown {
- if key.Required {
- return fmt.Errorf("unknown type field '%s' (id: %d)
must be optional, but was marked required",
- key.Name, key.ID)
- }
- }
- if typeRequiresNullDefaults(key.Type) {
- if key.InitialDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null initial-default, but got: %v",
- key.Type, key.Name, key.ID, key.InitialDefault)
- }
- if key.WriteDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null write-default, but got: %v",
- key.Type, key.Name, key.ID, key.WriteDefault)
- }
- }
-
- value := mapType.ValueField()
-
- if _, isValueUnknown := value.Type.(iceberg.UnknownType);
isValueUnknown {
- if value.Required {
- return fmt.Errorf("unknown type field '%s' (id: %d)
must be optional, but was marked required",
- value.Name, value.ID)
- }
- }
- if typeRequiresNullDefaults(value.Type) {
- if value.InitialDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null initial-default, but got: %v",
- value.Type, value.Name, value.ID,
value.InitialDefault)
- }
- if value.WriteDefault != nil {
- return fmt.Errorf("%s type field '%s' (id: %d) must
have null write-default, but got: %v",
- value.Type, value.Name, value.ID,
value.WriteDefault)
- }
+ if err := validateNestedUnknownField(mapType.KeyField()); err != nil {
+ return err
}
- return nil
+ return validateNestedUnknownField(mapType.ValueField())
}
func (v *unknownTypeValidator) Primitive(_ iceberg.PrimitiveType) error {
@@ -447,40 +416,38 @@ func (v *complexTypeDefaultValidator) Variant(_
iceberg.VariantType) error {
}
func validateComplexDefault(field iceberg.NestedField) error {
+ if err := validateComplexDefaultValue(field, "initial-default",
field.InitialDefault); err != nil {
+ return err
+ }
+
+ return validateComplexDefaultValue(field, "write-default",
field.WriteDefault)
+}
+
+// validateComplexDefaultValue checks the expected JSON shape without
+// duplicating the initial-default and write-default error handling.
+func validateComplexDefaultValue(field iceberg.NestedField, name string, value
any) error {
+ if value == nil {
+ return nil
+ }
+
+ var typeName, expectedShape string
+ var valid bool
switch field.Type.(type) {
case *iceberg.StructType:
- if field.InitialDefault != nil {
- if _, ok := field.InitialDefault.(map[string]any); !ok {
- return fmt.Errorf("struct type field '%s' (id:
%d) must have null or JSON object initial-default, but got: %v", field.Name,
field.ID, field.InitialDefault)
- }
- }
- if field.WriteDefault != nil {
- if _, ok := field.WriteDefault.(map[string]any); !ok {
- return fmt.Errorf("struct type field '%s' (id:
%d) must have null or JSON object write-default, but got: %v", field.Name,
field.ID, field.WriteDefault)
- }
- }
+ typeName, expectedShape = "struct", "JSON object"
+ _, valid = value.(map[string]any)
case *iceberg.ListType:
- if field.InitialDefault != nil {
- if _, ok := field.InitialDefault.([]any); !ok {
- return fmt.Errorf("list type field '%s' (id:
%d) must have null or JSON array initial-default, but got: %v", field.Name,
field.ID, field.InitialDefault)
- }
- }
- if field.WriteDefault != nil {
- if _, ok := field.WriteDefault.([]any); !ok {
- return fmt.Errorf("list type field '%s' (id:
%d) must have null or JSON array write-default, but got: %v", field.Name,
field.ID, field.WriteDefault)
- }
- }
+ typeName, expectedShape = "list", "JSON array"
+ _, valid = value.([]any)
case *iceberg.MapType:
- if field.InitialDefault != nil {
- if _, ok := field.InitialDefault.(map[string]any); !ok {
- return fmt.Errorf("map type field '%s' (id: %d)
must have null or JSON object initial-default, but got: %v", field.Name,
field.ID, field.InitialDefault)
- }
- }
- if field.WriteDefault != nil {
- if _, ok := field.WriteDefault.(map[string]any); !ok {
- return fmt.Errorf("map type field '%s' (id: %d)
must have null or JSON object write-default, but got: %v", field.Name,
field.ID, field.WriteDefault)
- }
- }
+ typeName, expectedShape = "map", "JSON object"
+ _, valid = value.(map[string]any)
+ default:
+ return nil
+ }
+ if !valid {
+ return fmt.Errorf("%s type field '%s' (id: %d) must have null
or %s %s, but got: %v",
+ typeName, field.Name, field.ID, expectedShape, name,
value)
}
return nil
diff --git a/table/metadata_schema_compatibility_test.go
b/table/metadata_schema_compatibility_test.go
index 2fc5aca7a..cb665bea1 100644
--- a/table/metadata_schema_compatibility_test.go
+++ b/table/metadata_schema_compatibility_test.go
@@ -50,3 +50,211 @@ func TestNewMetadata_DuplicateValues(t *testing.T) {
assert.ErrorIs(t, err, iceberg.ErrInvalidSchema)
assert.Contains(t, err.Error(), "multiple fields for name foo")
}
+
+func TestUnknownTypeValidatorRetainsErrorBehavior(t *testing.T) {
+ validator := &unknownTypeValidator{}
+ tests := []struct {
+ name string
+ validate func() error
+ want string
+ }{
+ {
+ name: "struct field requires optional unknown",
+ validate: func() error {
+ return validator.Field(iceberg.NestedField{
+ ID: 1, Name: "field", Type:
iceberg.UnknownType{}, Required: true,
+ }, nil)
+ },
+ want: "unknown type field 'field' (id: 1) must be
optional, but was marked as required",
+ },
+ {
+ name: "list element requires optional unknown",
+ validate: func() error {
+ return validator.List(iceberg.ListType{
+ ElementID: 2, Element:
iceberg.UnknownType{}, ElementRequired: true,
+ }, nil)
+ },
+ want: "unknown type field 'element' (id: 2) must be
optional, but was marked required",
+ },
+ {
+ name: "map key requires optional unknown",
+ validate: func() error {
+ return validator.Map(iceberg.MapType{
+ KeyID: 3, KeyType:
iceberg.UnknownType{},
+ ValueID: 4, ValueType:
iceberg.StringType{},
+ }, nil, nil)
+ },
+ want: "unknown type field 'key' (id: 3) must be
optional, but was marked required",
+ },
+ {
+ name: "map value requires optional unknown",
+ validate: func() error {
+ return validator.Map(iceberg.MapType{
+ KeyID: 3, KeyType: iceberg.StringType{},
+ ValueID: 4, ValueType:
iceberg.UnknownType{}, ValueRequired: true,
+ }, nil, nil)
+ },
+ want: "unknown type field 'value' (id: 4) must be
optional, but was marked required",
+ },
+ {
+ name: "unknown initial default is rejected",
+ validate: func() error {
+ return validator.Field(iceberg.NestedField{
+ ID: 5, Name: "field", Type:
iceberg.UnknownType{}, InitialDefault: "wrong",
+ }, nil)
+ },
+ want: "unknown type field 'field' (id: 5) must have
null initial-default, but got: wrong",
+ },
+ {
+ name: "unknown write default is rejected",
+ validate: func() error {
+ return validator.Field(iceberg.NestedField{
+ ID: 5, Name: "field", Type:
iceberg.UnknownType{}, WriteDefault: "wrong",
+ }, nil)
+ },
+ want: "unknown type field 'field' (id: 5) must have
null write-default, but got: wrong",
+ },
+ {
+ name: "variant initial default is rejected",
+ validate: func() error {
+ return validator.Field(iceberg.NestedField{
+ ID: 5, Name: "field", Type:
iceberg.VariantType{}, InitialDefault: "wrong",
+ }, nil)
+ },
+ want: "variant type field 'field' (id: 5) must have
null initial-default, but got: wrong",
+ },
+ {
+ name: "variant write default is rejected",
+ validate: func() error {
+ return validator.Field(iceberg.NestedField{
+ ID: 5, Name: "field", Type:
iceberg.VariantType{}, WriteDefault: "wrong",
+ }, nil)
+ },
+ want: "variant type field 'field' (id: 5) must have
null write-default, but got: wrong",
+ },
+ {
+ name: "required variant is allowed",
+ validate: func() error {
+ return validator.Field(iceberg.NestedField{
+ ID: 5, Name: "field", Type:
iceberg.VariantType{}, Required: true,
+ }, nil)
+ },
+ },
+ {
+ name: "regular types retain non-null defaults",
+ validate: func() error {
+ return validator.Field(iceberg.NestedField{
+ ID: 5, Name: "field", Type:
iceberg.StringType{},
+ InitialDefault: "initial",
WriteDefault: "written",
+ }, nil)
+ },
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ err := tt.validate()
+ if tt.want == "" {
+ require.NoError(t, err)
+ } else {
+ require.EqualError(t, err, tt.want)
+ }
+ })
+ }
+}
+
+func TestUnknownTypeValidatorPreservesChildErrorPriority(t *testing.T) {
+ validator := &unknownTypeValidator{}
+ childErr := iceberg.ErrInvalidSchema
+ require.ErrorIs(t, validator.Field(iceberg.NestedField{
+ ID: 1, Name: "field", Type: iceberg.UnknownType{}, Required:
true,
+ }, childErr), childErr)
+ require.ErrorIs(t, validator.List(iceberg.ListType{
+ ElementID: 2, Element: iceberg.UnknownType{}, ElementRequired:
true,
+ }, childErr), childErr)
+
+ mapType := iceberg.MapType{
+ KeyID: 3, KeyType: iceberg.UnknownType{},
+ ValueID: 4, ValueType: iceberg.UnknownType{}, ValueRequired:
true,
+ }
+ require.ErrorIs(t, validator.Map(mapType, childErr, nil), childErr)
+ require.ErrorIs(t, validator.Map(mapType, nil, childErr), childErr)
+}
+
+func TestValidateComplexDefaultRetainsShapeRules(t *testing.T) {
+ tests := []struct {
+ name string
+ typ iceberg.Type
+ initial any
+ write any
+ want string
+ }{
+ {
+ name: "struct initial default must be object",
+ typ: &iceberg.StructType{}, initial: "wrong",
+ want: "struct type field 'field' (id: 5) must have null
or JSON object initial-default, but got: wrong",
+ },
+ {
+ name: "struct write default must be object",
+ typ: &iceberg.StructType{}, write: "wrong",
+ want: "struct type field 'field' (id: 5) must have null
or JSON object write-default, but got: wrong",
+ },
+ {
+ name: "list initial default must be array",
+ typ: &iceberg.ListType{}, initial: "wrong",
+ want: "list type field 'field' (id: 5) must have null
or JSON array initial-default, but got: wrong",
+ },
+ {
+ name: "list write default must be array",
+ typ: &iceberg.ListType{}, write: "wrong",
+ want: "list type field 'field' (id: 5) must have null
or JSON array write-default, but got: wrong",
+ },
+ {
+ name: "map initial default must be object",
+ typ: &iceberg.MapType{}, initial: "wrong",
+ want: "map type field 'field' (id: 5) must have null or
JSON object initial-default, but got: wrong",
+ },
+ {
+ name: "map write default must be object",
+ typ: &iceberg.MapType{}, write: "wrong",
+ want: "map type field 'field' (id: 5) must have null or
JSON object write-default, but got: wrong",
+ },
+ {
+ name: "initial default error takes priority",
+ typ: &iceberg.ListType{}, initial: "initial", write:
"write",
+ want: "list type field 'field' (id: 5) must have null
or JSON array initial-default, but got: initial",
+ },
+ {
+ name: "struct accepts object defaults",
+ typ: &iceberg.StructType{},
+ initial: map[string]any{"a": 1}, write:
map[string]any(nil),
+ },
+ {
+ name: "list accepts array defaults",
+ typ: &iceberg.ListType{},
+ initial: []any{"a"}, write: []any(nil),
+ },
+ {
+ name: "map accepts object defaults",
+ typ: &iceberg.MapType{},
+ initial: map[string]any{"a": 1}, write:
map[string]any{"b": 2},
+ },
+ {
+ name: "primitive defaults unchanged",
+ typ: iceberg.StringType{}, initial: "initial", write:
"write",
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ field := iceberg.NestedField{
+ ID: 5, Name: "field", Type: tt.typ,
+ InitialDefault: tt.initial, WriteDefault:
tt.write,
+ }
+ err := validateComplexDefault(field)
+ if tt.want == "" {
+ require.NoError(t, err)
+ } else {
+ require.EqualError(t, err, tt.want)
+ }
+ })
+ }
+}
