laskoviymishka commented on code in PR #1908: URL: https://github.com/apache/iceberg-go/pull/1908#discussion_r3874719608
########## internal/manifest_file_ref.go: ########## @@ -0,0 +1,23 @@ +// 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 internal + +// ManifestFileRef authorizes zero-copy access to immutable manifest state from +// trusted packages within this module. Go's internal-package rule prevents +// external callers from constructing this token. +type ManifestFileRef struct{} Review Comment: The read-only/no-retain contract lives on `manifestFilePartitions`, but "trusted packages within this module" is really any in-module package, and any of them can construct this token and call the method directly, bypassing the helper. Since the token is the thing a caller holds, I'd repeat the invariant here: borrowed, read-only, don't retain past the current operation. That way the contract travels with the capability rather than living only on one entry point. ########## table/manifest_file_ref.go: ########## @@ -0,0 +1,39 @@ +// 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 ( + "github.com/apache/iceberg-go" + iceberginternal "github.com/apache/iceberg-go/internal" +) + +type manifestFilePartitionRef interface { Review Comment: The `DataFileRef` pattern already merged keeps all these borrow helpers in `internal` (`BorrowedDataFileStats`, `BorrowedDataFileBounds`, and friends), so any trusted package in the module can reach zero-copy access without redeclaring the interface. This one lives in `table`, which means the next caller that wants borrowed partition summaries (scan planning, metrics) has to redeclare the interface and dispatch from scratch. I'd move it to `internal` as `BorrowedManifestFilePartitions(ManifestFile) []FieldSummary` following that model, at which point this file collapses to a one-line call. While we're here, the unexported interface `manifestFilePartitionRef` and its method `ManifestFilePartitionRef` differ only by the leading case, which is easy to misread at a glance. Something like `manifestPartitionBorrower` would read more clearly. wdyt? ########## table/manifest_file_ref_test.go: ########## @@ -0,0 +1,71 @@ +// 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 ( + "testing" + + "github.com/apache/iceberg-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type publicManifestFile struct { + iceberg.ManifestFile + partitionCalls int +} + +func (m *publicManifestFile) Partitions() []iceberg.FieldSummary { + m.partitionCalls++ + + return m.ManifestFile.Partitions() +} + +func TestManifestFilePartitionsUsesBorrowedView(t *testing.T) { + containsNaN := false + lower := []byte{1, 2} + upper := []byte{3, 4} + manifest := iceberg.NewManifestFile(2, "manifest.avro", 0, 1, 1).Partitions( + []iceberg.FieldSummary{{ + ContainsNaN: &containsNaN, + LowerBound: &lower, + UpperBound: &upper, + }}, + ).Build() + + require.Implements(t, (*manifestFilePartitionRef)(nil), manifest) + partitions := manifestFilePartitions(manifest) + require.Len(t, partitions, 1) + assert.Equal(t, []byte{1, 2}, *partitions[0].LowerBound) + assert.Equal(t, []byte{3, 4}, *partitions[0].UpperBound) Review Comment: This proves the values are right and (below) that the call doesn't allocate, but it never proves the view actually aliases the manifest. A future refactor that returned a shallow copy sharing the `LowerBound` pointers, or a pooled deep copy, would pass this test unchanged, so it doesn't distinguish "truly borrowed" from "cheap but not aliasing", which is the property the whole PR turns on. I'd add an assertion that mutates through the borrowed pointer and shows the manifest sees it: grab `partitions[0].LowerBound`, write to `(*partitions[0].LowerBound)[0]`, then read the manifest's stored bound back and assert it changed. Label it as the aliasing hazard callers must not trigger, so it documents the contract and locks in the borrow at the same time. ########## table/evaluators_bench_test.go: ########## @@ -0,0 +1,120 @@ +// 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 ( + "fmt" + "testing" + + "github.com/apache/iceberg-go" +) + +var manifestEvaluatorBenchmarkSink int + +func BenchmarkManifestEvaluatorBuiltInPartitions(b *testing.B) { + for _, manifestCount := range []int{1, 100, 1_000} { + for _, fieldCount := range []int{1, 8, 32} { + b.Run(fmt.Sprintf("manifests=%d/fields=%d", manifestCount, fieldCount), func(b *testing.B) { + spec, schema := manifestEvaluatorBenchmarkSpec(fieldCount) + filter := manifestEvaluatorBenchmarkFilter(fieldCount) + eval, err := newManifestEvaluator(spec, schema, filter, true) + if err != nil { + b.Fatal(err) + } + + summaries := manifestEvaluatorBenchmarkSummaries(fieldCount) + manifests := make([]iceberg.ManifestFile, manifestCount) + for i := range manifests { + manifests[i] = iceberg.NewManifestFile( + 2, fmt.Sprintf("manifest-%d.avro", i), 0, int32(spec.ID()), 1, + ).Partitions(summaries).Build() + } + + b.ReportAllocs() + b.ReportMetric(float64(manifestCount), "manifests") Review Comment: These two report static dimensions, so the output reads "100 manifests/op" where the `/op` implies a per-iteration rate that isn't one. The counts are already in the sub-benchmark name (`manifests=100/fields=8`), so I'd just drop both `ReportMetric` calls. ########## table/evaluators_bench_test.go: ########## @@ -0,0 +1,120 @@ +// 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 ( + "fmt" + "testing" + + "github.com/apache/iceberg-go" +) + +var manifestEvaluatorBenchmarkSink int + +func BenchmarkManifestEvaluatorBuiltInPartitions(b *testing.B) { + for _, manifestCount := range []int{1, 100, 1_000} { + for _, fieldCount := range []int{1, 8, 32} { + b.Run(fmt.Sprintf("manifests=%d/fields=%d", manifestCount, fieldCount), func(b *testing.B) { + spec, schema := manifestEvaluatorBenchmarkSpec(fieldCount) + filter := manifestEvaluatorBenchmarkFilter(fieldCount) + eval, err := newManifestEvaluator(spec, schema, filter, true) + if err != nil { + b.Fatal(err) + } + + summaries := manifestEvaluatorBenchmarkSummaries(fieldCount) + manifests := make([]iceberg.ManifestFile, manifestCount) + for i := range manifests { + manifests[i] = iceberg.NewManifestFile( + 2, fmt.Sprintf("manifest-%d.avro", i), 0, int32(spec.ID()), 1, + ).Partitions(summaries).Build() + } + + b.ReportAllocs() + b.ReportMetric(float64(manifestCount), "manifests") + b.ReportMetric(float64(fieldCount), "partition_fields") + b.ResetTimer() + for range b.N { + matched := 0 + for _, manifest := range manifests { + keep, err := eval(manifest) + if err != nil { + b.Fatal(err) + } + if keep { + matched++ + } + } + manifestEvaluatorBenchmarkSink = matched + } + }) + } + } +} + +func manifestEvaluatorBenchmarkSpec(fieldCount int) (iceberg.PartitionSpec, *iceberg.Schema) { + schemaFields := make([]iceberg.NestedField, fieldCount) + partitionFields := make([]iceberg.PartitionField, fieldCount) + for i := range fieldCount { + fieldID := i + 1 + fieldName := fmt.Sprintf("field_%d", i) + schemaFields[i] = iceberg.NestedField{ + ID: fieldID, Name: fieldName, Type: iceberg.PrimitiveTypes.Int32, Required: true, + } + partitionFields[i] = iceberg.PartitionField{ + SourceIDs: []int{fieldID}, FieldID: 1000 + i, + Name: fieldName, Transform: iceberg.IdentityTransform{}, + } + } + + return iceberg.NewPartitionSpecID(1, partitionFields...), iceberg.NewSchema(1, schemaFields...) +} + +func manifestEvaluatorBenchmarkFilter(fieldCount int) iceberg.BooleanExpression { + var filter iceberg.BooleanExpression = iceberg.GreaterThanEqual( + iceberg.Reference("field_0"), int32(0)) + for i := 1; i < fieldCount; i++ { + filter = iceberg.NewAnd(filter, + iceberg.GreaterThanEqual(iceberg.Reference(fmt.Sprintf("field_%d", i)), int32(0))) + } + + return filter +} + +func manifestEvaluatorBenchmarkSummaries(fieldCount int) []iceberg.FieldSummary { + lower, err := iceberg.Int32Literal(0).MarshalBinary() + if err != nil { + panic(err) + } + upper, err := iceberg.Int32Literal(100).MarshalBinary() + if err != nil { + panic(err) + } + containsNaN := false + + summaries := make([]iceberg.FieldSummary, fieldCount) + for i := range summaries { + summaries[i] = iceberg.FieldSummary{ Review Comment: Every summary here shares the same three pointers (`&containsNaN`, `&lower`, `&upper`). It's safe today only because `ManifestBuilder.Partitions()` deep-clones each entry via `cloneFieldSummaries`. But this PR is specifically about borrowing those bounds without copying, so a fixture that aliases across elements is exactly the footgun the borrowed path could trip on if this pattern gets copied into a test that skips the builder. I'd give each element its own copy inside the loop (fresh bool, `slices.Clone` the bounds). ########## table/manifest_file_ref_test.go: ########## @@ -0,0 +1,71 @@ +// 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 ( + "testing" + + "github.com/apache/iceberg-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type publicManifestFile struct { + iceberg.ManifestFile + partitionCalls int +} + +func (m *publicManifestFile) Partitions() []iceberg.FieldSummary { + m.partitionCalls++ + + return m.ManifestFile.Partitions() +} + +func TestManifestFilePartitionsUsesBorrowedView(t *testing.T) { + containsNaN := false + lower := []byte{1, 2} + upper := []byte{3, 4} + manifest := iceberg.NewManifestFile(2, "manifest.avro", 0, 1, 1).Partitions( + []iceberg.FieldSummary{{ + ContainsNaN: &containsNaN, + LowerBound: &lower, + UpperBound: &upper, + }}, + ).Build() + + require.Implements(t, (*manifestFilePartitionRef)(nil), manifest) + partitions := manifestFilePartitions(manifest) + require.Len(t, partitions, 1) + assert.Equal(t, []byte{1, 2}, *partitions[0].LowerBound) + assert.Equal(t, []byte{3, 4}, *partitions[0].UpperBound) + + allocs := testing.AllocsPerRun(100, func() { + partitions = manifestFilePartitions(manifest) + }) + assert.InDelta(t, 0.0, allocs, 0.5) Review Comment: `AllocsPerRun(100, ...)` returns total allocations divided by 100, so `InDelta(0.0, allocs, 0.5)` passes with up to 50 allocations across the run. A regression that allocated on every other call would sail straight through it. Since a genuine zero-alloc path returns exactly `0.0`, I'd assert that directly: ```go assert.Zero(t, allocs) ``` This is the guarantee the test exists to make, so it's worth making it exact. ########## manifest_file_refs.go: ########## @@ -0,0 +1,32 @@ +// 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 iceberg + +import "github.com/apache/iceberg-go/internal" + +// ManifestFilePartitionRef returns the manifest's partition summaries without +// copying for trusted in-module callers. The returned slice and all nested +// bounds alias the manifest and must be treated as read-only for the current +// operation. +func (m *manifestFile) ManifestFilePartitionRef(_ internal.ManifestFileRef) []FieldSummary { + if m.PartitionList == nil { Review Comment: This returns the same nil/empty result as `Partitions()` only because of an invariant that isn't written down anywhere: when `PartitionList` is non-nil it always points at a non-nil slice (the builder never stores `&nilSlice`, and the Avro path normalizes through `ensurePartitionList`). If that ever stops holding, the borrowed path diverges from `Partitions()` silently. I'd add a one-line comment stating the invariant here so a future change doesn't break it unknowingly. -- 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]
