laskoviymishka commented on code in PR #1648:
URL: https://github.com/apache/iceberg-go/pull/1648#discussion_r3740298879
##########
table/metadata.go:
##########
@@ -1692,6 +1692,7 @@ type commonMetadata struct {
func initCommonMetadataForDeserialization() commonMetadata {
return commonMetadata{
+ // Negative sentinels distinguish omitted required fields from
explicit zero values.
LastColumnId: -1,
Review Comment:
This comment promises more than the validators actually deliver.
`LastUpdatedMS` is set to -1 in the same initializer, but `validate()`
checks `== 0`, so a document that omits `last-updated-ms` keeps the -1 sentinel
and passes. The "distinguish omitted from explicit zero" invariant doesn't hold
for it.
Same shape on `LastSeqNum`, which is the -1 round-trip zeroshade flagged and
which you now document in the reuse test. It's spec-required for v2/v3, so a
`validate()` guard would close it properly, but I'd be fine scoping this
comment to the fields that are really enforced now and leaving the guard as a
follow-up. wdyt?
##########
table/metadata.go:
##########
@@ -2653,18 +2662,22 @@ func (m *metadataV3) Equals(other Metadata) bool {
func (m *metadataV3) UnmarshalJSON(b []byte) error {
type Alias metadataV3
- aux := (*Alias)(m)
-
- // 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
+ next := initMetadataV3Deser()
+ aux := (*Alias)(next)
if err := json.Unmarshal(b, aux); err != nil {
return err
}
- m.preValidate()
+ next.preValidate()
Review Comment:
v1 and v2 `UnmarshalJSON` both call `rejectFieldsBeyondVersion`, v3 doesn't
(pre-existing, but this is the natural spot since the PR reworks this body).
It's harmless today, but once v4 lands, a v3 object read via `json.Unmarshal`
would silently accept v4-only fields where Java's parser rejects them.
Adding the call now with an empty field list documents the pattern and makes
the v4 addition mechanical. Non-blocking, up to you whether it's in scope here.
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,187 @@ 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) {
+ initialData := []byte(tt.data)
+ var initial map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal(initialData,
&initial))
+ setJSON := func(key string, value any) {
+ encoded, err := json.Marshal(value)
+ require.NoError(t, err)
+ initial[key] = encoded
+ }
+ snapshotID := int64(1925)
+ if tt.name != "v1" {
+ snapshotID = 3055729675574597004
+ }
+ setJSON("properties", map[string]any{"seed": "value"})
+ setJSON("current-snapshot-id", snapshotID)
+ setJSON("snapshot-log",
[]any{map[string]any{"snapshot-id": snapshotID, "timestamp-ms": int64(1)}})
+ setJSON("metadata-log",
[]any{map[string]any{"metadata-file": "s3://bucket/old.json", "timestamp-ms":
int64(1)}})
+ setJSON("refs", map[string]any{"main":
map[string]any{"snapshot-id": snapshotID, "type": "branch"}})
+ setJSON("last-partition-id", int64(1000))
+ setJSON("statistics", []any{map[string]any{
+ "snapshot-id": snapshotID, "statistics-path":
"s3://bucket/stats.puffin",
+ "file-size-in-bytes": int64(1),
"file-footer-size-in-bytes": int64(1), "blob-metadata": []any{},
+ }})
+ setJSON("partition-statistics", []any{map[string]any{
+ "snapshot-id": snapshotID, "statistics-path":
"s3://bucket/partition-stats.parquet", "file-size-in-bytes": int64(1),
+ }})
+ if tt.name == "v2" || tt.name == "v3" {
+ setJSON("last-sequence-number", int64(34))
+ }
+ if tt.name == "v3" {
+ setJSON("encryption-keys", []any{map[string]any{
+ "key-id": "key-1",
"encrypted-key-metadata": "YWJj",
+ }})
+ }
+ var err error
+ initialData, err = json.Marshal(initial)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(initialData,
tt.target))
+
+ common := metadataCommon(tt.target)
+ require.NotEmpty(t, common.Props)
+ require.NotEmpty(t, common.SnapshotList)
+ require.NotEmpty(t, common.SnapshotLog)
+ require.NotEmpty(t, common.MetadataLog)
+ require.NotEmpty(t, common.SnapshotRefs)
+ require.NotNil(t, common.CurrentSnapshotID)
+ require.NotNil(t, common.LastPartitionID)
+ require.NotEmpty(t, common.StatisticsList)
+ require.NotEmpty(t, common.PartitionStatsList)
+ switch metadata := tt.target.(type) {
+ case *metadataV2:
+ require.Equal(t, int64(34), metadata.LastSeqNum)
+ case *metadataV3:
+ require.Equal(t, int64(34), metadata.LastSeqNum)
+ }
+ if tt.name == "v3" {
+ require.NotEmpty(t, common.EncryptionKeyList)
+ }
+
+ 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",
+ "statistics", "partition-statistics",
"encryption-keys", "last-sequence-number",
+ } {
+ delete(reduced, key)
+ }
+ reduced["last-partition-id"] = json.RawMessage("1001")
+ reducedData, err := json.Marshal(reduced)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(reducedData,
tt.target))
+
+ common = metadataCommon(tt.target)
+ 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)
+ assert.Equal(t, 1001, *common.LastPartitionID)
+ assert.Empty(t, common.StatisticsList)
+ assert.Empty(t, common.PartitionStatsList)
+ assert.Empty(t, common.EncryptionKeyList)
+ // With no snapshots, an omitted last-sequence-number
retains the
+ // legacy -1 sentinel. This documents current behavior,
not the
+ // desired metadata normalization.
+ switch metadata := tt.target.(type) {
+ case *metadataV2:
+ assert.Equal(t, int64(-1), metadata.LastSeqNum)
+ case *metadataV3:
+ assert.Equal(t, int64(-1), metadata.LastSeqNum)
+ }
+ })
+ }
+}
+
+func metadataCommon(target any) *commonMetadata {
+ switch metadata := target.(type) {
+ case *metadataV1:
+ return &metadata.commonMetadata
+ case *metadataV2:
+ return &metadata.commonMetadata
+ case *metadataV3:
+ return &metadata.commonMetadata
+ default:
+ panic("unsupported metadata type")
+ }
+}
+
+func TestMetadataUnmarshalPreservesStateOnError(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ invalid string
+ target any
+ }{
+ {
+ name: "v1",
+ data: ExampleTableMetadataV1,
+ invalid: strings.Replace(ExampleTableMetadataV1,
`"current-snapshot-id": -1`, `"current-snapshot-id": 999`, 1),
+ target: &metadataV1{},
+ },
+ {
+ name: "v2",
+ data: ExampleTableMetadataV2,
+ invalid: strings.Replace(ExampleTableMetadataV2,
`"current-schema-id": 1`, `"current-schema-id": 99`, 1),
+ target: &metadataV2{},
+ },
+ {
+ name: "v3",
+ data: ExampleTableMetadataV3,
+ invalid: strings.Replace(ExampleTableMetadataV3,
`"current-schema-id": 1`, `"current-schema-id": 99`, 1),
+ target: &metadataV3{},
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
tt.target))
+ before, err := json.Marshal(tt.target)
+ require.NoError(t, err)
+
+ require.Error(t, json.Unmarshal([]byte(tt.invalid),
tt.target))
+
+ after, err := json.Marshal(tt.target)
+ require.NoError(t, err)
+ assert.Equal(t, before, after)
+ })
+ }
+}
+
+func TestMetadataV2UnmarshalRejectsMissingRequiredStateOnReuse(t *testing.T) {
+ var metadata metadataV2
+ require.NoError(t, json.Unmarshal([]byte(ExampleTableMetadataV2),
&metadata))
+
+ var reduced map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(ExampleTableMetadataV2),
&reduced))
+ delete(reduced, "default-spec-id")
+ reducedData, err := json.Marshal(reduced)
+ require.NoError(t, err)
+
+ require.Error(t, json.Unmarshal(reducedData, &metadata))
+ assert.Equal(t, 0, metadata.DefaultSpecID)
Review Comment:
This assertion doesn't discriminate what we want it to. The preserved
`DefaultSpecID` is 0, but 0 is also the Go zero value, so a hypothetical
"zero-fill the receiver on failure" regression would pass this too.
I'd seed the first decode with a non-zero `default-spec-id` and assert that
value here. The `SnapshotList` length check below is the strong one, so keep
that. (This test also overlaps `TestMetadataUnmarshalPreservesStateOnError` on
the state-preservation angle, so you could narrow it to just the missing-field
error if you'd rather not duplicate.)
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,187 @@ 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) {
+ initialData := []byte(tt.data)
+ var initial map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal(initialData,
&initial))
+ setJSON := func(key string, value any) {
+ encoded, err := json.Marshal(value)
+ require.NoError(t, err)
+ initial[key] = encoded
+ }
+ snapshotID := int64(1925)
+ if tt.name != "v1" {
+ snapshotID = 3055729675574597004
Review Comment:
This is the `current-snapshot-id` from `ExampleTableMetadataV2/V3` hardcoded
as a literal. If the fixture's snapshot id ever changes, the test keeps using
this value and fails as a confusing "snapshot not found" rather than pointing
at the fixture.
I'd pull it from the parsed fixture at setup or give it a named constant.
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,187 @@ 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) {
+ initialData := []byte(tt.data)
+ var initial map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal(initialData,
&initial))
+ setJSON := func(key string, value any) {
+ encoded, err := json.Marshal(value)
+ require.NoError(t, err)
+ initial[key] = encoded
+ }
+ snapshotID := int64(1925)
+ if tt.name != "v1" {
+ snapshotID = 3055729675574597004
+ }
+ setJSON("properties", map[string]any{"seed": "value"})
+ setJSON("current-snapshot-id", snapshotID)
+ setJSON("snapshot-log",
[]any{map[string]any{"snapshot-id": snapshotID, "timestamp-ms": int64(1)}})
+ setJSON("metadata-log",
[]any{map[string]any{"metadata-file": "s3://bucket/old.json", "timestamp-ms":
int64(1)}})
+ setJSON("refs", map[string]any{"main":
map[string]any{"snapshot-id": snapshotID, "type": "branch"}})
+ setJSON("last-partition-id", int64(1000))
+ setJSON("statistics", []any{map[string]any{
+ "snapshot-id": snapshotID, "statistics-path":
"s3://bucket/stats.puffin",
+ "file-size-in-bytes": int64(1),
"file-footer-size-in-bytes": int64(1), "blob-metadata": []any{},
+ }})
+ setJSON("partition-statistics", []any{map[string]any{
+ "snapshot-id": snapshotID, "statistics-path":
"s3://bucket/partition-stats.parquet", "file-size-in-bytes": int64(1),
+ }})
+ if tt.name == "v2" || tt.name == "v3" {
+ setJSON("last-sequence-number", int64(34))
+ }
+ if tt.name == "v3" {
+ setJSON("encryption-keys", []any{map[string]any{
+ "key-id": "key-1",
"encrypted-key-metadata": "YWJj",
+ }})
+ }
+ var err error
+ initialData, err = json.Marshal(initial)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(initialData,
tt.target))
+
+ common := metadataCommon(tt.target)
+ require.NotEmpty(t, common.Props)
+ require.NotEmpty(t, common.SnapshotList)
+ require.NotEmpty(t, common.SnapshotLog)
+ require.NotEmpty(t, common.MetadataLog)
+ require.NotEmpty(t, common.SnapshotRefs)
+ require.NotNil(t, common.CurrentSnapshotID)
+ require.NotNil(t, common.LastPartitionID)
+ require.NotEmpty(t, common.StatisticsList)
+ require.NotEmpty(t, common.PartitionStatsList)
+ switch metadata := tt.target.(type) {
+ case *metadataV2:
+ require.Equal(t, int64(34), metadata.LastSeqNum)
+ case *metadataV3:
+ require.Equal(t, int64(34), metadata.LastSeqNum)
+ }
+ if tt.name == "v3" {
+ require.NotEmpty(t, common.EncryptionKeyList)
+ }
+
+ 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",
+ "statistics", "partition-statistics",
"encryption-keys", "last-sequence-number",
+ } {
+ delete(reduced, key)
+ }
+ reduced["last-partition-id"] = json.RawMessage("1001")
+ reducedData, err := json.Marshal(reduced)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(reducedData,
tt.target))
+
+ common = metadataCommon(tt.target)
+ 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)
+ assert.Equal(t, 1001, *common.LastPartitionID)
+ assert.Empty(t, common.StatisticsList)
+ assert.Empty(t, common.PartitionStatsList)
+ assert.Empty(t, common.EncryptionKeyList)
+ // With no snapshots, an omitted last-sequence-number
retains the
+ // legacy -1 sentinel. This documents current behavior,
not the
+ // desired metadata normalization.
+ switch metadata := tt.target.(type) {
+ case *metadataV2:
+ assert.Equal(t, int64(-1), metadata.LastSeqNum)
+ case *metadataV3:
+ assert.Equal(t, int64(-1), metadata.LastSeqNum)
+ }
+ })
+ }
+}
+
+func metadataCommon(target any) *commonMetadata {
+ switch metadata := target.(type) {
+ case *metadataV1:
+ return &metadata.commonMetadata
+ case *metadataV2:
+ return &metadata.commonMetadata
+ case *metadataV3:
+ return &metadata.commonMetadata
+ default:
+ panic("unsupported metadata type")
+ }
+}
+
+func TestMetadataUnmarshalPreservesStateOnError(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ invalid string
+ target any
+ }{
+ {
+ name: "v1",
+ data: ExampleTableMetadataV1,
+ invalid: strings.Replace(ExampleTableMetadataV1,
`"current-snapshot-id": -1`, `"current-snapshot-id": 999`, 1),
+ target: &metadataV1{},
+ },
+ {
+ name: "v2",
+ data: ExampleTableMetadataV2,
+ invalid: strings.Replace(ExampleTableMetadataV2,
`"current-schema-id": 1`, `"current-schema-id": 99`, 1),
Review Comment:
All three invalid cases here are semantically invalid but syntactically
valid, so they only exercise the `validate()` error path, which runs after
`json.Unmarshal` succeeds. The earlier `json.Unmarshal` error path, the one
that returns before `*m = *next`, isn't covered by any case.
A fourth case per version with something like ``invalid: `not json {` ``
would exercise it in a line each.
##########
table/metadata_internal_test.go:
##########
@@ -324,6 +324,187 @@ 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) {
+ initialData := []byte(tt.data)
+ var initial map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal(initialData,
&initial))
+ setJSON := func(key string, value any) {
+ encoded, err := json.Marshal(value)
+ require.NoError(t, err)
+ initial[key] = encoded
+ }
+ snapshotID := int64(1925)
+ if tt.name != "v1" {
+ snapshotID = 3055729675574597004
+ }
+ setJSON("properties", map[string]any{"seed": "value"})
+ setJSON("current-snapshot-id", snapshotID)
+ setJSON("snapshot-log",
[]any{map[string]any{"snapshot-id": snapshotID, "timestamp-ms": int64(1)}})
+ setJSON("metadata-log",
[]any{map[string]any{"metadata-file": "s3://bucket/old.json", "timestamp-ms":
int64(1)}})
+ setJSON("refs", map[string]any{"main":
map[string]any{"snapshot-id": snapshotID, "type": "branch"}})
+ setJSON("last-partition-id", int64(1000))
+ setJSON("statistics", []any{map[string]any{
+ "snapshot-id": snapshotID, "statistics-path":
"s3://bucket/stats.puffin",
+ "file-size-in-bytes": int64(1),
"file-footer-size-in-bytes": int64(1), "blob-metadata": []any{},
+ }})
+ setJSON("partition-statistics", []any{map[string]any{
+ "snapshot-id": snapshotID, "statistics-path":
"s3://bucket/partition-stats.parquet", "file-size-in-bytes": int64(1),
+ }})
+ if tt.name == "v2" || tt.name == "v3" {
+ setJSON("last-sequence-number", int64(34))
+ }
+ if tt.name == "v3" {
+ setJSON("encryption-keys", []any{map[string]any{
+ "key-id": "key-1",
"encrypted-key-metadata": "YWJj",
+ }})
+ }
+ var err error
+ initialData, err = json.Marshal(initial)
+ require.NoError(t, err)
+
+ require.NoError(t, json.Unmarshal(initialData,
tt.target))
+
+ common := metadataCommon(tt.target)
+ require.NotEmpty(t, common.Props)
+ require.NotEmpty(t, common.SnapshotList)
+ require.NotEmpty(t, common.SnapshotLog)
+ require.NotEmpty(t, common.MetadataLog)
+ require.NotEmpty(t, common.SnapshotRefs)
+ require.NotNil(t, common.CurrentSnapshotID)
+ require.NotNil(t, common.LastPartitionID)
+ require.NotEmpty(t, common.StatisticsList)
+ require.NotEmpty(t, common.PartitionStatsList)
+ switch metadata := tt.target.(type) {
Review Comment:
I think both of these type switches will trip the exhaustive linter we run
in CI. They cover `*metadataV2` and `*metadataV3` but there's no `case
*metadataV1` and no `default`, and the second switch further down (line 427)
has the same gap.
Adding `case *metadataV1:` to both keeps CI green, and it means the test
won't silently skip v1 if that type ever grows a `LastSeqNum`-like field.
--
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]
