laskoviymishka commented on code in PR #1910:
URL: https://github.com/apache/iceberg-go/pull/1910#discussion_r3892709157
##########
schema_test.go:
##########
@@ -2293,3 +2294,48 @@ func
TestVisitGeoSchemaWithSchemaVisitorPerPrimitiveType(t *testing.T) {
assert.Equal(t, 1, v.geometryCalls)
assert.Equal(t, 1, v.geographyCalls)
}
+
+func TestSchemaMarshalJSONConcurrentLazyLookups(t *testing.T) {
+ for range 32 {
+ schema := iceberg.NewSchemaWithIdentifiers(17, nil,
Review Comment:
The test always builds with `nil` identifier IDs, so the non-nil `ids`
branch in `MarshalJSON` never actually gets raced. I'd add a parallel case with
something like `[]int{1}` and assert the encoded JSON carries
`"identifier-field-ids":[1]`.
Related: the closing `assert.Nil(t, schema.IdentifierFieldIDs)` passes on
both old and new code (the old copy mutated `aliasCopy`, never `*s`), so it
doesn't actually guard this fix; `-race` is what catches the regression. I'd
either drop it or leave a one-line note that the real invariant is that
marshaling mustn't mutate the receiver. wdyt?
##########
schema.go:
##########
@@ -346,8 +346,7 @@ func (s *Schema) MarshalJSON() ([]byte, error) {
type Alias Schema
- aliasCopy := *(*Alias)(s)
- aliasCopy.IdentifierFieldIDs = ids
+ aliasCopy := Alias{ID: s.ID, IdentifierFieldIDs: ids}
Review Comment:
This is the right fix. The old `*(*Alias)(s)` copied the five
`atomic.Pointer` cache fields by value, which is the copy-after-use case `go
vet` flags, and the selective literal keeps the JSON byte-identical (only
`schema-id` and `identifier-field-ids` are exported off `Alias`, and the
nil-to-`[]int{}` coercion is preserved).
The one thing I'd guard: the named literal is point-in-time. If a new
exported JSON-tagged field gets added to `Schema` later, marshaling silently
zeroes it, with no compiler error and no failing test. I'd drop a one-line
comment above it noting only `ID` and `IdentifierFieldIDs` are exported here,
so anyone adding a field knows to list it. Non-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]
