technicolorbeat commented on PR #1956:
URL: https://github.com/apache/iceberg-go/pull/1956#issuecomment-5515330804
> All three of my prior blockers are fixed (DCO on every commit, merge
commit gone, branch is `MERGEABLE`), and the mechanism itself is sound. I want
to call out the test suite specifically: I reverted each of the eight defensive
changes in turn and **every one was caught by a named test** — no inert
assertions, no tautologies. That is the strongest test suite I've reviewed in
this batch.
>
> Two substantive problems remain.
>
> ## Blocking
> **1. `table/deferred_snapshots.go:213-215` and `:171-173` — the deferred
parser rejects metadata the eager parser accepts.**
>
> `validateSummaryObject` and `validateStringArray` are hand-rolled
stand-ins for what `encoding/json` does when decoding into `map[string]string`
/ `[]string`, and they diverge on JSON `null`. Go's decoder stores `null` into
a string as `""` without error; both validators require the value token to
start with `"`. Minimal reproducers — both parse clean today, both fail with
`snapshot-loading-mode=refs`:
>
> ```
> "summary": {"operation":"append","extra":null}
> eager=<nil> defer=invalid metadata: snapshot …: cannot unmarshal
summary value into string
>
> "manifests": ["a.avro", null]
> eager=<nil> defer=invalid metadata: snapshot …: cannot unmarshal
manifest location into string
> ```
>
> Why this matters more than a normal parse-strictness difference: it fires
on the _commit response_, i.e. **after the server has already applied the
commit**. `Transaction.Commit` would return `ErrInvalidMetadata` for a
transaction that actually succeeded, and the natural user reaction — retry —
risks a double-apply. It also directly contradicts the PR's claim that "the
public `Metadata` API and v1/v2/v3 behavior are unchanged": enabling a
_performance_ mode must not change what parses.
>
> Fix: in `validateSummaryObject`, accept a `null` value token
(`bytes.HasPrefix(raw[i:], []byte("null"))` → advance 4 and continue), and
likewise for elements in `validateStringArray`; add a parity test asserting
both documents above still parse under the deferred path. A lower-risk
alternative is to validate by decoding into `map[string]string`/`[]string` for
unreferenced entries — you lose part of the allocation win but get parity for
free.
>
> (A third, much more obscure instance of the same class:
`"summary":{"operation":"","operation":"append"}` — Go's last-key-wins map
decode accepts it, the scanner rejects on the first key. Not worth fixing on
its own, but the null fix should come with a comment stating the intended
acceptance contract.)
>
> **2. `table/metadata.go:425-429` + `table/table.go:210` — the realistic
post-commit path is an unmeasured ~21% allocation regression.**
>
> `Table.NewTransaction*` calls `MetadataBuilderFromBase`, which calls
`snapshotsForMarshal()` and fully materialises. So the _second_ write on a
committed table pays the deferred index build **and** the full decode. Measured
(parse + `MetadataBuilderFromBase`, `-count=3`):
>
> metadata bytes eager allocs/op deferred allocs/op Δ eager
B/op deferred B/op Δ
> 7,056 906 1,096 **+21.0%** 58,952 77,272 **+31.1%**
> 59,078 6,132 7,405 **+20.8%** 419,130 582,586 **+39.0%**
> 582,880 58,345 70,424 **+20.7%** 3,953,508 5,547,678
**+40.3%**
> 1,999,830 198,613 239,713 **+20.7%** 13,850,244 19,212,749
**+38.7%**
> 5,856,882 580,444 700,558 **+20.7%** 41,413,480 57,358,112
**+38.5%**
> At 5.86 MB the deferred side was also consistently slower in wall time
(256/278/293 ms vs 213/227/239 ms).
>
> Your own `BenchmarkStageDeferredAllSnapshots` already contains this
evidence — at ~2 MB it reports 239,675 allocs / 19.07 MB versus
`BenchmarkStageFullParse`'s 174,658 allocs / 10.43 MB — but it's only described
as "the intentional fallback … pay the deferred full-decoding cost once" and is
never put next to the eager baseline. That reads as "you pay it once" rather
than "it is strictly worse than not deferring".
>
> Please either (a) add this parse→`NewTransaction` pair to the benchmark
set and state the trade-off in the description and in the doc comment on
`ParseMetadataBytesDeferredSnapshots`, or (b) carry the deferred state into
`MetadataBuilder` so a builder that never touches history doesn't materialise.
I don't think (b) is required for this PR, but (a) is — as written, a reader
would conclude this is a strict improvement for REST commit traffic, and for
any table that gets more than one write per `Table` object it isn't.
>
> ## Major
> **3. `table/metadata.go:3071`,`:3169`,`:3289` — marshalling a metadata
_value_ silently drops unreferenced snapshots.** The three `MarshalJSON`
methods that project `snapshotsForMarshal()` have **pointer receivers**, so
`json.Marshal` on a non-addressable `metadataV2` value skips them and falls
back to reflection over `commonMetadata.SnapshotList` — which under deferral
holds only the ref-reachable subset. Measured on the example v2 document with
one unreferenced snapshot: `json.Marshal(ptr)` → **2** snapshots,
`json.Marshal(*ptr)` → **1**, no error.
>
> Every production write path is safe today (`catalog/internal/utils.go:72`
encodes the interface, which holds a pointer; `metadataV1.ToV2()` returns a
value but clears `deferredSnapshots` first), so this is latent — I could not
find a live call site that trips it. But this PR is what turns the consequence
into _silent snapshot loss in a written metadata file_ rather than a missing
`current-snapshot-id`, and the type does return by value (`ToV2`). Fix is three
characters: value receivers (`func (m metadataV2) MarshalJSON()`), which puts
the method in both method sets — `Snapshot.MarshalJSON` at `snapshots.go:306`
already uses that form. Please add a regression test asserting
`json.Marshal(*meta.(*metadataV2))` preserves the full history.
>
> **4. `table/metadata.go:2384-2390`,`:2372-2378` — a deferred decode error
is swallowed and presented as "no snapshots".** `allSnapshots()` discards the
error and returns `nil`, so `Snapshots()` reports an empty history and
`Equals()` compares two empty lists as equal; `SnapshotByID` returns `nil`,
indistinguishable from absent. The two callers that _can_ return an error
(`MarshalJSON`, `MetadataBuilderFromBase`) do propagate it, which is the right
half.
>
> I tried hard to reach the swallowed path and couldn't —
`deferredSnapshotFields` is field-for-field type-identical to `Snapshot` except
`manifests`/`summary`, the validators are equal-or-stricter than
`encoding/json` (20,000-case fuzz found 0 eager-stricter), and `encoding/json`
has syntax-checked the document before `splitJSONArray` runs. So it isn't
currently reachable. But it's precisely the "not yet decoded treated as absent"
failure mode, and the invariant keeping it unreachable is exactly the one
Blocking 1 shows isn't being maintained deliberately. At minimum add a comment
at `:2384` recording that `snapshotsForMarshal` cannot fail for state produced
by `prepareDeferredSnapshots`, and why.
>
> ## What checks out
> * **8/8 mutations killed** — reverting the head commit, moving
`s.mu.RUnlock()` before `&s.entries[i]` (real data race at
`deferred_snapshots.go:108` under `-race -count=5`), deleting each
`MarshalJSON`, stripping `ToV2` materialisation, `Equals` → `c.SnapshotList`,
`Snapshots()` → `c.SnapshotList`, stripping the v3 `snapshots` projection,
stripping the deferred branch from `SnapshotByID`. Each caught by a named test.
> * **Differential fuzz, 20,000 generated v2 documents**: 3,697 divergences,
**all in the deferred-stricter direction, 0 eager-stricter**. For all 16,303
mutually-accepted documents, deferred and eager marshalled **byte-identically**
while still deferred, and `SnapshotByID` on an unreferenced snapshot returned a
value `reflect.DeepEqual` to the eager one.
> * **Enumerated every non-test reader** of `commonMetadata.SnapshotList`
between response decode and final metadata (`metadata.go:451`, `:1403`,
`:2364-2370`, `:2736`, `:2868`, `:3187`, `:3314`, `:3338`, `:3090`). All are
either safe by construction — the current snapshot and every ref target are
eagerly decoded, and `deferredSnapshots` is assigned _after_
`finishDeferredMetadataUnmarshal`, so `checkRefsExist` can't see a
half-populated state — or duplicated synchronously in
`prepareDeferredSnapshots`. Every public reader in the repo goes through
`Snapshots()`/`SnapshotByID`, which materialise. One hole found (Major 3).
> * **Benchmarks reproduce.** At ~2 MB I measure 41,409 vs 174,658 allocs/op
against your 41,503 vs 174,749 — a match to <0.1%. Historical-lookup delta
41,460 vs 41,407 (+53) against your +52.
> * `golangci-lint` 0 issues; `-race` clean on both touched packages; CI
15/15.
>
> ## Minor
> * The new `MarshalJSON` wrappers move the top-level `snapshots` key to the
**end** of the serialized object for v1, v2 _and_ v3, in **all**
snapshot-loading modes, because Go orders fields by index sequence and the
explicit `SnapshotList` field follows the embedded `*Alias`. Semantically
irrelevant, but it changes every metadata file this client writes, and
"v1/v2/v3 behavior are unchanged" doesn't cover it.
> * `metadata.go:1953-1974` — `ParseMetadataBytesDeferredSnapshots`
duplicates the preflight of `ParseMetadataBytes` almost verbatim; factor out
the shared step. It also drops the `ret.Version() != formatVersion` cross-check
the eager path does at `:1942-1945`. I couldn't construct an input where they
disagree, so this is a consistency nit.
> * `metadata.go:450-453` — the current-snapshot presence check uses
`common.snapshotIndex, common.SnapshotList` (the eager subset) even though
`b.snapshotIndex`/`b.snapshotList` were built two lines earlier. Correct today
only because the current snapshot is always eagerly decoded; using `b.*`
removes the dependency.
> * `deferred_snapshots.go:279` — `splitJSONArray` re-trims a slice its only
caller already trimmed, and returns offsets the caller applies to _its_ slice.
Works only because the second trim is a no-op; a future caller passing
untrimmed bytes gets silently wrong spans. (I stress-tested the scanner on
braces/commas/brackets inside strings, escaped quotes and backslashes, nested
structures, empty and whitespace-only arrays, and a 501-element array — all
correct.)
> * Commit hygiene: the builder assertion at
`deferred_snapshots_test.go:87-100` was present from the first commit, and
reverting only the head commit's single hunk makes it fail — so `go test
./table/` was **red at both `d704277d` and `187c473b`**. Bisect-hostile; please
squash or fix in place.
> * ASF headers present on both new files; range-over-int used correctly
(the index loops in the scanners are genuine index arithmetic and correctly not
converted).
>
> ## Prior items
> * DCO missing on 3 of 4 commits → **Fixed**, all three commits signed off.
> * Merge commit `d6986d2` → **Fixed**, history is linear.
> * `mergeStateStatus: DIRTY` → **Fixed**, now `MERGEABLE`.
> * Mode resolution / unknown values rejected → **still correct, N.A.**
> * Per-entry `sync.Once`, never a panic → **Confirmed and now stronger**:
reverting `187c473b`'s lock ordering produces a real race the suite catches.
> * "No observable loss; `Snapshots()` fully materialises" → **Partially
fixed.** True for the interface path, and the head commit closed the
`MetadataBuilderFromBase` leak my prior review missed. But the value-marshal
path does observably lose snapshots (Major 3), and `Snapshots()` returns nil
rather than an error on a deferred decode failure (Major 4).
> * Unknown snapshot fields round-trip → **Confirmed.** Fuzz cases carrying
unknown nested objects round-trip byte-identically on both paths.
> * Aliasing / `json.RawMessage` ownership → **Confirmed, unchanged.**
> * "The lazy-decode design is sound, no findings against the mechanism" →
**partially superseded.** The mechanism is sound; that round didn't examine
validation parity between the two parsers, nor benchmark the
parse→`NewTransaction` sequence. Those are Blocking 1 and 2.
>
> ## Description
> * **"The public `Metadata` API and v1/v2/v3 behavior are unchanged" is
inaccurate twice** — the deferred path rejects documents the eager path
accepts, and serialized top-level key order changes for all three versions in
all modes.
> * "Synchronously validate … summary values. Historical corruption
therefore remains a parse-time error" — true, but the validation isn't
_equivalent_ to the eager path's; it's strictly stricter on `null`.
> * "Callers that request all snapshots pay the deferred full-decoding cost
once" — understated. Full materialisation costs **+37% allocations and +83%
bytes** versus never deferring, from your own benchmarks, and it's triggered by
`Table.NewTransaction`, not only by explicit history access.
> * Benchmark revisions are honestly disclosed as stale relative to head.
Allocation figures still reproduce to <0.1%, so the substance holds. Two don't:
the claimed **38.2% improvement at 7,056 B** isn't statistically
distinguishable on my box (290.0 µs vs 218.5 µs, p=0.123 over 10 interleaved
rounds) — allocations at that size are unambiguously better (−41%), so I'd lead
with allocations at the small end rather than a wall-clock percentage. And
**−25.2% B/op at ~2 MB** now measures **−29.9%**, because main's baseline
moved. Worth re-measuring on the rebased head.
>
> > _This review was drafted by an AI-assisted tool and confirmed by an
Iceberg Go maintainer. The findings cite the project's review criteria; if you
think one is mis-applied, please reply and a maintainer will weigh in._
Thanks for the detailed review and for validating the test suite so
thoroughly. I reproduced the findings and pushed the follow-up changes.
At a high level, I aligned deferred parsing with eager JSON semantics,
ensured full snapshot history is preserved across serialization, documented the
deferred-state invariants, and added regression and parse-to-builder benchmark
coverage.
The updated benchmarks make the trade-off explicit. At ~2 MB, deferred
commit-response parsing improves from **78.420 ms to 50.326 ms**, with **76.3%
fewer allocations** and **29.9% fewer allocated bytes**. However, immediately
creating a `MetadataBuilder` forces full materialization: parse-plus-builder
increases from **80.611 ms to 98.314 ms**, with **20.7% more allocations** and
**38.9% more allocated bytes**.
I agree this is not a strict improvement for repeated writes on the same
returned `Table`. This PR optimizes the `snapshot-loading-mode=refs`
commit-response path when full history is not immediately required. Carrying
deferred state into `MetadataBuilder` would be a separate follow-up - #1986
--
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]
