zeroshade commented on code in PR #1956: URL: https://github.com/apache/iceberg-go/pull/1956#discussion_r3936149142
########## catalog/rest/lazy_snapshot_bench_test.go: ########## @@ -0,0 +1,229 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "encoding/json" + "fmt" + "runtime" + "testing" + + "github.com/apache/iceberg-go/table" + "github.com/stretchr/testify/require" +) + +// These counts generate metadata payloads of approximately 7 KiB, 58 KiB, +// 569 KiB, 2 MB, and 5.6 MiB. Benchmark names and metrics report actual bytes. +var benchSnapshotCounts = []int64{10, 100, 1000, 3418, 10000} + +func extractMetadata(tb testing.TB, body []byte) json.RawMessage { + tb.Helper() + var top map[string]json.RawMessage + require.NoError(tb, json.Unmarshal(body, &top)) + + return top["metadata"] +} + +func metadataBenchmarkName(raw json.RawMessage) string { + return fmt.Sprintf("metadata=%d-bytes", len(raw)) +} + +// BenchmarkStageFullParse measures table.ParseMetadataBytes, the call the REST client +// actually makes on both LoadTable and CommitTable responses. +func BenchmarkStageFullParse(b *testing.B) { + for _, n := range benchSnapshotCounts { + body := makeTableResponseWithSnapshots(n) + rawMeta := extractMetadata(b, body) + b.Run(metadataBenchmarkName(rawMeta), func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + b.ReportMetric(float64(len(rawMeta)), "metadata-bytes") + b.ReportMetric(float64(n), "snapshots") + for range b.N { + meta, err := table.ParseMetadataBytes(rawMeta) + if err != nil { + b.Fatal(err) + } + if meta.CurrentSnapshot() == nil { + b.Fatal("missing current snapshot") + } + } + }) + } +} + +// BenchmarkStageDeferredParse measures the parse-time lazy materialization +// path used for CommitTable responses when snapshot-loading-mode is refs. +func BenchmarkStageDeferredParse(b *testing.B) { + for _, n := range benchSnapshotCounts { + body := makeTableResponseWithSnapshots(n) + rawMeta := extractMetadata(b, body) + b.Run(metadataBenchmarkName(rawMeta), func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + b.ReportMetric(float64(len(rawMeta)), "metadata-bytes") + b.ReportMetric(float64(n), "snapshots") + for range b.N { + meta, err := table.ParseMetadataBytesDeferredSnapshots(rawMeta) + if err != nil { + b.Fatal(err) + } + if meta.CurrentSnapshot() == nil { + b.Fatal("missing current snapshot") + } + } + }) + } +} + +// BenchmarkStageDeferredHistoricalLookup measures parsing followed by a lookup +// of one unreferenced historical snapshot. The indexed lazy path should decode +// only that snapshot rather than materializing the complete history. +func BenchmarkStageDeferredHistoricalLookup(b *testing.B) { + for _, n := range benchSnapshotCounts { + body := makeTableResponseWithSnapshots(n) + rawMeta := extractMetadata(b, body) + b.Run(metadataBenchmarkName(rawMeta), func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + b.ReportMetric(float64(len(rawMeta)), "metadata-bytes") + b.ReportMetric(float64(n), "snapshots") + for range b.N { + meta, err := table.ParseMetadataBytesDeferredSnapshots(rawMeta) + if err != nil { + b.Fatal(err) + } + if meta.SnapshotByID(0) == nil { + b.Fatal("missing historical snapshot") + } + } + }) + } +} + +// BenchmarkStageDeferredAllSnapshots measures the intentional fallback that +// parses and then materializes the complete snapshot history. +func BenchmarkStageDeferredAllSnapshots(b *testing.B) { + for _, n := range benchSnapshotCounts { + body := makeTableResponseWithSnapshots(n) + rawMeta := extractMetadata(b, body) + b.Run(metadataBenchmarkName(rawMeta), func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + b.ReportMetric(float64(len(rawMeta)), "metadata-bytes") + b.ReportMetric(float64(n), "snapshots") + for range b.N { + meta, err := table.ParseMetadataBytesDeferredSnapshots(rawMeta) + if err != nil { + b.Fatal(err) + } + if len(meta.Snapshots()) != int(n) { + b.Fatal("incomplete snapshot history") + } + } + }) + } +} + +// BenchmarkStageParseThenMetadataBuilder measures the path taken when a caller +// starts another transaction from the Table returned by Commit. Builder +// creation requires complete snapshot history, so this exposes the cost of +// indexing deferred snapshots and then fully materializing them. +func BenchmarkStageParseThenMetadataBuilder(b *testing.B) { + for _, n := range benchSnapshotCounts { + body := makeTableResponseWithSnapshots(n) + rawMeta := extractMetadata(b, body) + for _, mode := range []struct { + name string + parse func([]byte) (table.Metadata, error) + }{ + {name: "eager", parse: table.ParseMetadataBytes}, + {name: "deferred", parse: table.ParseMetadataBytesDeferredSnapshots}, + } { + b.Run(metadataBenchmarkName(rawMeta)+"/"+mode.name, func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + b.ReportMetric(float64(len(rawMeta)), "metadata-bytes") + b.ReportMetric(float64(n), "snapshots") + for range b.N { + meta, err := mode.parse(rawMeta) + if err != nil { + b.Fatal(err) + } + builder, err := table.MetadataBuilderFromBase(meta, "") + if err != nil { + b.Fatal(err) + } + if builder == nil { + b.Fatal("missing metadata builder") + } + } + }) + } + } +} + +// BenchmarkStageDeferredRetainedHeap measures the live heap held by one +// deferred metadata object before and after full snapshot materialization. +// Run with -benchtime=1x: retained-heap measurements describe one object and +// cannot be meaningfully averaged across b.N independent lifecycles. +func BenchmarkStageDeferredRetainedHeap(b *testing.B) { + if b.N != 1 { + b.Skip("retained-heap benchmark requires -benchtime=1x") Review Comment: **nit** — Retained-heap benchmark skips unless -benchtime=1x BenchmarkStageDeferredRetainedHeap b.Skip()s under a default `go test -bench` invocation and reports a synthetic ReportMetric(0, "ns/op"). It is legitimate (it produced the retained-heap table in the description) but is an unusual benchmark shape that will read as dead to anyone running the suite normally; a one-line comment already explains the -benchtime=1x requirement. ########## table/metadata.go: ########## @@ -2969,42 +3036,69 @@ func (m *metadataV1) UnmarshalJSON(b []byte) error { if err := json.Unmarshal(b, aux); err != nil { return err } + if err := next.finishUnmarshal(); err != nil { + return err + } + + *m = *next + return nil +} + +func (m *metadataV1) finishUnmarshal() error { if err := rejectFieldsBeyondVersion( - aux.FormatVersion, - versionScopedField{name: "last-sequence-number", introduced: 2, present: aux.LastSequenceNumber != nil}, - versionScopedField{name: "next-row-id", introduced: 3, present: aux.NextRowID != nil}, - versionScopedField{name: "encryption-keys", introduced: 3, present: len(aux.EncryptionKeyList) > 0}, + m.FormatVersion, + versionScopedField{name: "last-sequence-number", introduced: 2, present: m.commonMetadata.LastSequenceNumber != nil}, + versionScopedField{name: "next-row-id", introduced: 3, present: m.commonMetadata.NextRowID != nil}, + versionScopedField{name: "encryption-keys", introduced: 3, present: len(m.EncryptionKeyList) > 0}, ); err != nil { return err } // CurrentSchemaID was optional in v1, it can also be expressed via Schema. - if aux.CurrentSchemaID == -1 && aux.Schema != nil { - aux.CurrentSchemaID = aux.Schema.ID - if !slices.ContainsFunc(aux.SchemaList, func(s *iceberg.Schema) bool { - return s.Equals(aux.Schema) && s.ID == aux.CurrentSchemaID + if m.CurrentSchemaID == -1 && m.Schema != nil { + m.CurrentSchemaID = m.Schema.ID + if !slices.ContainsFunc(m.SchemaList, func(s *iceberg.Schema) bool { + return s.Equals(m.Schema) && s.ID == m.CurrentSchemaID }) { - aux.SchemaList = append(aux.SchemaList, aux.Schema) + m.SchemaList = append(m.SchemaList, m.Schema) } } - next.preValidate() - if err := next.checkRequiredFields(); err != nil { + m.preValidate() + if err := m.checkRequiredFields(); err != nil { return err } - if err := next.validate(); err != nil { + if err := m.validate(); err != nil { return err } - *m = *next - return nil } +func (m metadataV1) MarshalJSON() ([]byte, error) { + snapshots, err := m.snapshotsForMarshal() + if err != nil { + return nil, err + } + + type Alias metadataV1 + + return json.Marshal(&struct { + *Alias + SnapshotList []Snapshot `json:"snapshots,omitempty"` + }{ + Alias: (*Alias)(&m), + SnapshotList: snapshots, + }) +} + func (m *metadataV1) ToV2() metadataV2 { commonOut := m.commonMetadata + commonOut.SnapshotList = m.allSnapshots() + commonOut.snapshotIndex = buildSnapshotIndex(commonOut.SnapshotList) Review Comment: **nit** — ToV2() result aliases the source's snapshot backing array (pre-existing, not a regression) commonOut.SnapshotList = m.allSnapshots() returns the internal slice rather than a clone, so mutating the returned metadataV2's SnapshotList mutates what the source metadata reports. I verified this is NOT introduced by this PR: the eager path has the identical aliasing on main because commonOut := m.commonMetadata shallow-copies the slice header. Noting only so it is not mistaken for new behaviour. ########## table/deferred_snapshots.go: ########## @@ -0,0 +1,532 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "strconv" + "sync" +) + +// deferredSnapshotState owns the raw snapshot array and the materialized +// collection. It is pointer-owned because commonMetadata is copied by format +// upgrades and builders; copying a used sync.Once would be unsafe. +type deferredSnapshotState struct { + raw json.RawMessage + entries []deferredSnapshotEntry + byID map[int64]int + + allOnce sync.Once + mu sync.RWMutex + snapshots []Snapshot + index *snapshotIndexData + err error +} + +type deferredSnapshotEntry struct { + start int + end int + + once sync.Once + snapshot Snapshot + err error +} + +func (s *deferredSnapshotState) load() ([]Snapshot, *snapshotIndexData, error) { + s.allOnce.Do(func() { + s.mu.RLock() + raw := s.raw + s.mu.RUnlock() + + var snapshots []Snapshot + if err := json.Unmarshal(raw, &snapshots); err != nil { + s.mu.Lock() + s.err = fmt.Errorf("%w: deferred snapshots: %w", ErrInvalidMetadata, err) + s.mu.Unlock() + + return + } + + s.mu.Lock() + s.snapshots = snapshots + s.index = buildSnapshotIndex(snapshots) + s.raw = nil + s.entries = nil + s.byID = nil + s.mu.Unlock() + }) + + s.mu.RLock() + defer s.mu.RUnlock() + + return s.snapshots, s.index, s.err +} + +func (s *deferredSnapshotState) snapshotByID(id int64) (*Snapshot, error) { + s.mu.RLock() + if s.err != nil { + err := s.err + s.mu.RUnlock() + + return nil, err + } + if s.snapshots != nil { + i, ok := snapshotIndexPosition(s.index, s.snapshots, id) + if !ok { + s.mu.RUnlock() + + return nil, nil + } + snapshot := cloneSnapshotPtr(&s.snapshots[i]) + s.mu.RUnlock() + + return snapshot, nil + } + entryIndex, ok := s.byID[id] + if !ok { + s.mu.RUnlock() + + return nil, nil + } + entry := &s.entries[entryIndex] + s.mu.RUnlock() + + entry.once.Do(func() { + s.mu.RLock() + if s.snapshots != nil { + i, found := snapshotIndexPosition(s.index, s.snapshots, id) + if found { + entry.snapshot = s.snapshots[i] + } + s.mu.RUnlock() + + return + } + raw := s.raw[entry.start:entry.end] + s.mu.RUnlock() + + if err := json.Unmarshal(raw, &entry.snapshot); err != nil { + entry.err = fmt.Errorf("%w: deferred snapshot %d: %w", ErrInvalidMetadata, id, err) + } + }) + if entry.err != nil { + return nil, entry.err + } + + return cloneSnapshotPtr(&entry.snapshot), nil +} + +type deferredSnapshotFields struct { + SnapshotID *int64 `json:"snapshot-id"` + ParentSnapshotID *int64 `json:"parent-snapshot-id,omitempty"` + SequenceNumber int64 `json:"sequence-number"` + TimestampMs *int64 `json:"timestamp-ms"` + ManifestList string `json:"manifest-list,omitempty"` + ManifestLocations json.RawMessage `json:"manifests,omitempty"` + Summary json.RawMessage `json:"summary,omitempty"` + SchemaID *int `json:"schema-id,omitempty"` + FirstRowID *int64 `json:"first-row-id,omitempty"` + AddedRows *int64 `json:"added-rows,omitempty"` +} + +func (s deferredSnapshotFields) validateHeavyFields() error { + if err := validateStringArray(s.ManifestLocations); err != nil { + return err + } + + return validateSummaryObject(s.Summary) +} + +func validateStringArray(raw json.RawMessage) error { + raw = bytes.TrimSpace(raw) + if len(raw) == 0 || bytes.Equal(raw, []byte("null")) { + return nil + } + if raw[0] != '[' || raw[len(raw)-1] != ']' { + return errors.New("cannot unmarshal manifests into []string") + } + for i := 1; i < len(raw)-1; { + i = skipJSONSpace(raw, i) + if i == len(raw)-1 { + return nil + } + switch { + case raw[i] == '"': + i = scanJSONString(raw, i) + case hasJSONNullAt(raw, i): + // Match encoding/json's []string behavior: null decodes to "". + i += len("null") + default: + return errors.New("cannot unmarshal manifest location into string") + } + i = skipJSONSpace(raw, i) + if i < len(raw)-1 && raw[i] == ',' { + i++ + + continue + } + if i != len(raw)-1 { + return errors.New("invalid manifests array") + } + } + + return nil +} + +func validateSummaryObject(raw json.RawMessage) error { + raw = bytes.TrimSpace(raw) + if len(raw) == 0 || bytes.Equal(raw, []byte("null")) { + return nil + } + if raw[0] != '{' || raw[len(raw)-1] != '}' { + return errors.New("cannot unmarshal summary into map[string]string") + } + // Summary.UnmarshalJSON decodes through map[string]string. Preserve its + // acceptance contract here: null becomes "", and duplicate keys use the + // last value. In particular, only the final operation value determines + // whether ErrInvalidOperation is returned. + operationSeen, operationEmpty := false, false + for i := 1; i < len(raw)-1; { + i = skipJSONSpace(raw, i) + if i == len(raw)-1 { + break + } + if raw[i] != '"' { + return errors.New("invalid summary key") + } + keyStart := i + i = scanJSONString(raw, i) + key := raw[keyStart:i] + i = skipJSONSpace(raw, i) + if i >= len(raw)-1 || raw[i] != ':' { + return errors.New("invalid summary object") + } + i = skipJSONSpace(raw, i+1) + if i >= len(raw)-1 { + return errors.New("cannot unmarshal summary value into string") + } + + valueEmpty := false + switch { + case raw[i] == '"': + valueStart := i + i = scanJSONString(raw, i) + valueEmpty = i == valueStart+2 + case hasJSONNullAt(raw, i): + // Match encoding/json's map[string]string behavior: null decodes + // to "". A final null operation is therefore invalid. + i += len("null") + valueEmpty = true + default: + return errors.New("cannot unmarshal summary value into string") + } + if summaryKeyIsOperation(key) { + operationSeen = true + operationEmpty = valueEmpty + } + i = skipJSONSpace(raw, i) + if i < len(raw)-1 && raw[i] == ',' { + i++ + + continue + } + if i != len(raw)-1 { + return errors.New("invalid summary object") + } + + break + } + if operationSeen && operationEmpty { + return fmt.Errorf("%w: found empty operation", ErrInvalidOperation) + } + + return nil +} + +func hasJSONNullAt(raw []byte, i int) bool { + return i+len("null") <= len(raw) && bytes.Equal(raw[i:i+len("null")], []byte("null")) +} + +func summaryKeyIsOperation(raw []byte) bool { + if bytes.Equal(raw, []byte(`"operation"`)) { + return true + } + if !bytes.Contains(raw, []byte{'\\'}) { + return false + } + decoded, err := strconv.Unquote(string(raw)) + + return err == nil && decoded == operationKey +} + +func skipJSONSpace(raw []byte, i int) int { + for i < len(raw) { + switch raw[i] { + case ' ', '\t', '\n', '\r': + i++ + default: + return i + } + } + + return i +} + +func scanJSONString(raw []byte, i int) int { + for i++; i < len(raw); i++ { + switch raw[i] { + case '\\': + i++ + case '"': + return i + 1 + } + } + + return len(raw) +} + +type rawJSONSpan struct { + start int + end int +} + +func splitJSONArray(raw []byte) ([]rawJSONSpan, error) { + arrayStart, arrayEnd := trimJSONSpan(raw, 0, len(raw)) + if arrayEnd-arrayStart < 2 || raw[arrayStart] != '[' || raw[arrayEnd-1] != ']' { + return nil, errors.New("expected JSON array") + } + + elements := make([]rawJSONSpan, 0) + start, depth := arrayStart+1, 0 + for i := start; i < arrayEnd-1; i++ { + switch raw[i] { + case '"': + i = scanJSONString(raw, i) - 1 + case '{', '[': + depth++ + case '}', ']': + depth-- + case ',': + if depth == 0 { + elementStart, elementEnd := trimJSONSpan(raw, start, i) + elements = append(elements, rawJSONSpan{start: elementStart, end: elementEnd}) + start = i + 1 + } + } + } + elementStart, elementEnd := trimJSONSpan(raw, start, arrayEnd-1) + if elementStart < elementEnd { + elements = append(elements, rawJSONSpan{start: elementStart, end: elementEnd}) + } + + return elements, nil +} + +func trimJSONSpan(raw []byte, start, end int) (int, int) { + start = skipJSONSpace(raw, start) + for end > start { + switch raw[end-1] { + case ' ', '\t', '\n', '\r': + end-- + default: + return start, end + } + } + + return start, end +} + +func parseNormalizedMetadataBytesDeferredSnapshots(normalized []byte, formatVersion int) (Metadata, error) { + metadata, rawSnapshots, err := decodeMetadataWithRawSnapshots(normalized, formatVersion) + if err != nil { + if errors.Is(err, ErrInvalidMetadata) { + return nil, err + } + + return nil, fmt.Errorf("%w: %w", ErrInvalidMetadata, err) + } + if metadata.Version() != formatVersion { + return nil, fmt.Errorf("%w: preflight selected version %d, decoded version %d", + ErrInvalidMetadataFormatVersion, formatVersion, metadata.Version()) + } + + common := commonMetadataOf(metadata) + deferred, eager, err := prepareDeferredSnapshots(rawSnapshots, common, metadata) + if err != nil { + return nil, err + } + common.SnapshotList = eager + + if err := finishDeferredMetadataUnmarshal(metadata); err != nil { + return nil, err + } + if deferred == nil { + return metadata, nil + } + common.deferredSnapshots = deferred + + return metadata, nil +} + +func decodeMetadataWithRawSnapshots(normalized []byte, formatVersion int) (Metadata, json.RawMessage, error) { + var rawSnapshots json.RawMessage + switch formatVersion { + case 1: + next := initMetadataV1Deser() + type alias metadataV1 + aux := struct { + *alias + SnapshotList json.RawMessage `json:"snapshots"` + }{alias: (*alias)(next)} + if err := json.Unmarshal(normalized, &aux); err != nil { + return nil, nil, err + } + rawSnapshots = aux.SnapshotList + + return next, rawSnapshots, nil + case 2: + next := initMetadataV2Deser() + type alias metadataV2 + aux := struct { + *alias + SnapshotList json.RawMessage `json:"snapshots"` + }{alias: (*alias)(next)} + if err := json.Unmarshal(normalized, &aux); err != nil { + return nil, nil, err + } + rawSnapshots = aux.SnapshotList + + return next, rawSnapshots, nil + case 3: + next := initMetadataV3Deser() + type alias metadataV3 + aux := struct { + *alias + SnapshotList json.RawMessage `json:"snapshots"` + }{alias: (*alias)(next)} + if err := json.Unmarshal(normalized, &aux); err != nil { + return nil, nil, err + } + rawSnapshots = aux.SnapshotList + + return next, rawSnapshots, nil + default: + return nil, nil, ErrInvalidMetadataFormatVersion + } +} + +func finishDeferredMetadataUnmarshal(metadata Metadata) error { + switch typed := metadata.(type) { + case *metadataV1: + return typed.finishUnmarshal() + case *metadataV2: + return typed.finishUnmarshal() + case *metadataV3: + return typed.finishUnmarshal() + default: + return fmt.Errorf("%w: unsupported metadata implementation %T", ErrInvalidMetadata, metadata) + } +} + +func prepareDeferredSnapshots(rawSnapshots json.RawMessage, common *commonMetadata, metadata Metadata) (*deferredSnapshotState, []Snapshot, error) { + rawSnapshots = bytes.TrimSpace(rawSnapshots) + if len(rawSnapshots) == 0 || bytes.Equal(rawSnapshots, []byte("null")) { + return nil, nil, nil + } + + rawEntries, err := splitJSONArray(rawSnapshots) + if err != nil { + return nil, nil, fmt.Errorf("%w: snapshots: %w", ErrInvalidMetadata, err) + } + if len(rawEntries) == 0 { + return nil, []Snapshot{}, nil + } + + refIDs := make(map[int64]struct{}) + for _, ref := range common.SnapshotRefs { + refIDs[ref.SnapshotID] = struct{}{} + } + if common.CurrentSnapshotID != nil && *common.CurrentSnapshotID != -1 { + refIDs[*common.CurrentSnapshotID] = struct{}{} + } + + eager := make([]Snapshot, 0, len(refIDs)) + seen := make(map[int64]struct{}, len(rawEntries)) + state := &deferredSnapshotState{ + raw: rawSnapshots, + entries: make([]deferredSnapshotEntry, len(rawEntries)), + byID: make(map[int64]int, len(rawEntries)), + } + for i, span := range rawEntries { + raw := rawSnapshots[span.start:span.end] + var fields deferredSnapshotFields + if err := json.Unmarshal(raw, &fields); err != nil { + return nil, nil, fmt.Errorf("%w: snapshot at index %d: %w", ErrInvalidMetadata, i, err) + } + if fields.SnapshotID == nil { + return nil, nil, fmt.Errorf("%w: snapshot-id is absent or null", ErrInvalidMetadata) + } + if fields.TimestampMs == nil { + return nil, nil, fmt.Errorf("%w: timestamp-ms is absent or null", ErrInvalidMetadata) + } + if _, ok := seen[*fields.SnapshotID]; ok { + return nil, nil, fmt.Errorf("%w: duplicate snapshot ID %d", ErrInvalidMetadata, *fields.SnapshotID) + } + seen[*fields.SnapshotID] = struct{}{} + state.entries[i] = deferredSnapshotEntry{start: span.start, end: span.end} + state.byID[*fields.SnapshotID] = i + + if err := fields.validateHeavyFields(); err != nil { + return nil, nil, fmt.Errorf("%w: snapshot %d: %w", ErrInvalidMetadata, *fields.SnapshotID, err) + } + if metadata.Version() > 1 && fields.SequenceNumber > metadata.LastSequenceNumber() { + return nil, nil, fmt.Errorf("%w: snapshot %d has sequence number %d which is greater than last-sequence-number %d", + ErrInvalidMetadata, *fields.SnapshotID, fields.SequenceNumber, metadata.LastSequenceNumber()) Review Comment: **nit** — Divergent (but still rejecting) error message when v2 metadata omits last-sequence-number prepareDeferredSnapshots runs its sequence-number check before finishUnmarshal, so a v2 document with no last-sequence-number reports the per-snapshot message against the -1 sentinel instead of the eager path's 'last-sequence-number is required for format versions greater than 1'. Both paths reject with ErrInvalidMetadata, so acceptance parity holds; only the diagnostic differs. -- 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]
