You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/01 23:36:40 UTC
[PR] fix(schema): replace state during JSON decoding [iceberg-go]
fallintoplace opened a new pull request, #1628:
URL: https://github.com/apache/iceberg-go/pull/1628
`Schema.UnmarshalJSON` decoded through the existing receiver, so omitted fields could retain values from an earlier document. Validation failures could also leave the receiver partially modified.
Decode into fresh temporary state, validate it, and replace the receiver only after success. Reinitialize the lazy schema indexes after replacement.
Tests cover successful reuse, clearing omitted values and cached indexes, plus preserving the original schema after invalid input.
Tested with: `go test .`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(schema): replace state during JSON decoding [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1628:
URL: https://github.com/apache/iceberg-go/pull/1628#discussion_r3719815211
##########
schema_test.go:
##########
@@ -570,6 +570,65 @@ func TestUnmarshalSchema(t *testing.T) {
assert.True(t, tableSchemaSimple.Equals(&schema))
}
+func TestUnmarshalSchemaReplacesExistingState(t *testing.T) {
+ schema := iceberg.NewSchemaWithIdentifiers(7, []int{1},
+ iceberg.NestedField{ID: 1, Name: "old", Type: iceberg.PrimitiveTypes.String},
+ )
+ _, ok := schema.FindFieldByID(1)
+ require.True(t, ok)
+ _, ok = schema.FindColumnName(1)
+ require.True(t, ok)
+ _, ok = schema.FindFieldByName("old")
+ require.True(t, ok)
+ _, ok = schema.FindFieldByNameCaseInsensitive("OLD")
+ require.True(t, ok)
+ assert.Contains(t, schema.NameMapping().String(), "old")
+
+ require.NoError(t, json.Unmarshal([]byte(`{
+ "type": "struct",
+ "fields": [{"id": 2, "name": "new", "type": "long", "required": true}]
+ }`), schema))
+
+ assert.Zero(t, schema.ID)
+ assert.Empty(t, schema.IdentifierFieldIDs)
+ assert.Equal(t, 1, schema.NumFields())
+ _, ok = schema.FindFieldByID(1)
+ assert.False(t, ok)
+ _, ok = schema.FindColumnName(1)
+ assert.False(t, ok)
+ _, ok = schema.FindFieldByName("old")
+ assert.False(t, ok)
+ _, ok = schema.FindFieldByNameCaseInsensitive("OLD")
+ assert.False(t, ok)
+ field, ok := schema.FindFieldByID(2)
+ require.True(t, ok)
+ assert.Equal(t, "new", field.Name)
+ assert.Contains(t, schema.NameMapping().String(), "new")
+ assert.NotContains(t, schema.NameMapping().String(), "old")
+}
+
+func TestUnmarshalSchemaPreservesExistingStateOnError(t *testing.T) {
+ schema := iceberg.NewSchemaWithIdentifiers(7, []int{1},
+ iceberg.NestedField{ID: 1, Name: "old", Type: iceberg.PrimitiveTypes.String},
+ )
+
+ err := json.Unmarshal([]byte(`{
+ "type": "struct",
+ "fields": [
+ {"id": 2, "name": "first", "type": "long", "required": true},
+ {"id": 2, "name": "duplicate", "type": "string", "required": false}
+ ]
+ }`), schema)
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+
+ assert.Equal(t, 7, schema.ID)
+ assert.Equal(t, []int{1}, schema.IdentifierFieldIDs)
+ assert.Equal(t, 1, schema.NumFields())
+ field, ok := schema.FindFieldByID(1)
Review Comment:
This doesn't actually prove the caches survive the error.
`FindFieldByID` goes through `lazyIDToField`, which rebuilds from `s.fields` on a nil atomic. So even if the error path had wrongly zeroed the caches, this would still pass by recomputing from the unchanged `fields` slice, and the test can't tell "caches left intact" from "caches cleared but recomputed". Since stale cached lookups are exactly the bug we're guarding against, I'd warm every lookup (`FindFieldByID`, `FindFieldByName`, `FindColumnName`, `FindFieldByNameCaseInsensitive`, `NameMapping()`) before the failing unmarshal, then assert they still return the old field afterward.
##########
schema_test.go:
##########
@@ -570,6 +570,65 @@ func TestUnmarshalSchema(t *testing.T) {
assert.True(t, tableSchemaSimple.Equals(&schema))
}
+func TestUnmarshalSchemaReplacesExistingState(t *testing.T) {
+ schema := iceberg.NewSchemaWithIdentifiers(7, []int{1},
+ iceberg.NestedField{ID: 1, Name: "old", Type: iceberg.PrimitiveTypes.String},
+ )
+ _, ok := schema.FindFieldByID(1)
+ require.True(t, ok)
+ _, ok = schema.FindColumnName(1)
+ require.True(t, ok)
+ _, ok = schema.FindFieldByName("old")
+ require.True(t, ok)
+ _, ok = schema.FindFieldByNameCaseInsensitive("OLD")
+ require.True(t, ok)
+ assert.Contains(t, schema.NameMapping().String(), "old")
Review Comment:
The pre-warm seeds `lazyNameMapping` and the four atomic caches, but skips `lazyIDToParent`, and that's the one cache reset by `init()` rather than an explicit `Store(nil)`, so it's the most likely to regress if `init()` ever changes.
Could we seed it with `schema.FieldHasOptionalParent(1)` before the unmarshal, then assert `FieldHasOptionalParent(2)` is `false` after the reuse? wdyt?
##########
schema.go:
##########
@@ -290,13 +291,21 @@ func (s *Schema) UnmarshalJSON(b []byte) error {
return err
}
- s.init()
-
- s.fields = aux.Fields
- if s.IdentifierFieldIDs == nil {
- s.IdentifierFieldIDs = []int{}
+ decoded.fields = aux.Fields
+ if decoded.IdentifierFieldIDs == nil {
+ decoded.IdentifierFieldIDs = []int{}
}
+ s.ID = decoded.ID
+ s.IdentifierFieldIDs = decoded.IdentifierFieldIDs
+ s.fields = decoded.fields
+ s.idToName.Store(nil)
+ s.idToField.Store(nil)
+ s.nameToID.Store(nil)
+ s.nameToIDLower.Store(nil)
+ s.idToAccessor.Store(nil)
+ s.init()
Review Comment:
Small ordering thing: we write `s.fields`/`s.ID`/`s.IdentifierFieldIDs` first and then clear the five atomics. In the window between, a concurrent reader on a lazy-rebuild path can recompute an index from the new fields and store it, only for the `Store(nil)` to immediately throw it away. Harmless today, and mutating a `*Schema` concurrently is racy regardless, but the idiomatic order is clear the derived caches first, then overwrite the authoritative fields, then `init()`.
While we're here: this clears 5 of the 7 cached values explicitly but leaves `lazyIDToParent` and `lazyNameMapping` to `init()`. That's correct, but non-obvious. A one-line comment noting those two are reset by `init()` because they're func values rather than atomics would save the next person from wondering why they're not in the `Store(nil)` list.
##########
schema.go:
##########
@@ -290,13 +291,21 @@ func (s *Schema) UnmarshalJSON(b []byte) error {
return err
}
- s.init()
-
- s.fields = aux.Fields
- if s.IdentifierFieldIDs == nil {
- s.IdentifierFieldIDs = []int{}
+ decoded.fields = aux.Fields
+ if decoded.IdentifierFieldIDs == nil {
+ decoded.IdentifierFieldIDs = []int{}
}
+ s.ID = decoded.ID
Review Comment:
This manual three-field copy is a bit of a trap. If someone later adds a new JSON-tagged field to `Schema`, they'll wire up the tag and `MarshalJSON` but won't know to update this block, so the value decodes into `decoded` and then gets silently dropped on the re-unmarshal path, with nothing at compile time to catch it.
I'd add a comment right here spelling out the contract: decode-into-temp-then-copy is deliberate (validate before we touch the receiver, the same reason Java's `SchemaParser.fromJson` always returns a fresh `Schema`), and any new JSON-decodable field has to be copied here too. wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(schema): replace state during JSON decoding [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1628:
URL: https://github.com/apache/iceberg-go/pull/1628
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org