laskoviymishka commented on code in PR #1955:
URL: https://github.com/apache/iceberg-go/pull/1955#discussion_r3903454239
##########
table/metadata_builder_bench_test.go:
##########
@@ -198,6 +198,75 @@ func BenchmarkMetadataBuilderCloneAndBuild(b *testing.B) {
}
}
+func BenchmarkTransactionApplyNoRequirements(b *testing.B) {
+ for _, snapshotCount := range []int{1, 100, 1_000, 10_000} {
+ b.Run(fmt.Sprintf("snapshots=%d", snapshotCount), func(b
*testing.B) {
+ template := benchmarkSnapshotBuilder(snapshotCount)
+ template.ensureSnapshotIndex()
+ txn := &Transaction{meta: template.clone()}
+
+ b.ReportAllocs()
+ b.ResetTimer()
+
+ for b.Loop() {
+ if err := txn.apply(nil, nil); err != nil {
+ b.Fatal(err)
+ }
+ metadataBuilderBenchmarkSink =
len(txn.meta.snapshotList)
+ }
+ b.ReportMetric(float64(snapshotCount),
"snapshot_entries/op")
Review Comment:
Small consistency thing across all three new benchmarks: the others in this
file call `ReportMetric` before `ResetTimer` and drop the `/op` suffix (the
framework already divides by iteration count). Matching that would keep the
numbers lined up with the rest of the file. Pure nit.
##########
table/transaction.go:
##########
@@ -208,6 +208,12 @@ func (t *Transaction) apply(updates []Update, reqs
[]Requirement) error {
continue
}
+ if current == nil {
+ current, err = stagedMeta.Build()
Review Comment:
Still not a bug, since you check `err` on the next line, but this assignment
lands in two different scopes: `current` is the function-level cache, while
`err` binds to the loop-body `err` from `key, err := requirementSemanticKey(r)`
a few lines up, not the function-level one. A future reorder or rename could
quietly have it fall through to the outer `err`. A local keeps the intent
obvious:
```go
if current == nil {
built, err := stagedMeta.Build()
if err != nil {
return err
}
current = built
}
```
Cheap to do while you're in here; fine to leave if you'd rather not churn it.
--
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]
