zeroshade commented on PR #1648:
URL: https://github.com/apache/iceberg-go/pull/1648#issuecomment-5219738561
Re-reviewed after `95a0d56` and `1efe8e2`. These pick up six of the seven
notes from my last pass, and several of them go further than I asked — seeding
the fixture with the previously-absent optional fields is a better fix for the
vacuous-v1 problem than the guard assertions I suggested, and overriding
`last-partition-id` to 1001 rather than deleting it tests replacement instead
of just absence. Thanks for working through all of them.
For the record, what landed:
- `ParseMetadataBytes` now builds `&metadataV1{}` / `&metadataV2{}` /
`&metadataV3{}` (`table/metadata.go:1514-1518`).
- The three dead `aux.LastColumnId = -1` assignments are gone, with the
explanation moved to `initCommonMetadataForDeserialization`
(`table/metadata.go:1662`) where the value originates.
- The delete-list gained `statistics`, `partition-statistics`,
`encryption-keys`, and `last-sequence-number`.
- Failure-path coverage is table-driven across all three versions.
- The failure-path assertion is now a `json.Marshal` before/after byte
comparison.
Two things I found this round, both cheap, and then one wording item that is
still outstanding.
**1. `TestMetadataUnmarshalPreservesStateOnError/v1` does not currently
guard anything.**
I checked this by reverting all three `UnmarshalJSON`s to decode into the
receiver via `aux := (*Alias)(m)`. The v2 and v3 subtests fail, as they should.
The v1 subtest passes.
The reason is that the invalid v1 document differs from the valid one only
inside `"schema"`, and the error it raises — `invalid schema: multiple fields
for id 1: x and y` — comes out of `json.Unmarshal` itself. Every field the
decoder writes before reaching that point is byte-identical to what the
receiver already holds, so `before == after` whether or not the fix is present.
This matters because v1 is the version with the largest partial-mutation
surface: `metadataV1.preValidate` (`table/metadata.go:2432-2464`) mutates
`SchemaList`, `Specs`, `DefaultSpecID`, `LastPartitionID`, and `SortOrderList`
before validation runs.
The fix is to perturb a top-level scalar so a partial write is observable. I
verified two candidates — both fail with the fix reverted and pass with it in
place:
- `"current-snapshot-id": -1` → `999`, which fails with `invalid metadata:
snapshot ref main with ID 999 does not exist in snapshot list`. This is the
direct parallel to what the v2 and v3 subtests already do.
- `"last-column-id": 3` → `-5`, which fails with `invalid metadata: missing
last-column-id`.
Either works; the first keeps the three subtests symmetric.
**2. The new `assert.Equal(t, int64(-1), metadata.LastSeqNum)` pins a
pre-existing bug as expected behavior.**
I raised this leak last time as non-blocking, and it is genuinely
pre-existing rather than anything this PR introduces — I confirmed that on
`main`. But it is now asserted, which changes its status: a future fix to the
sentinel normalization will break this line and read as a regression.
To be concrete about what is being pinned: for a v2 document that omits
`last-sequence-number` and has no snapshots, `checkLastSequenceNumber`
(`table/metadata.go:2709`) only iterates the snapshot list, so with zero
snapshots nothing checks it. On `main` today, `ParseMetadataBytes` on such a
document returns successfully, `LastSequenceNumber()` reports `-1`, and
re-serializing emits `"last-sequence-number": -1` — an invalid v2 document,
through the public API.
A short comment on the assertion noting that `-1` is the un-normalized
sentinel and not the desired end state would be enough. Happy to file the
underlying issue separately.
**3. Still outstanding from last time: the description.**
`ccc5e4e` has no commit body, so the wording lives only in the PR
description, which still reads "Existing version-specific defaults and
sentinels are preserved". They are now applied *unconditionally*, which makes
direct `UnmarshalJSON` strictly more rejecting — `default-spec-id` becomes
required for v2 and `next-row-id` for v3, matching what `ParseMetadataBytes`
already enforced. That is the right behavior and unobservable externally since
`metadataV{1,2,3}` are unexported, but it is a tightening rather than a
preservation, and the description is what carries into the record on squash.
Nothing here touches the production change, which I verified last pass and
which is unchanged. On my side: `go test ./table/... ./ -count=1` green, `gofmt
-l table/` and `go vet ./table/...` clean, all 15 CI checks passing.
Happy to approve once the v1 fixture is swapped and the description is
updated.
--
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]
