tanmayrauth commented on code in PR #1591:
URL: https://github.com/apache/iceberg-go/pull/1591#discussion_r3678294266
##########
table/metadata.go:
##########
@@ -1648,6 +1648,20 @@ func initCommonMetadataForDeserialization()
commonMetadata {
}
}
+func requireLastUpdatedMS(data []byte) error {
Review Comment:
This re-parses the whole metadata document into map[string]json.RawMessage,
but ParseMetadataBytes already builds exactly that map in
assignMissingPartitionFieldIDs (metadata.go:1519) two steps before the struct
unmarshal — so on the main load path the document now gets fully parsed into
the same map type three times (once here per version). Beyond the redundant
parse, this scatters the presence invariant across all three UnmarshalJSON
methods. Consider asserting presence once in ParseMetadataBytes, reusing the
map that assignMissingPartitionFieldIDs already has, so there's a single check
for all format versions and no extra parse.
##########
table/metadata.go:
##########
@@ -2422,6 +2433,10 @@ func (m *metadataV1) preValidate() {
}
func (m *metadataV1) UnmarshalJSON(b []byte) error {
+ if err := requireLastUpdatedMS(b); err != nil {
Review Comment:
Related to the above: this PR also removes the last-updated-ms case from
commonMetadata.validate(), which is the shared invariant checker for both the
JSON and builder paths. After this change validate() no longer asserts the
field is set at all — the builder path is safe only because buildCommonMetadata
forces
time.Now() when lastUpdatedMS is 0 (metadata.go:1010). So the "must be
present" invariant now lives incidentally in two spots (this unmarshal hook +
the builder default) rather than in validate(). If the check moves into
ParseMetadataBytes per the note above, that's fine; the thing to avoid is
leaving validate()
with no last-updated-ms assertion while the presence guarantee depends on
two unrelated code paths.
--
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]
