From 3ec934650bd8306b63cb92f9816aa88010836e41 Mon Sep 17 00:00:00 2001 From: Anshul Mishra Date: Wed, 7 Oct 2026 19:32:36 +0530 Subject: [PATCH 1/2] fix(table): keep earlier updates when chaining remove_statistics UpdateStatistics.remove_statistics assigned self._updates instead of appending to it, so any set_statistics call staged earlier in the same chain was silently dropped. For example, table.update_statistics().set_statistics(f).remove_statistics(id).commit() only removed the statistics and never registered f. This is the usage shown in the class docstring and in the API docs. Append with += like set_statistics does, and add a regression test that chains set_statistics and remove_statistics and checks that both updates are staged and applied. Follow-up to #3557. --- pyiceberg/table/update/statistics.py | 2 +- tests/table/test_init.py | 31 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/pyiceberg/table/update/statistics.py b/pyiceberg/table/update/statistics.py index 76fe2cb07b..42e5e47c65 100644 --- a/pyiceberg/table/update/statistics.py +++ b/pyiceberg/table/update/statistics.py @@ -62,7 +62,7 @@ def set_statistics(self, statistics_file: StatisticsFile) -> "UpdateStatistics": return self def remove_statistics(self, snapshot_id: int) -> "UpdateStatistics": - self._updates = ( + self._updates += ( RemoveStatisticsUpdate( snapshot_id=snapshot_id, ), diff --git a/tests/table/test_init.py b/tests/table/test_init.py index 3d160781e3..e6a86db350 100644 --- a/tests/table/test_init.py +++ b/tests/table/test_init.py @@ -1849,6 +1849,37 @@ def test_remove_statistics_update(table_v2_with_statistics: Table) -> None: ) +def test_update_statistics_set_then_remove_keeps_both_updates(table_v2_with_statistics: Table) -> None: + current_snapshot_id = 3055729675574597004 + previous_snapshot_id = 3051729675574597004 + + statistics_file = StatisticsFile( + snapshot_id=current_snapshot_id, + statistics_path="s3://bucket/warehouse/new-stats.puffin", + file_size_in_bytes=124, + file_footer_size_in_bytes=27, + blob_metadata=[ + BlobMetadata( + type="apache-datasketches-theta-v1", + snapshot_id=current_snapshot_id, + sequence_number=2, + fields=[1], + ) + ], + ) + + transaction = table_v2_with_statistics.transaction() + transaction.update_statistics().set_statistics(statistics_file).remove_statistics(previous_snapshot_id).commit() + + # Both operations in the chain must be staged, in order: set_statistics must not be + # discarded by the remove_statistics call that follows it. + assert transaction._updates == ( # pylint: disable=W0212 + SetStatisticsUpdate(statistics=statistics_file), + RemoveStatisticsUpdate(snapshot_id=previous_snapshot_id), + ) + assert transaction.table_metadata.statistics == [statistics_file] + + def test_set_partition_statistics_update(table_v2_with_statistics: Table) -> None: snapshot_id = table_v2_with_statistics.metadata.current_snapshot_id From 139a846ee1d2b1377c5111d2a973fa4290ce9de7 Mon Sep 17 00:00:00 2001 From: Anshul Mishra Date: Wed, 7 Oct 2026 19:47:42 +0530 Subject: [PATCH 2/2] test: assert fixture statistics and final metadata in chained update test Make the precondition explicit: the fixture already has statistics for both snapshots, which is why remove_statistics targets the previous one. Assert on the resulting table metadata only, instead of the private staged updates. --- tests/table/test_init.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/table/test_init.py b/tests/table/test_init.py index e6a86db350..4dc271092e 100644 --- a/tests/table/test_init.py +++ b/tests/table/test_init.py @@ -1868,15 +1868,16 @@ def test_update_statistics_set_then_remove_keeps_both_updates(table_v2_with_stat ], ) + # The fixture already has statistics for both snapshots + assert {stats.snapshot_id for stats in table_v2_with_statistics.metadata.statistics} == { + current_snapshot_id, + previous_snapshot_id, + } + transaction = table_v2_with_statistics.transaction() transaction.update_statistics().set_statistics(statistics_file).remove_statistics(previous_snapshot_id).commit() - # Both operations in the chain must be staged, in order: set_statistics must not be - # discarded by the remove_statistics call that follows it. - assert transaction._updates == ( # pylint: disable=W0212 - SetStatisticsUpdate(statistics=statistics_file), - RemoveStatisticsUpdate(snapshot_id=previous_snapshot_id), - ) + # set_statistics replaces the current snapshot's statistics, remove_statistics drops the previous one assert transaction.table_metadata.statistics == [statistics_file]