zeroshade commented on code in PR #1806:
URL: https://github.com/apache/iceberg-go/pull/1806#discussion_r3806460838
##########
table/metadata.go:
##########
@@ -319,6 +363,7 @@ func (b *MetadataBuilder) clone() *MetadataBuilder {
lastPartitionID: clonePtr(b.lastPartitionID),
props: maps.Clone(b.props),
snapshotList: slices.Clone(b.snapshotList),
+ snapshotIndex: maps.Clone(b.snapshotIndex),
Review Comment:
This adds an O(snapshot-count) map clone to every `MetadataBuilder.clone()`.
`Transaction.apply` calls `clone()` for each staged operation, so large
snapshot histories pay this cost even when no snapshot lookup occurs. Please
make the index lazy/immutable-shareable, or otherwise avoid copying it until
the snapshot list is actually mutated.
##########
table/metadata.go:
##########
@@ -1121,6 +1176,7 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
LastPartitionID: b.lastPartitionID,
Props: b.props,
SnapshotList: b.snapshotList,
+ snapshotIndex: buildSnapshotIndex(b.snapshotList),
Review Comment:
This rebuilds the complete index during `Build()` immediately after the
transaction staging path cloned the existing index. At 10,000 snapshots, the
combined `clone()` + `Build()` path increased locally from 228 µs / 1.34 MB to
494 µs / 1.93 MB. Please avoid this second O(n) construction for unchanged
snapshots and add a benchmark that includes index construction rather than only
warmed lookups.
--
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]
