laskoviymishka commented on code in PR #1425:
URL: https://github.com/apache/iceberg-go/pull/1425#discussion_r3550067539
##########
table/update_schema.go:
##########
@@ -836,10 +836,15 @@ func (a *applyChanges) List(listType iceberg.ListType,
elementResult iceberg.Typ
panic(fmt.Sprintf("cannot delete element type from list: %s",
elementResult))
}
+ elementRequired := listType.ElementRequired
+ if update, ok := a.updates[listType.ElementID]; ok {
Review Comment:
this reads `update.Required` whenever any update record exists, which is
correct today only because `updateColumn` initializes the update from a copy of
the original field — so for a type-promotion-only or doc-only change,
`update.Required` happens to equal the original optionality.
It works, but the correctness lives in a detail two functions away rather
than here. If `updateColumn` ever switches to lazily populating only the
touched fields, this silently starts writing a zero-value `false`. I'd either
mirror `Struct()` (which copies the field wholesale) or gate on an explicit
"was Required set" signal so this site can tell the difference.
Same shape at line 877 for `mapType.ValueID`. Since `Struct()`, `List()`,
and `Map()` now all do this lookup, could we pull it into a small
`resolveRequired(fieldID, original)` helper? wdyt?
##########
table/update_schema_test.go:
##########
@@ -1043,6 +1043,192 @@ func TestAddColumnMonotonicFieldIDs(t *testing.T) {
"new field id must be allocated above metadata.LastColumnID()
(13), not reused from the current schema's highest id (11)")
}
+func TestApplyChangesListElementOptionalityUpdates(t *testing.T) {
+ newListSchema := func(elementRequired bool, elementType iceberg.Type)
*iceberg.Schema {
+ return iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int32, Required: true},
+ iceberg.NestedField{ID: 2, Name: "tags", Type:
&iceberg.ListType{
+ ElementID: 3,
+ Element: elementType,
+ ElementRequired: elementRequired,
+ }, Required: false},
+ )
+ }
+
+ t.Run("required -> optional is applied", func(t *testing.T) {
+ meta, err := NewMetadata(newListSchema(true,
iceberg.PrimitiveTypes.String), nil, UnsortedSortOrder, "", nil)
+ assert.NoError(t, err)
+
+ table := New([]string{"id"}, meta, "", nil, nil)
+ newSchema, err := NewUpdateSchema(table.NewTransaction(), true,
false).
+ UpdateColumn([]string{"tags", "element"}, ColumnUpdate{
+ Required: iceberg.Optional[bool]{Val: false,
Valid: true},
+ }).Apply()
+ assert.NoError(t, err)
+
+ tags, ok := newSchema.FindFieldByName("tags")
+ assert.True(t, ok)
+ listType, ok := tags.Type.(*iceberg.ListType)
+ assert.True(t, ok)
+ assert.False(t, listType.ElementRequired,
+ "list element optionality update must be reflected in
the applied schema")
+ })
+
+ t.Run("optional -> required is applied with allowIncompatibleChanges",
func(t *testing.T) {
Review Comment:
We cover optional→required *with* `allowIncompatibleChanges`, but never
assert the rejection when it's off — and that guard (`update_schema.go:463`) is
the whole safety contract for tightening nullability on elements/values. Java
and Py both test this rejection on nested element/value fields; we test the
apply side but not the guard-rail.
The worry is a future refactor that moves or misroutes the check downstream:
the struct-field error tests would still pass while list/map silently
regressed. A subtest per List/Map like:
```go
_, err := NewUpdateSchema(table.NewTransaction(), true, false).
UpdateColumn([]string{"tags", "element"}, ColumnUpdate{
Required: iceberg.Optional[bool]{Val: true, Valid: true},
}).Apply()
assert.Error(t, err)
assert.Contains(t, err.Error(), "cannot change column nullability from
optional to required")
```
would pin it. A doc-only-update subtest (registers an update but leaves
`Required.Valid` false) would also lock in the fallback behavior from the
comment on `update_schema.go`.
##########
table/update_schema_test.go:
##########
@@ -1043,6 +1043,192 @@ func TestAddColumnMonotonicFieldIDs(t *testing.T) {
"new field id must be allocated above metadata.LastColumnID()
(13), not reused from the current schema's highest id (11)")
}
+func TestApplyChangesListElementOptionalityUpdates(t *testing.T) {
+ newListSchema := func(elementRequired bool, elementType iceberg.Type)
*iceberg.Schema {
+ return iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int32, Required: true},
+ iceberg.NestedField{ID: 2, Name: "tags", Type:
&iceberg.ListType{
+ ElementID: 3,
+ Element: elementType,
+ ElementRequired: elementRequired,
+ }, Required: false},
+ )
+ }
+
+ t.Run("required -> optional is applied", func(t *testing.T) {
+ meta, err := NewMetadata(newListSchema(true,
iceberg.PrimitiveTypes.String), nil, UnsortedSortOrder, "", nil)
+ assert.NoError(t, err)
+
+ table := New([]string{"id"}, meta, "", nil, nil)
+ newSchema, err := NewUpdateSchema(table.NewTransaction(), true,
false).
+ UpdateColumn([]string{"tags", "element"}, ColumnUpdate{
+ Required: iceberg.Optional[bool]{Val: false,
Valid: true},
+ }).Apply()
+ assert.NoError(t, err)
+
+ tags, ok := newSchema.FindFieldByName("tags")
+ assert.True(t, ok)
+ listType, ok := tags.Type.(*iceberg.ListType)
Review Comment:
`assert.True` doesn't stop the test, so on a regression where `ok` is false
we fall through to a nil `listType` and the next line panics on the deref — a
confusing stack trace instead of a clean failure message. This repeats in all
eight subtests.
I'd switch the two guard checks in each to `require.True(t, ok, ...)` (add
the `testify/require` import) so a bad assertion fails cleanly right there.
--
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]
