laskoviymishka commented on code in PR #1591:
URL: https://github.com/apache/iceberg-go/pull/1591#discussion_r3681135799
##########
table/metadata.go:
##########
@@ -1648,6 +1651,15 @@ func initCommonMetadataForDeserialization()
commonMetadata {
}
}
+func requireLastUpdatedMS(fields map[string]json.RawMessage) error {
Review Comment:
this bare `return err` means a malformed-JSON blob comes back unwrapped, so
`errors.Is(err, ErrInvalidMetadata)` is false for broken JSON but true for the
null/missing case right below. Since this is a fresh call site I'd wrap it with
`fmt.Errorf("%w: %w", ErrInvalidMetadata, err)` so callers get a consistent
sentinel.
##########
table/metadata.go:
##########
@@ -1648,6 +1651,15 @@ func initCommonMetadataForDeserialization()
commonMetadata {
}
}
+func requireLastUpdatedMS(fields map[string]json.RawMessage) error {
+ value, ok := fields["last-updated-ms"]
+ if !ok || bytes.Equal(bytes.TrimSpace(value), []byte("null")) {
+ return fmt.Errorf("%w: missing last-updated-ms",
ErrInvalidMetadata)
+ }
Review Comment:
the `bytes.Equal(bytes.TrimSpace(value), []byte("null"))` is doing more work
than it needs to. A `json.RawMessage` coming out of a
`map[string]json.RawMessage` is already whitespace-trimmed by the decoder, so
`string(value) == "null"` is equivalent and lets us drop the new `bytes` import
entirely.
```suggestion
if !ok || string(value) == "null" {
```
##########
table/metadata.go:
##########
@@ -1648,6 +1651,15 @@ func initCommonMetadataForDeserialization()
commonMetadata {
}
}
+func requireLastUpdatedMS(fields map[string]json.RawMessage) error {
+ value, ok := fields["last-updated-ms"]
+ if !ok || bytes.Equal(bytes.TrimSpace(value), []byte("null")) {
+ return fmt.Errorf("%w: missing last-updated-ms",
ErrInvalidMetadata)
+ }
+
Review Comment:
small thing: this says "missing" but it also fires for an explicit
`"last-updated-ms": null`, and the test asserts that exact substring for the
null case, so the wording gets locked in. Something like `"last-updated-ms is
absent or null"` (or just `"required"`) would be accurate for both paths.
##########
table/metadata_internal_test.go:
##########
@@ -310,6 +310,69 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+func TestLastUpdatedMSPresence(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ }{
+ {name: "v1", data: ExampleTableMetadataV1},
+ {name: "v2", data: ExampleTableMetadataV2},
+ {name: "v3", data: ExampleTableMetadataV3},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var metadata map[string]any
Review Comment:
this map is declared once per version and then mutated in place by each
inner subtest (missing/null/epoch/negative), so the cases only pass because
they run in order. A `t.Parallel()` or `-shuffle` would have them stepping on
each other. I'd `maps.Clone(metadata)` into a local at the top of each subtest
so they're independent.
##########
table/metadata_internal_test.go:
##########
@@ -310,6 +310,69 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+func TestLastUpdatedMSPresence(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ }{
+ {name: "v1", data: ExampleTableMetadataV1},
+ {name: "v2", data: ExampleTableMetadataV2},
+ {name: "v3", data: ExampleTableMetadataV3},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ var metadata map[string]any
+ require.NoError(t, json.Unmarshal([]byte(tt.data),
&metadata))
+
+ for _, key := range []string{
+ "snapshots", "snapshot-log", "metadata-log",
"current-snapshot-id",
+ "refs", "statistics", "partition-statistics",
+ } {
+ delete(metadata, key)
+ }
+
+ t.Run("missing", func(t *testing.T) {
+ delete(metadata, "last-updated-ms")
+ assertMissingLastUpdatedMS(t, metadata)
+ })
+
+ t.Run("null", func(t *testing.T) {
+ metadata["last-updated-ms"] = nil
+ assertMissingLastUpdatedMS(t, metadata)
+ })
+
+ t.Run("epoch", func(t *testing.T) {
+ metadata["last-updated-ms"] = float64(0)
+ raw, err := json.Marshal(metadata)
+ require.NoError(t, err)
+ parsed, err := ParseMetadataBytes(raw)
+ require.NoError(t, err)
+ assert.Zero(t, parsed.LastUpdatedMillis())
+ })
+
+ t.Run("negative", func(t *testing.T) {
+ metadata["last-updated-ms"] = float64(-1)
+ raw, err := json.Marshal(metadata)
+ require.NoError(t, err)
+ parsed, err := ParseMetadataBytes(raw)
+ require.NoError(t, err)
+ assert.Equal(t, int64(-1),
parsed.LastUpdatedMillis())
Review Comment:
the matrix covers missing/null/epoch/negative but not a
present-but-wrong-type value like `"last-updated-ms": "2024-01-01"`. That case
slips past `requireLastUpdatedMS` (present and non-null) and instead fails
later with a `json.UnmarshalTypeError`, which isn't wrapped as
`ErrInvalidMetadata`, so `errors.Is` holds for null but not for a wrong type.
Worth a subtest that pins down which error we expect there. 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]
