zeroshade commented on code in PR #2118:
URL: https://github.com/apache/iceberg-go/pull/2118#discussion_r4198575436
##########
internal/data_file_ref.go:
##########
@@ -80,6 +82,21 @@ type DataFileCollectionsRef interface {
)
}
+// BorrowedDataFileSplitOffsets returns split offsets without copying for the
+// built-in data file and falls back to the public getter for external
+// implementations. The returned slice is read-only and must not be mutated or
+// retained beyond the current planning or read operation.
+func BorrowedDataFileSplitOffsets(file DataFileCollections) []int64 {
+ var splitOffsets []int64
+ if ref, ok := file.(DataFileCollectionsRef); ok {
+ _, _, splitOffsets, _ =
ref.DataFileCollectionsRef(DataFileRef{})
Review Comment:
This goes through `DataFileCollectionsRef`, and that method starts with
`d.initColumnStatsData()`. So the first borrow on a `*dataFile` whose stats
haven't been touched yet builds up to seven column-stat maps (sizes,
value/null/NaN/distinct counts, lower/upper bounds) just to read `*d.Splits`.
The `SplitOffsets()` clone this replaces never did that, and the maps then stay
on the `DataFile` that every returned task holds.
That's the normal state for `Scan.PlanFiles()` without a row filter: the
manifest is decoded with full metrics, the AlwaysTrue metrics evaluator only
looks at `Count()`, and `splitParquetScanTask` is the first thing to call into
the Ref for every file above `read.split.target-size`. REST-planned files hit
the same path whenever the server returns stats. `ToArrowRecords` is fine
because it projects stats away before splitting.
Measured on `PlanFiles`, 4 manifests × 256 files of 512 MiB, 8 split
offsets, no filter (benchstat, `-count=6`, merge-base `0b0828b` → this head):
| metric columns | B/op | allocs/op |
| ---: | ---: | ---: |
| 1 | 6.60 → 7.76 MiB (+17.5%) | 40.3k → 49.6k (+22.9%) |
| 20 | 10.7 → 15.0 MiB (+40.2%) | 79.6k → 99.0k (+24.5%) |
| 100 | 28.9 → 45.6 MiB (+57.4%) | 244.7k → 264.2k (+8.0%) |
Per file, the first borrow costs 20 allocs and 2.4–33 KiB (10–200 columns),
where the clone it replaces was one 64 B alloc.
The narrow accessor from your first commit (a71b303) didn't have this
problem. A `DataFileSplitOffsetsRef(DataFileRef) []int64` on `*dataFile` that
returns `*d.Splits` without the init measured 0 allocs / 0 B on first access at
every column count, and it's token-gated, so it adds no public API. I'd go back
to that, keep the `slices.Clip`, and:
- update the interface named in the `splitOffsetsPublicBenchmarkFile`
comment, the internal test fake, and the `require.Implements` in
`TestBorrowedDataFileSplitOffsetsUsesBuiltInBackingSlice`;
- add a regression test that borrowing from a fresh file with column stats
leaves its stat maps unmaterialized (a root-package test checking `colSizeMap
== nil` after the call is enough).
##########
table/scan_splits_bench_test.go:
##########
@@ -0,0 +1,92 @@
+// 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}
Review Comment:
All 1,024 tasks share one `DataFile` with no column stats, so this can't see
per-file costs on the borrow path: any one-time work on first access (like the
`initColumnStatsData()` call flagged on `BorrowedDataFileSplitOffsets`) runs
once for the whole benchmark and then hits the `sync.Once` fast path. It's also
why the description's "1,024 files per operation" is really 1,024 tasks over
one file. Please build a distinct file per task with some column metrics
(`ColumnSizes`/`ValueCounts`/bounds) and rebuild them outside the timer each
iteration, or drive it through `PlanFiles` over a written manifest, so it
measures first access per file the way planning does.
--
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]
