You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "alliasgher (via GitHub)" <gi...@apache.org> on 2026/04/13 12:10:50 UTC
[PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
alliasgher opened a new pull request, #873:
URL: https://github.com/apache/iceberg-go/pull/873
## Summary
`MetadataBuilder.RemoveSnapshots()` previously pruned the snapshot list, snapshot log, and refs, but left `StatisticsFile` and `PartitionStatisticsFile` entries behind even when their `SnapshotID` matched a removed snapshot. This left stale statistics entries in the table metadata after snapshot expiration.
Fixes #836
## Changes
### `table/metadata.go`
- Add `statisticsList` and `partitionStatsList` fields to `MetadataBuilder` so the builder can hold them across operations
- Load both lists from the base metadata in `MetadataBuilderFromBase` using the existing `Statistics()` / `PartitionStatistics()` iterators
- Include both lists when building `commonMetadata` in `buildCommonMetadata` so they survive a `Build()` round trip
- In `RemoveSnapshots`, `slices.DeleteFunc` the matching entries out of both lists, mirroring how `snapshotList` and `snapshotLog` are pruned
### `table/metadata_builder_internal_test.go`
- Add `TestRemoveSnapshotsPrunesStatistics` that constructs a metadata with statistics for two snapshots, removes one, and verifies:
- `MetadataBuilderFromBase` loads both lists from the base
- Only the kept snapshot's statistics remain after `RemoveSnapshots`
- The kept entries survive a `Build()` round trip via `Statistics()` and `PartitionStatistics()`
## Verification
- `go build ./table/` passes
- `go vet ./table/` passes
- `go test ./table/` passes (new test + all existing tests)
Fixes #836
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #873:
URL: https://github.com/apache/iceberg-go/pull/873
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on PR #873:
URL: https://github.com/apache/iceberg-go/pull/873#issuecomment-4238643105
Thanks @laskoviymishka for resolving it, I will close #884.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #873:
URL: https://github.com/apache/iceberg-go/pull/873#issuecomment-4238632700
I suggests to close - #884 then, let's split credit by finishing some of each of you PR batches.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "alliasgher (via GitHub)" <gi...@apache.org>.
alliasgher commented on code in PR #873:
URL: https://github.com/apache/iceberg-go/pull/873#discussion_r3074793047
##########
table/metadata.go:
##########
@@ -871,6 +884,8 @@ func (b *MetadataBuilder) buildCommonMetadata() (*commonMetadata, error) {
SortOrderList: b.sortOrderList,
DefaultSortOrderID: b.defaultSortOrderID,
SnapshotRefs: b.refs,
+ StatisticsList: b.statisticsList,
+ PartitionStatsList: b.partitionStatsList,
Review Comment:
Good call — updated the PR description to lead with the builder bug and frame RemoveSnapshots pruning as the secondary fix.
##########
table/metadata.go:
##########
@@ -505,6 +509,15 @@ func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64, postCommit bool)
}
b.refs = newRefs
+ // Prune statistics entries whose snapshot was removed so that table
+ // metadata does not retain stale statistics references.
+ b.statisticsList = slices.DeleteFunc(b.statisticsList, func(e StatisticsFile) bool {
+ return slices.Contains(snapshotIds, e.SnapshotID)
+ })
+ b.partitionStatsList = slices.DeleteFunc(b.partitionStatsList, func(e PartitionStatisticsFile) bool {
+ return slices.Contains(snapshotIds, e.SnapshotID)
+ })
Review Comment:
Noted.
##########
table/metadata_builder_internal_test.go:
##########
@@ -622,6 +624,76 @@ func TestRemoveSnapshotRemovesBranch(t *testing.T) {
}
}
+// TestRemoveSnapshotsPrunesStatistics verifies that RemoveSnapshots also
+// prunes StatisticsList and PartitionStatsList entries whose SnapshotID
+// matches a removed snapshot. See iceberg-go#836.
+func TestRemoveSnapshotsPrunesStatistics(t *testing.T) {
Review Comment:
Added `TestBuildPreservesStatistics` in ed1b085 — plain `MetadataBuilderFromBase → Build()` with no mutations, asserts both lists survive.
##########
table/metadata.go:
##########
@@ -261,6 +263,8 @@ func MetadataBuilderFromBase(metadata Metadata, currentFileLocation string) (*Me
b.refs = maps.Collect(metadata.Refs())
b.snapshotLog = slices.Collect(metadata.SnapshotLogs())
b.metadataLog = slices.Collect(metadata.PreviousFiles())
+ b.statisticsList = slices.Collect(metadata.Statistics())
+ b.partitionStatsList = slices.Collect(metadata.PartitionStatistics())
Review Comment:
Exactly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "alliasgher (via GitHub)" <gi...@apache.org>.
alliasgher commented on PR #873:
URL: https://github.com/apache/iceberg-go/pull/873#issuecomment-4236328645
The `integration-test` failure is `TestScanner/TestIsInFilterTable` in `scanner_test.go`, an Arrow memory-leak assertion. This test is unrelated to the changes in this PR (which only touch `table/metadata.go` for `RemoveSnapshots` and add a new test in `metadata_builder_internal_test.go`).
The same flake hit #870 on Apr 10. Locally, `go test ./table/` passes cleanly with these changes.
Happy to push an empty commit to retrigger CI, or this can be retried by a maintainer.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "alliasgher (via GitHub)" <gi...@apache.org>.
alliasgher commented on code in PR #873:
URL: https://github.com/apache/iceberg-go/pull/873#discussion_r3074793481
##########
table/metadata.go:
##########
@@ -261,6 +263,8 @@ func MetadataBuilderFromBase(metadata Metadata, currentFileLocation string) (*Me
b.refs = maps.Collect(metadata.Refs())
b.snapshotLog = slices.Collect(metadata.SnapshotLogs())
b.metadataLog = slices.Collect(metadata.PreviousFiles())
+ b.statisticsList = slices.Collect(metadata.Statistics())
+ b.partitionStatsList = slices.Collect(metadata.PartitionStatistics())
Review Comment:
Exactly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #873:
URL: https://github.com/apache/iceberg-go/pull/873#discussion_r3074357530
##########
table/metadata.go:
##########
@@ -505,6 +509,15 @@ func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64, postCommit bool)
}
b.refs = newRefs
+ // Prune statistics entries whose snapshot was removed so that table
+ // metadata does not retain stale statistics references.
+ b.statisticsList = slices.DeleteFunc(b.statisticsList, func(e StatisticsFile) bool {
+ return slices.Contains(snapshotIds, e.SnapshotID)
+ })
+ b.partitionStatsList = slices.DeleteFunc(b.partitionStatsList, func(e PartitionStatisticsFile) bool {
+ return slices.Contains(snapshotIds, e.SnapshotID)
+ })
Review Comment:
Consistent with the existing snapshotList/snapshotLog pruning pattern above. Same O(n*m) complexity. Fine for typical use.
##########
table/metadata.go:
##########
@@ -871,6 +884,8 @@ func (b *MetadataBuilder) buildCommonMetadata() (*commonMetadata, error) {
SortOrderList: b.sortOrderList,
DefaultSortOrderID: b.defaultSortOrderID,
SnapshotRefs: b.refs,
+ StatisticsList: b.statisticsList,
+ PartitionStatsList: b.partitionStatsList,
Review Comment:
This is actually the most important line in the PR.
Before this change, every Build() call silently dropped all statistics from the metadata, not just during RemoveSnapshots, but during ANY builder operation (add snapshot, update schema, etc.). The PR description frames this as a RemoveSnapshots bug, but it's a general metadata builder bug. Worth calling out in the description.
##########
table/metadata.go:
##########
@@ -261,6 +263,8 @@ func MetadataBuilderFromBase(metadata Metadata, currentFileLocation string) (*Me
b.refs = maps.Collect(metadata.Refs())
b.snapshotLog = slices.Collect(metadata.SnapshotLogs())
b.metadataLog = slices.Collect(metadata.PreviousFiles())
+ b.statisticsList = slices.Collect(metadata.Statistics())
+ b.partitionStatsList = slices.Collect(metadata.PartitionStatistics())
Review Comment:
Correct — Statistics() and PartitionStatistics() return iterators over the base metadata's lists. slices.Collect materializes them into owned slices for the builder. Consistent with how snapshotLog and metadataLog are loaded right above.
##########
table/metadata_builder_internal_test.go:
##########
@@ -622,6 +624,76 @@ func TestRemoveSnapshotRemovesBranch(t *testing.T) {
}
}
+// TestRemoveSnapshotsPrunesStatistics verifies that RemoveSnapshots also
+// prunes StatisticsList and PartitionStatsList entries whose SnapshotID
+// matches a removed snapshot. See iceberg-go#836.
+func TestRemoveSnapshotsPrunesStatistics(t *testing.T) {
Review Comment:
The test is good — covers load, prune, and build round-trip.
One thing missing: a test that only does MetadataBuilderFromBase → Build() (no removals) and verifies statistics survive.
That pins the buildCommonMetadata fix independently of RemoveSnapshots.
Right now, if someone reverts just the buildCommonMetadata lines but keeps the RemoveSnapshots pruning, this test would still pass (the builder's internal state is correct, but Build() would drop stats) — actually no, the test does call Build() and checks the result.
So it does cover both. But a separate "no-op round-trip preserves stats" test is clearer about what it'sasserting.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: prune StatisticsList and PartitionStatsList in RemoveSnapshots [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #873:
URL: https://github.com/apache/iceberg-go/pull/873#issuecomment-4238532154
@tanmayrauth - can you do a round of code review, since you also worked on this item?
since this PR got landed first i think it worth to work on this, or we can do vice-versa with @alliasgher reviewing your PR-s, just try to avoid conflicts here :D
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org