badalprasadsingh commented on code in PR #2204: URL: https://github.com/apache/iceberg-go/pull/2204#discussion_r4312140407
########## table/compaction/partition_tuple_test.go: ########## @@ -0,0 +1,118 @@ +// 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 compaction + +import ( + "bytes" + "fmt" + "math" + "slices" + "strconv" + "strings" + "testing" + "time" +) + +type partitionTupleStringer string + +func (s partitionTupleStringer) String() string { return "custom:" + string(s) } + +func TestAppendPartitionTupleMatchesFmt(t *testing.T) { + values := []any{ + nil, + "", + "a=b;:é", + -123, + int32(-45), + int64(1234567890123), + true, + false, + uint64(1 << 63), + math.NaN(), + math.Inf(1), + math.Copysign(0, -1), + float32(3.5), + []byte{0, 255}, + []int{1, 2}, + partitionTupleStringer("x"), + time.Unix(0, 0).UTC(), + (*int)(nil), + } + + for _, fields := range []int{0, 1, 3, 8, 9, 16, 32} { + for seed := range 32 { + partition := make(map[int]any, fields) + for i := range fields { + partition[1000+i*7] = values[(i*3+seed)%len(values)] + } + + ids := make([]int, 0, len(partition)) + for id := range partition { + ids = append(ids, id) + } + slices.Sort(ids) + + for _, prefix := range []string{"", "42:"} { + want := []byte(prefix) + for _, id := range ids { + want = fmt.Appendf(want, "%d=%v;", id, partition[id]) + } + got := appendPartitionTuple([]byte(prefix), partition) + if !bytes.Equal(got, want) { + t.Fatalf("fields=%d seed=%d prefix=%q: got %q, want %q", + fields, seed, prefix, got, want) + } + } + } + } +} + +func TestPartitionBucketKeyFromTupleMatchesFmt(t *testing.T) { + for _, specID := range []int32{math.MinInt32, -1, 0, 1, math.MaxInt32} { + for _, tuple := range []string{"", "1000=abc;", strings.Repeat("x", 128)} { + got := partitionBucketKeyFromTuple(specID, []byte(tuple)) + want := fmt.Sprintf("%d:%s", specID, tuple) + if got != want { + t.Fatalf("spec ID %d: got %q, want %q", specID, got, want) + } + } + } +} + +var benchmarkPartitionTuple []byte + +func BenchmarkAppendPartitionTuple(b *testing.B) { Review Comment: P2: `BenchmarkAddSurvivorWithSpec` in `eq_delete_decision_bench_test.go` already covers this path at 1, 8 and 32 fields. We should drop this benchmark and extend that one with the value types manifests actually produce (`int32`, `string`, `iceberg.Date`). So that there is one benchmark per hot path. Could you also post benchstat for it (main vs this branch with -count=10)? The numbers in the description come from a ambiguous setup not in the PR. Also, go.mod requires Go 1.26.3, so a Go 1.23.2 install would have switched toolchains. Allocation counts depend on which compiler ran. ########## table/compaction/eq_delete_decision.go: ########## @@ -246,21 +247,48 @@ func partitionBucketKey(specID int32, part map[int]any) string { func partitionBucketKeyFromTuple(specID int32, tuple []byte) string { key := make([]byte, 0, len(tuple)+12) - key = fmt.Appendf(key, "%d:", specID) + key = strconv.AppendInt(key, int64(specID), 10) + key = append(key, ':') return string(append(key, tuple...)) } // appendPartitionTuple emits the sorted "id=value;" tuple into dst. // Caller is responsible for any leading prefix (e.g. specID). func appendPartitionTuple(dst []byte, part map[int]any) []byte { - ids := make([]int, 0, len(part)) + var inlineIDs [8]int + ids := inlineIDs[:0] + if len(part) > len(inlineIDs) { + ids = make([]int, 0, len(part)) + } for id := range part { ids = append(ids, id) } slices.Sort(ids) + + if cap(dst) == 0 && len(part) != 0 { + dst = make([]byte, 0, len(part)*16) + } for _, id := range ids { - dst = fmt.Appendf(dst, "%d=%v;", id, part[id]) + dst = strconv.AppendInt(dst, int64(id), 10) + dst = append(dst, '=') + switch value := part[id].(type) { + case nil: Review Comment: P2: The fast-path types don't quite match what manifests decode to. Avro int/long come back as int32/int64, so case int only helps hand-built maps in tests. day() partitions are very common, and they decode to `iceberg.Date`, which still goes through `fmt.Append`. I'd add this (and case int could go): ```go case iceberg.Date: dst = strconv.AppendInt(dst, int64(value), 10) ``` The output doesn't change because Date has no String method. Please add a Date value to the equivalence test too. ########## table/compaction/eq_delete_decision.go: ########## @@ -246,21 +247,48 @@ func partitionBucketKey(specID int32, part map[int]any) string { func partitionBucketKeyFromTuple(specID int32, tuple []byte) string { key := make([]byte, 0, len(tuple)+12) - key = fmt.Appendf(key, "%d:", specID) + key = strconv.AppendInt(key, int64(specID), 10) + key = append(key, ':') return string(append(key, tuple...)) } // appendPartitionTuple emits the sorted "id=value;" tuple into dst. // Caller is responsible for any leading prefix (e.g. specID). func appendPartitionTuple(dst []byte, part map[int]any) []byte { - ids := make([]int, 0, len(part)) + var inlineIDs [8]int + ids := inlineIDs[:0] + if len(part) > len(inlineIDs) { + ids = make([]int, 0, len(part)) + } for id := range part { ids = append(ids, id) } slices.Sort(ids) + + if cap(dst) == 0 && len(part) != 0 { + dst = make([]byte, 0, len(part)*16) + } for _, id := range ids { - dst = fmt.Appendf(dst, "%d=%v;", id, part[id]) + dst = strconv.AppendInt(dst, int64(id), 10) + dst = append(dst, '=') + switch value := part[id].(type) { + case nil: + dst = append(dst, "<nil>"...) Review Comment: P2: A one-line comment would help here. These cases must produce exactly what `%v` did, because the keys are visible outside this file: iceberg compact prints `Group.PartitionKey`, and `SurvivorSurvey.PartMinSeq` is exported. Without the comment, `"<nil>"` looks arbitrary and someone may "tidy" it later. -- 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]
