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: [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]
