tanmayrauth commented on code in PR #1525:
URL: https://github.com/apache/iceberg-go/pull/1525#discussion_r3650566606
##########
view/metadata.go:
##########
@@ -450,6 +454,32 @@ func (m *metadata) validate() error {
return nil
}
+func (m *metadata) checkSchemaAndVersionEntries() error {
Review Comment:
This fixes the view path, but the identical latent panic is still live on
the table metadata read path: commonMetadata.checkSchemas()
(table/metadata.go:2056) ranges over SchemaList and dereferences s.ID with no
nil guard, and it runs during ParseMetadataBytes — so a table metadata doc with
"schemas":[null] panics with a nil-pointer deref instead of returning
ErrInvalidMetadata (confirmed locally). Table metadata also never rejects
duplicate schema-ids, so it silently last-wins through the same indexBy
pattern this PR is fixing for views, whereas Java throws for both
(versionsById()/schemasById() use ImmutableMap.builder()). Not asking to expand
this PR — but a follow-up applying the same null + duplicate guard to
checkSchemas (and the partition-spec/sort-order equivalents) would keep the two
metadata paths from diverging.
##########
view/metadata.go:
##########
Review Comment:
Minor: now that checkSchemaAndVersionEntries runs first and guarantees
non-nil, unique schema ids, this function rebuilds a schema-id set that init()
already materializes as lazySchemasByID(). Could drop the local map and use
m.lazySchemasByID() to avoid a third pass over SchemaList. Cosmetic, not
blocking.
--
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]
