zeroshade commented on code in PR #1646:
URL: https://github.com/apache/iceberg-go/pull/1646#discussion_r3732167603
##########
view/metadata.go:
##########
@@ -654,18 +654,26 @@ func (m *metadata) init() {
func (m *metadata) UnmarshalJSON(b []byte) error {
type Alias metadata
- aux := (*Alias)(m)
-
- aux.FormatVersionValue = -1
- aux.CurrentVersionIDValue = -1
+ next := metadata{
+ FormatVersionValue: -1,
+ CurrentVersionIDValue: -1,
+ }
+ aux := (*Alias)(&next)
if err := json.Unmarshal(b, aux); err != nil {
return err
}
+ next.init()
+
+ if err := next.validate(); err != nil {
+ return err
+ }
+
+ *m = next
m.init()
Review Comment:
This call is required, not redundant — which is worth a one-line comment
saying so. `init()` installs `sync.OnceValue` closures that capture the
*receiver pointer* (`view/metadata.go:647-652`), so the closures copied in by
`*m = next` on the line above are bound to the dead `next` local rather than to
`m`.
Without a note the call reads as dead code, and deleting it produces a
latent bug rather than an immediate one: the lazy indexes still return correct
answers, they just keep `next` alive through the closure. That is the kind of
defect that survives review indefinitely.
Consider something like:
```go
// init() binds the lazy-index closures to the receiver; rebind after
the copy.
m.init()
```
`TestMetadataUnmarshalReplacesLookupCaches` already covers the behavior
well; this is only about making the reason legible at the call site.
##########
view/metadata_test.go:
##########
@@ -143,6 +144,64 @@ func TestUnmarshalViewMetadata(t *testing.T) {
assert.Equal(t, iceberg.Properties{"prop": "value"}, md.Properties())
}
+func TestMetadataUnmarshalReplacesReceiverState(t *testing.T) {
+ var metadata metadata
+ require.NoError(t, json.Unmarshal([]byte(exampleViewJSON), &metadata))
+
+ var reduced map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(exampleViewJSON), &reduced))
+ delete(reduced, "properties")
+ delete(reduced, "version-log")
+ reducedData, err := json.Marshal(reduced)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(reducedData, &metadata))
+ assert.Empty(t, metadata.Props)
+ assert.Empty(t, metadata.VersionLogList)
+
+ invalid := strings.Replace(exampleViewJSON, `"current-version-id": 1`,
`"current-version-id": 99`, 1)
+ require.Error(t, json.Unmarshal([]byte(invalid), &metadata))
+ assert.Equal(t, int64(1), metadata.CurrentVersionIDValue)
Review Comment:
Non-blocking: these three field assertions cannot catch a *partial*
regression — a future change that reset `Props` and `VersionLogList` but forgot
`SchemaList` would still pass here. `assert.Equal` on the whole struct is not
an option, since `reflect.DeepEqual` is always false for a struct holding
non-nil func fields (the `sync.OnceValue` closures).
A workable substitute: snapshot `json.Marshal(&metadata)` immediately before
the failing decode and assert the bytes are identical afterwards. That covers
every serialized field at once, at roughly the cost of the three assertions it
replaces.
##########
view/metadata.go:
##########
@@ -654,18 +654,26 @@ func (m *metadata) init() {
func (m *metadata) UnmarshalJSON(b []byte) error {
type Alias metadata
- aux := (*Alias)(m)
-
- aux.FormatVersionValue = -1
- aux.CurrentVersionIDValue = -1
+ next := metadata{
+ FormatVersionValue: -1,
+ CurrentVersionIDValue: -1,
+ }
+ aux := (*Alias)(&next)
if err := json.Unmarshal(b, aux); err != nil {
return err
}
+ next.init()
Review Comment:
Non-blocking: the schema index is now built twice per successful parse —
once here, because `validate()` forces `next.lazySchemasByID()` via
`checkVersionSchemasExist`, and again lazily after `m.init()` on line 674
resets the `OnceValue`. Harmless at these sizes.
If you would rather avoid it, one option is to validate against a locally
computed index and install the closures once, after the copy.
--
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]
