zeroshade commented on code in PR #1648:
URL: https://github.com/apache/iceberg-go/pull/1648#discussion_r3732167999
##########
table/metadata_internal_test.go:
##########
@@ -1402,6 +1478,7 @@ func TestMetadataV2Validation(t *testing.T) {
"current-schema-id": 0,
"last-partition-id": 1000,
"schemas": [{"type":"struct","schema-id":0,"fields":[]}],
+ "default-spec-id": 0,
Review Comment:
This added line — and the matching one at `:1501` — is the proof that the
description is inaccurate, and it is the main thing worth changing here.
"Existing version-specific defaults and sentinels are preserved" reads as a
no-op, but they are now applied unconditionally, so direct `UnmarshalJSON` is
strictly more rejecting than before. `initMetadataV2Deser()` yields
`DefaultSpecID = -1`, and `commonMetadata.preValidate` has no `-1 → max(specs)`
fallback — only `metadataV1.preValidate` does, at
`table/metadata.go:2444-2446`. That is exactly why these two fixtures had to
gain `"default-spec-id": 0`. The same applies to v3 `next-row-id`: a zero-value
receiver gave `0` and passed, whereas `-1` now reaches `checkNextRowID`'s
"next-row-id is required for v3 tables" (`table/metadata.go:2705-2709`).
This is the right behavior — it matches what `ParseMetadataBytes` already
enforced, and `metadataV{1,2,3}` are unexported so no external caller can
observe the change. Consider rewording the commit message to say so, e.g. "the
deserialization sentinels are now applied on every decode path, so
`default-spec-id` / `next-row-id` are required for v2/v3 documents decoded
directly, matching `ParseMetadataBytes`."
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,80 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+func TestMetadataUnmarshalReplacesReceiverState(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ target any
+ }{
+ {name: "v1", data: ExampleTableMetadataV1, target:
&metadataV1{}},
+ {name: "v2", data: ExampleTableMetadataV2, target:
&metadataV2{}},
+ {name: "v3", data: ExampleTableMetadataV3, target:
&metadataV3{}},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
tt.target))
+
+ var reduced map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
&reduced))
+ for _, key := range []string{
+ "properties", "current-snapshot-id",
"snapshots", "snapshot-log", "metadata-log", "refs",
Review Comment:
Non-blocking: this delete-list omits several `omitempty` `commonMetadata`
fields in the same bug class — `statistics`, `partition-statistics`,
`encryption-keys`, `last-partition-id`, and `last-sequence-number` would all
have survived a reuse identically. `last-sequence-number` is free to add; the
rest would need fixture extension first.
##########
table/metadata.go:
##########
@@ -2465,7 +2465,8 @@ func (m *metadataV1) preValidate() {
func (m *metadataV1) UnmarshalJSON(b []byte) error {
type Alias metadataV1
- aux := (*Alias)(m)
+ next := initMetadataV1Deser()
Review Comment:
Worth adding to the description: this fixes two things beyond the stated one.
First, `commonMetadata.preValidate()` injects a `main` entry into
`SnapshotRefs` when `CurrentSnapshotID != nil` (`table/metadata.go:2161-2168`).
On a reused receiver whose new document omitted `refs`, that injection mutated
the **previous** document's map. `TestMetadataUnmarshalReplacesReceiverState`'s
`assert.Nil(t, common.CurrentSnapshotID)` plus `assert.Empty(t,
common.SnapshotRefs)` is exactly the right assertion for it.
Second, v1's `if aux.CurrentSchemaID == -1 && aux.Schema != nil` branch
(`table/metadata.go:2488-2495`) previously never fired on a zero-value
receiver, because `CurrentSchemaID` was `0` rather than the `-1` sentinel.
Direct `UnmarshalJSON` therefore rejected v1 documents that
`ParseMetadataBytes` accepted. The two paths are now consistent.
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,80 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+func TestMetadataUnmarshalReplacesReceiverState(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ target any
+ }{
+ {name: "v1", data: ExampleTableMetadataV1, target:
&metadataV1{}},
Review Comment:
Non-blocking: the v1 subtest is close to vacuous. `ExampleTableMetadataV1`
(`table/metadata_internal_test.go:181-199`) has `"properties": {}`, no `refs`,
no `snapshot-log`, no `metadata-log`, and `"current-snapshot-id": -1`
(normalized to nil). Five of the six `assert.Empty`/`assert.Nil` calls below
therefore already hold *before* the second decode, and would pass with the fix
reverted. Only `SnapshotList` is genuinely exercised for v1.
Either extend the v1 fixture, or — cheaper and more durable — add guard
assertions right after the first decode (`require.NotEmpty(t, common.Props)`,
and so on) so no version's subtest can silently go vacuous as fixtures change.
##########
table/metadata.go:
##########
@@ -2465,7 +2465,8 @@ func (m *metadataV1) preValidate() {
func (m *metadataV1) UnmarshalJSON(b []byte) error {
type Alias metadataV1
- aux := (*Alias)(m)
+ next := initMetadataV1Deser()
+ aux := (*Alias)(next)
// Set LastColumnId to -1 to indicate that it is not set as
LastColumnId = 0 is a valid value for when no schema is present
aux.LastColumnId = -1
Review Comment:
Non-blocking: this assignment and its comment are now dead code. `next`
comes from `initMetadataV1Deser()`, and
`initCommonMetadataForDeserialization()` (`table/metadata.go:1660`) already
sets `LastColumnId` to `-1`. Same at `:2554` and `:2637`.
Leaving both makes it ambiguous which one is authoritative. Consider
dropping the redundant assignments and keeping the explanatory comment at the
`init...Deser` helper, where the value actually originates.
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,80 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+func TestMetadataUnmarshalReplacesReceiverState(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ target any
+ }{
+ {name: "v1", data: ExampleTableMetadataV1, target:
&metadataV1{}},
+ {name: "v2", data: ExampleTableMetadataV2, target:
&metadataV2{}},
+ {name: "v3", data: ExampleTableMetadataV3, target:
&metadataV3{}},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
tt.target))
+
+ var reduced map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
&reduced))
+ for _, key := range []string{
+ "properties", "current-snapshot-id",
"snapshots", "snapshot-log", "metadata-log", "refs",
+ } {
+ delete(reduced, key)
+ }
+ reducedData, err := json.Marshal(reduced)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(reducedData,
tt.target))
+
+ var common *commonMetadata
+ switch metadata := tt.target.(type) {
+ case *metadataV1:
+ common = &metadata.commonMetadata
+ case *metadataV2:
+ common = &metadata.commonMetadata
+ case *metadataV3:
+ common = &metadata.commonMetadata
+ }
+ assert.Empty(t, common.Props)
+ assert.Empty(t, common.SnapshotList)
+ assert.Empty(t, common.SnapshotLog)
+ assert.Empty(t, common.MetadataLog)
+ assert.Empty(t, common.SnapshotRefs)
+ assert.Nil(t, common.CurrentSnapshotID)
+ })
+ }
+}
+
+func TestMetadataV2UnmarshalPreservesStateOnError(t *testing.T) {
Review Comment:
Non-blocking: failure-path coverage exists only for v2, yet all three
`UnmarshalJSON`s changed.
v1 needs it most — `metadataV1.preValidate` (`table/metadata.go:2432-2464`)
mutates `SchemaList`, `Specs`, `DefaultSpecID`, `LastPartitionID`, and
`SortOrderList` before validation runs, by far the largest partial-mutation
surface of the three. v3 likewise fails inside `checkNextRowID` after
`preValidate` has already run.
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,80 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+func TestMetadataUnmarshalReplacesReceiverState(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ target any
+ }{
+ {name: "v1", data: ExampleTableMetadataV1, target:
&metadataV1{}},
+ {name: "v2", data: ExampleTableMetadataV2, target:
&metadataV2{}},
+ {name: "v3", data: ExampleTableMetadataV3, target:
&metadataV3{}},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
tt.target))
+
+ var reduced map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
&reduced))
+ for _, key := range []string{
+ "properties", "current-snapshot-id",
"snapshots", "snapshot-log", "metadata-log", "refs",
+ } {
+ delete(reduced, key)
+ }
+ reducedData, err := json.Marshal(reduced)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(reducedData,
tt.target))
+
+ var common *commonMetadata
+ switch metadata := tt.target.(type) {
+ case *metadataV1:
+ common = &metadata.commonMetadata
+ case *metadataV2:
+ common = &metadata.commonMetadata
+ case *metadataV3:
+ common = &metadata.commonMetadata
+ }
+ assert.Empty(t, common.Props)
+ assert.Empty(t, common.SnapshotList)
+ assert.Empty(t, common.SnapshotLog)
+ assert.Empty(t, common.MetadataLog)
+ assert.Empty(t, common.SnapshotRefs)
+ assert.Nil(t, common.CurrentSnapshotID)
+ })
+ }
+}
+
+func TestMetadataV2UnmarshalPreservesStateOnError(t *testing.T) {
+ var metadata metadataV2
+ require.NoError(t, json.Unmarshal([]byte(ExampleTableMetadataV2),
&metadata))
+
+ invalid := strings.Replace(ExampleTableMetadataV2,
`"current-schema-id": 1`, `"current-schema-id": 99`, 1)
+ require.Error(t, json.Unmarshal([]byte(invalid), &metadata))
+
+ assert.Equal(t, 1, metadata.CurrentSchemaID)
Review Comment:
Non-blocking, and the same suggestion made on #1646: rather than three
hand-picked fields, snapshot `json.Marshal(&metadata)` before the failing
decode and assert the bytes are unchanged afterwards. That covers every
serialized field at once and would catch a partial regression that these three
assertions would miss.
--
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]
