laskoviymishka commented on code in PR #1654: URL: https://github.com/apache/iceberg-go/pull/1654#discussion_r3779436072
########## catalog/rest/load_table_bench_test.go: ########## @@ -0,0 +1,201 @@ +// 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" + "testing" + + "github.com/apache/iceberg-go/table" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// BenchmarkDecodeTableMetadata benchmarks the json decoding of LoadTable responses. Because responses can get big +// with tables with a long snapshot history, the impact of the json decoding performance can be significant in +// higher-throughput workloads. This benchmark can be extended to experiment with other json decoder for +// performance comparisons. +func BenchmarkDecodeTableMetadata(b *testing.B) { + snapshotCounts := []struct { + name string + snapshotCount int64 + decode func(body []byte, v any) error + }{ + { + name: "1 snapshot, encoding/json", + snapshotCount: 1, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10 snapshots, encoding/json", + snapshotCount: 10, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "100 snapshots, encoding/json", + snapshotCount: 100, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "1000 snapshots, encoding/json", + snapshotCount: 1000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, encoding/json", + snapshotCount: 10000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, goccy/go-json", + snapshotCount: 10000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + } + + for _, tc := range snapshotCounts { + b.Run(tc.name, func(b *testing.B) { + body := makeTableResponseWithSnapshots(tc.snapshotCount) + + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + var resp loadTableResponse + err := tc.decode(body, &resp) + require.NoError(b, err) + } + }) + } +} + +// makeTableResponseWithSnapshots formats a LoadTable json body with a specified number of snapshots. Snapshots are used +// here to exercise different response size profiles as it's one of the common sources for large responses. +func makeTableResponseWithSnapshots(snapshotCount int64) []byte { + snapshots := make([]table.Snapshot, 0, snapshotCount) + snapshotLogEntries := make([]table.SnapshotLogEntry, 0, snapshotCount) + schemaID := 0 + var snapshotTimestamp int64 + var snapshotID int64 + for i := int64(0); i < snapshotCount; i++ { + var parentID *int64 + if i > 0 { + parentID = &i Review Comment: `parentID = &i` takes the address of the loop counter, not a copy. This is a C-style for loop, so Go 1.22's per-iteration capture doesn't apply and every snapshot shares the one `i`. After the loop `i == snapshotCount`, so every non-first snapshot marshals with `parent-snapshot-id: <snapshotCount>`, an ID that isn't in the list. Doesn't move the decode timing, but the fixture stops being a valid snapshot chain, which bites anyone who copies this helper for a correctness test. `prev := i - 1; parentID = &prev` fixes it. ########## catalog/rest/load_table_bench_test.go: ########## @@ -0,0 +1,201 @@ +// 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" + "testing" + + "github.com/apache/iceberg-go/table" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// BenchmarkDecodeTableMetadata benchmarks the json decoding of LoadTable responses. Because responses can get big +// with tables with a long snapshot history, the impact of the json decoding performance can be significant in +// higher-throughput workloads. This benchmark can be extended to experiment with other json decoder for +// performance comparisons. +func BenchmarkDecodeTableMetadata(b *testing.B) { + snapshotCounts := []struct { + name string + snapshotCount int64 + decode func(body []byte, v any) error + }{ + { + name: "1 snapshot, encoding/json", + snapshotCount: 1, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10 snapshots, encoding/json", + snapshotCount: 10, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "100 snapshots, encoding/json", + snapshotCount: 100, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "1000 snapshots, encoding/json", + snapshotCount: 1000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, encoding/json", + snapshotCount: 10000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, goccy/go-json", Review Comment: This row is labeled `goccy/go-json` but its decode closure calls `encoding/json`'s Unmarshal, identical to the `encoding/json` row above it, so the benchmark reports two 10000-snapshot numbers that are the same decoder under two names. The whole point of the harness is comparing decoders, so a mislabeled slot is worse than no slot: anyone reading the output concludes goccy is a wash, on fake data. I'd drop this entry until there's a real second decoder to put here. Matt's suggestion of `encoding/json/v2` on 1.26 is the natural thing to slot in: a real comparison and no third-party risk. wdyt? ########## catalog/rest/load_table_bench_test.go: ########## @@ -0,0 +1,201 @@ +// 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" + "testing" + + "github.com/apache/iceberg-go/table" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// BenchmarkDecodeTableMetadata benchmarks the json decoding of LoadTable responses. Because responses can get big +// with tables with a long snapshot history, the impact of the json decoding performance can be significant in +// higher-throughput workloads. This benchmark can be extended to experiment with other json decoder for +// performance comparisons. +func BenchmarkDecodeTableMetadata(b *testing.B) { + snapshotCounts := []struct { + name string + snapshotCount int64 + decode func(body []byte, v any) error + }{ + { + name: "1 snapshot, encoding/json", + snapshotCount: 1, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10 snapshots, encoding/json", + snapshotCount: 10, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "100 snapshots, encoding/json", + snapshotCount: 100, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "1000 snapshots, encoding/json", + snapshotCount: 1000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, encoding/json", + snapshotCount: 10000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, goccy/go-json", + snapshotCount: 10000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + } + + for _, tc := range snapshotCounts { + b.Run(tc.name, func(b *testing.B) { + body := makeTableResponseWithSnapshots(tc.snapshotCount) + + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + var resp loadTableResponse + err := tc.decode(body, &resp) + require.NoError(b, err) Review Comment: `require.NoError` runs testify's reflection/formatting machinery every iteration, and for the 1- and 10-snapshot cases that's a real fraction of the measured time. `if err != nil { b.Fatal(err) }` keeps the timed loop clean. While we're in here: `ReportAllocs` conventionally goes before `ResetTimer`, and since the module's on 1.25 you could drop both the `b.N` loop and `ResetTimer` for `for b.Loop() { ... }`, which handles the reset itself. ########## catalog/rest/scan_task_decoder_test.go: ########## @@ -482,34 +484,42 @@ func TestDecodePartitionLiteralCoversPrimitiveWireTypes(t *testing.T) { decimalType := iceberg.DecimalTypeOf(9, 2) fixedType := iceberg.FixedTypeOf(4) tests := []struct { - name string - raw string - typ iceberg.Type - want iceberg.Literal + name string + raw string + typ iceberg.Type + want iceberg.Literal + expectedErr error }{ - {"boolean", `true`, iceberg.PrimitiveTypes.Bool, iceberg.BoolLiteral(true)}, - {"int", `2147483647`, iceberg.PrimitiveTypes.Int32, iceberg.Int32Literal(2147483647)}, - {"long above JSON exact float range", `9007199254740993`, iceberg.PrimitiveTypes.Int64, iceberg.Int64Literal(9007199254740993)}, - {"float", `1.25`, iceberg.PrimitiveTypes.Float32, iceberg.Float32Literal(1.25)}, - {"double", `1.25`, iceberg.PrimitiveTypes.Float64, iceberg.Float64Literal(1.25)}, - {"string", `"hello"`, iceberg.PrimitiveTypes.String, iceberg.StringLiteral("hello")}, - {"date", `"2026-07-17"`, iceberg.PrimitiveTypes.Date, mustLiteral(t, "2026-07-17", iceberg.PrimitiveTypes.Date)}, - {"time", `"10:15:30.123456"`, iceberg.PrimitiveTypes.Time, mustLiteral(t, "10:15:30.123456", iceberg.PrimitiveTypes.Time)}, - {"timestamp", `"2026-07-17T10:15:30.123456"`, iceberg.PrimitiveTypes.Timestamp, mustLiteral(t, "2026-07-17T10:15:30.123456", iceberg.PrimitiveTypes.Timestamp)}, - {"timestamptz", `"2026-07-17T10:15:30.123456+00:00"`, iceberg.PrimitiveTypes.TimestampTz, mustLiteral(t, "2026-07-17T10:15:30.123456+00:00", iceberg.PrimitiveTypes.TimestampTz)}, - {"timestamp nanos", `"2026-07-17T10:15:30.123456789"`, iceberg.PrimitiveTypes.TimestampNs, mustLiteral(t, "2026-07-17T10:15:30.123456789", iceberg.PrimitiveTypes.TimestampNs)}, - {"timestamptz nanos", `"2026-07-17T10:15:30.123456789+00:00"`, iceberg.PrimitiveTypes.TimestampTzNs, mustLiteral(t, "2026-07-17T10:15:30.123456789+00:00", iceberg.PrimitiveTypes.TimestampTzNs)}, - {"decimal", `"12.34"`, decimalType, mustLiteral(t, "12.34", decimalType)}, - {"uuid", `"f79c3e09-677c-4bbd-a479-3f349cb785e7"`, iceberg.PrimitiveTypes.UUID, mustLiteral(t, "f79c3e09-677c-4bbd-a479-3f349cb785e7", iceberg.PrimitiveTypes.UUID)}, - {"fixed", `"78797A21"`, fixedType, iceberg.FixedLiteral([]byte("xyz!"))}, - {"binary", `"00FF10"`, iceberg.PrimitiveTypes.Binary, iceberg.BinaryLiteral([]byte{0, 255, 16})}, + {"boolean", `true`, iceberg.PrimitiveTypes.Bool, iceberg.BoolLiteral(true), nil}, + {"int", `2147483647`, iceberg.PrimitiveTypes.Int32, iceberg.Int32Literal(2147483647), nil}, + {"long above JSON exact float range", `9007199254740993`, iceberg.PrimitiveTypes.Int64, iceberg.Int64Literal(9007199254740993), nil}, + {"max int64", `9223372036854775807`, iceberg.PrimitiveTypes.Int64, iceberg.Int64Literal(math.MaxInt64), nil}, + {"above max int64", `9223372036854775808`, iceberg.PrimitiveTypes.Int64, iceberg.Int64Literal(0), strconv.ErrRange}, Review Comment: The `want` on this row is dead: the error branch returns before `got.Equals(tt.want)` runs, so `iceberg.Int64Literal(0)` is never checked and reads as if we assert a zero literal on overflow when we don't. `nil` says "unused" unambiguously since `want` is an interface. Or, if you do want to pin the return value on the error path, assert `got` inside the `expectedErr` branch, either's fine, just not a placeholder that looks load-bearing. ########## catalog/rest/load_table_bench_test.go: ########## @@ -0,0 +1,201 @@ +// 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" + "testing" + + "github.com/apache/iceberg-go/table" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// BenchmarkDecodeTableMetadata benchmarks the json decoding of LoadTable responses. Because responses can get big +// with tables with a long snapshot history, the impact of the json decoding performance can be significant in +// higher-throughput workloads. This benchmark can be extended to experiment with other json decoder for +// performance comparisons. +func BenchmarkDecodeTableMetadata(b *testing.B) { + snapshotCounts := []struct { + name string + snapshotCount int64 + decode func(body []byte, v any) error + }{ + { + name: "1 snapshot, encoding/json", + snapshotCount: 1, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10 snapshots, encoding/json", + snapshotCount: 10, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "100 snapshots, encoding/json", + snapshotCount: 100, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "1000 snapshots, encoding/json", + snapshotCount: 1000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, encoding/json", + snapshotCount: 10000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + { + name: "10000 snapshots, goccy/go-json", + snapshotCount: 10000, + decode: func(body []byte, v any) error { + return json.Unmarshal(body, v) + }, + }, + } + + for _, tc := range snapshotCounts { + b.Run(tc.name, func(b *testing.B) { + body := makeTableResponseWithSnapshots(tc.snapshotCount) + + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + var resp loadTableResponse + err := tc.decode(body, &resp) + require.NoError(b, err) + } + }) + } +} + +// makeTableResponseWithSnapshots formats a LoadTable json body with a specified number of snapshots. Snapshots are used +// here to exercise different response size profiles as it's one of the common sources for large responses. +func makeTableResponseWithSnapshots(snapshotCount int64) []byte { + snapshots := make([]table.Snapshot, 0, snapshotCount) + snapshotLogEntries := make([]table.SnapshotLogEntry, 0, snapshotCount) + schemaID := 0 + var snapshotTimestamp int64 + var snapshotID int64 + for i := int64(0); i < snapshotCount; i++ { + var parentID *int64 + if i > 0 { + parentID = &i + } + snapshotTimestamp = 1785448901408 + i + snapshotID = i + + snapshots = append(snapshots, table.Snapshot{ + SnapshotID: snapshotID, + ParentSnapshotID: parentID, + SequenceNumber: i, + TimestampMs: snapshotTimestamp, + ManifestList: fmt.Sprintf("s3://warehouse/database/table/metadata/snap-%s.avro", uuid.NewString()), + Summary: &table.Summary{ + Operation: "append", + Properties: map[string]string{ + "spark.app.id": "local-1646787004168", + "added-data-files": "1", + "added-records": "1", + "added-files-size": "697", + "changed-partition-count": "1", + "total-records": "1", + "total-files-size": "697", + "total-data-files": "1", + "total-delete-files": "0", + "total-position-deletes": "0", + "total-equality-deletes": "0", + }, + }, + SchemaID: &schemaID, + }) + snapshotLogEntries = append(snapshotLogEntries, table.SnapshotLogEntry{ + SnapshotID: i, + TimestampMs: snapshotTimestamp, + }) + } + snapshotsJson, err := json.Marshal(snapshots) + if err != nil { + panic(fmt.Errorf("failed to generate load table response: %w", err)) + } + + snapshotsLogEntriesJson, err := json.Marshal(snapshotLogEntries) + if err != nil { + panic(fmt.Errorf("failed to generate load table response: %w", err)) + } + + return []byte(fmt.Sprintf(`{ + "metadata-location": "s3://warehouse/database/table/metadata/00001-5f2f8166-244c-4eae-ac36-384ecdec81fc.gz.metadata.json", + "metadata": { + "format-version": 1, Review Comment: Small realism thing: the fixture says format-version 1 but each snapshot carries a `sequence-number` (`Snapshot.SequenceNumber` has no `omitempty` and you set it to `i`), plus it uses `schemas`/`current-schema-id`, which are V2 shapes. A real V1 LoadTable response wouldn't have per-snapshot sequence numbers, so the decode is chewing tokens it'd never see in the wild. Since the body's already V2-shaped, I'd just bump this to `"format-version": 2`. wdyt? -- 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]
