laskoviymishka commented on code in PR #2118:
URL: https://github.com/apache/iceberg-go/pull/2118#discussion_r4193433513
##########
table/arrow_scanner_bench_test.go:
##########
@@ -276,6 +277,11 @@ func benchmarkSplitParquetScan(b *testing.B) (Metadata,
iceio.IO, []FileScanTask
dataFile := dataFileBuilder.SplitOffsets(offsets).Build()
fullTask := FileScanTask{File: dataFile, Start: 0, Length:
int64(buf.Len())}
splitTasks, split := splitParquetScanTask(fullTask, 1)
+ if rowGroups == 1 && !split {
Review Comment:
A one-line comment would help, since it isn't obvious why `Length--` is
here. A single row group can't be split (`len(offsets) < 2`), so this
fabricates a partial task to force the `RangeSet`/`PlanningSplitOffsets` path.
The `expected one split task per row group` fatal just below also no longer
describes the `rowGroups==1` case, so worth a tweak.
##########
internal/data_file_ref_test.go:
##########
@@ -0,0 +1,102 @@
+// 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
+
+import (
+ "slices"
+ "testing"
+)
+
+type splitOffsetsCollectionsTestFile struct {
+ offsets []int64
+ splitOffsetsCalls int
+ otherCollectionCalls int
+}
+
+func (f *splitOffsetsCollectionsTestFile) ColumnSizes() map[int]int64 {
+ f.otherCollectionCalls++
+
+ return nil
+}
+
+func (f *splitOffsetsCollectionsTestFile) KeyMetadata() []byte {
+ f.otherCollectionCalls++
+
+ return nil
+}
+
+func (f *splitOffsetsCollectionsTestFile) SplitOffsets() []int64 {
+ f.splitOffsetsCalls++
+
+ return f.offsets
+}
+
+func (f *splitOffsetsCollectionsTestFile) EqualityFieldIDs() []int {
+ f.otherCollectionCalls++
+
+ return nil
+}
+
+type splitOffsetsReferenceTestFile struct {
+ splitOffsetsCollectionsTestFile
+ borrowedOffsets []int64
+ refCalls int
+}
+
+func (f *splitOffsetsReferenceTestFile) DataFileCollectionsRef(DataFileRef) (
+ map[int]int64, []byte, []int64, []int,
+) {
+ f.refCalls++
+
+ return nil, nil, f.borrowedOffsets, nil
+}
+
+func TestBorrowedDataFileSplitOffsetsUsesPublicFallbackOnly(t *testing.T) {
+ want := []int64{12, 40, 96}
+ file := &splitOffsetsCollectionsTestFile{offsets: want}
+
+ got := BorrowedDataFileSplitOffsets(file)
+ if !slices.Equal(got, want) {
+ t.Fatalf("BorrowedDataFileSplitOffsets() = %v, want %v", got,
want)
+ }
+ if file.splitOffsetsCalls != 1 {
+ t.Fatalf("SplitOffsets() called %d times, want 1",
file.splitOffsetsCalls)
+ }
+ if file.otherCollectionCalls != 0 {
+ t.Fatalf("other collection getters called %d times, want 0",
file.otherCollectionCalls)
+ }
+}
+
+func TestBorrowedDataFileSplitOffsetsUsesBorrowedReference(t *testing.T) {
+ borrowed := []int64{12, 40, 96}
+ file := &splitOffsetsReferenceTestFile{borrowedOffsets: borrowed}
+
+ got := BorrowedDataFileSplitOffsets(file)
+ if !slices.Equal(got, borrowed) {
+ t.Fatalf("BorrowedDataFileSplitOffsets() = %v, want %v", got,
borrowed)
+ }
+ if len(got) > 0 && &got[0] != &borrowed[0] {
Review Comment:
Both new tests drive hand-written fakes that return whatever they're handed,
so this identity check proves the helper dispatches, not that the real
`*dataFile` borrows. The allocation win rests entirely on
`*dataFile.DataFileCollectionsRef` returning its backing `Splits` array without
copying, and nothing here exercises that path. If it clones, the win silently
disappears and CI stays green.
I'd add a test in the root or `table` package that builds a real `DataFile`,
calls `BorrowedDataFileSplitOffsets` on it twice and asserts `&a[0] == &b[0]`,
plus asserts `df.SplitOffsets()` hands back a different backing array. Making
the two existing cases table-driven over `nil` / `[]int64{}` / populated would
cover the empty paths cheaply while you're in here.
##########
table/arrow_scanner.go:
##########
@@ -2035,7 +2035,7 @@ func (as *arrowScan) processRecordsWithPlans(
(task.Value.Length != 0 && task.Value.Length !=
task.Value.File.FileSizeBytes()) {
tester.RangeSet = true
tester.Start, tester.Length =
adjustParquetTaskRange(task.Value, rdr.SourceFileSize())
- tester.PlanningSplitOffsets =
task.Value.File.SplitOffsets()
+ tester.PlanningSplitOffsets =
iceinternal.BorrowedDataFileSplitOffsets(task.Value.File)
Review Comment:
This stores the borrowed slice in `tester.PlanningSplitOffsets`, which
outlives the call and is shared across the split tasks fanned out from one
file. Every consumer today is read-only so there's no bug, but the field's doc
doesn't say it's now a borrowed, read-only view of the `DataFile`'s internal
state.
A later in-place `slices.Sort`/`Insert`/`append` on it would corrupt
manifest-derived state and race with the other concurrent tasks sharing that
file. I'd restate the read-only contract on the `PlanningSplitOffsets` field
(and/or right here at the assignment), and capping with a three-index slice
(`...[:n:n]`) is a free way to shut the append-aliasing path.
##########
table/scan_splits_bench_test.go:
##########
@@ -0,0 +1,90 @@
+// 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 splitRemoteScanTasksBenchmarkSink []FileScanTask
+
+func BenchmarkSplitRemoteScanTasksSplitOffsets(b *testing.B) {
+ const filesPerOp = 1024
+
+ for _, offsetCount := range []int{1, 32, 128, 512} {
+ for _, publicGetter := range []bool{false, true} {
+ b.Run(fmt.Sprintf("offsets=%d/public=%t", offsetCount,
publicGetter), func(b *testing.B) {
+ fileSize := int64(offsetCount+1) * 64
+ offsets := make([]int64, offsetCount)
+ for i := range offsets {
+ offsets[i] = int64(i+1) * 64
+ }
+
+ dataFileBuilder, err :=
iceberg.NewDataFileBuilder(
+ *iceberg.UnpartitionedSpec,
+ iceberg.EntryContentData,
+ "mem://benchmark/split-offsets.parquet",
+ iceberg.ParquetFile,
+ nil,
+ nil,
+ nil,
+ 1,
+ fileSize,
+ )
+ if err != nil {
+ b.Fatal(err)
+ }
+ file :=
dataFileBuilder.SplitOffsets(offsets).Build()
+ if publicGetter {
+ file =
splitOffsetsPublicBenchmarkFile{DataFile: file}
+ }
+ tasks := make([]FileScanTask, filesPerOp)
+ for i := range tasks {
+ tasks[i] = FileScanTask{File: file,
Start: 0, Length: fileSize}
+ }
+
+ wantTasksPerFile := 1
+ if offsetCount > 1 {
+ splits, ok :=
splitParquetScanTask(tasks[0], fileSize/2)
+ if !ok {
+ b.Fatal("expected split offsets
to produce scan tasks")
+ }
+ wantTasksPerFile = len(splits)
+ }
+ wantTasks := filesPerOp * wantTasksPerFile
+
+ b.ReportAllocs()
+ b.ResetTimer()
+ for b.Loop() {
+ result := splitRemoteScanTasks(tasks,
fileSize/2)
+ if len(result) != wantTasks {
+
b.Fatalf("splitRemoteScanTasks() returned %d tasks, want %d", len(result),
wantTasks)
+ }
+ splitRemoteScanTasksBenchmarkSink =
result
+ }
+ b.ReportMetric(filesPerOp, "files/op")
+ b.ReportMetric(float64(offsetCount),
"split_offsets/file")
+ })
+ }
+ }
+}
+
+type splitOffsetsPublicBenchmarkFile struct{ iceberg.DataFile }
Review Comment:
Worth a one-line comment that this embeds the `iceberg.DataFile` interface
on purpose: that promotes only the public methods, so `DataFileCollectionsRef`
isn't visible, the type assertion fails, and the clone fallback runs, which is
the baseline this benchmark wants. The name alone doesn't convey that the
embedding is what hides the fast path.
--
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]
