Skip to content

Commit dd56273

Browse files
committed
test: count table_metadata reads, not recomputes, in the hoist guard
The guard asserted that _MergeAppendFiles.__init__ triggers exactly one more update_table_metadata call than its superclass. Recompute count is a proxy for read count that the cache breaks: repeated reads of an unchanged state collapse to one recompute, so an un-hoisting became invisible and the assertion had to be relaxed to an upper bound. Counting property reads directly restores the original assertion and makes the guard orthogonal to caching. Verified both ways: it passes with and without the cache, and un-hoisting __init__ back to three separate reads fails it (8 - 5). Confidence: high Not-tested: only the _MergeAppendFiles path was mutation-probed; the _summary() assertions were left as-is beyond the oracle swap.
1 parent b2cb225 commit dd56273

1 file changed

Lines changed: 26 additions & 17 deletions

File tree

tests/table/test_snapshots.py

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -616,37 +616,46 @@ def test_snapshot_producer_bounded_metadata_access(table_v2: Table) -> None:
616616
"""
617617
from unittest import mock
618618

619-
from pyiceberg.table.update import update_table_metadata
619+
from pyiceberg.table import Transaction
620+
from pyiceberg.table.metadata import TableMetadata
620621
from pyiceberg.table.update.snapshot import _FastAppendFiles, _MergeAppendFiles
621622

622623
def make_file() -> DataFile:
623624
return DataFile.from_args(content=DataFileContent.DATA, record_count=1, file_size_in_bytes=1, partition=Record())
624625

625626
txn = table_v2.transaction()
626627

627-
with mock.patch("pyiceberg.table.update_table_metadata", wraps=update_table_metadata) as spy:
628+
# Counts property reads rather than update_table_metadata calls: hoisting removes reads,
629+
# and a read that the cache serves is still a read the hoisting was meant to remove.
630+
reads = [0]
631+
fget = Transaction.__dict__["table_metadata"].fget
632+
633+
def counting(self: Transaction) -> TableMetadata:
634+
reads[0] += 1
635+
return fget(self)
636+
637+
with mock.patch.object(Transaction, "table_metadata", property(counting)):
628638
# _summary() cost must not scale with the number of data files
629-
def summary_calls(n_files: int) -> int:
639+
def summary_reads(n_files: int) -> int:
630640
append = _FastAppendFiles(operation=Operation.APPEND, transaction=txn, io=table_v2.io)
631641
for _ in range(n_files):
632642
append.append_data_file(make_file())
633-
spy.reset_mock()
643+
reads[0] = 0
634644
append._summary()
635-
return spy.call_count
645+
return reads[0]
636646

637-
few, many = summary_calls(10), summary_calls(100)
638-
assert few == many, f"_summary() update_table_metadata calls scale with file count ({few} vs {many})"
639-
assert many <= 2, f"_summary() triggered {many} update_table_metadata calls; expected O(1)"
647+
few, many = summary_reads(10), summary_reads(100)
648+
assert few == many, f"_summary() table_metadata reads scale with file count ({few} vs {many})"
649+
assert many <= 2, f"_summary() made {many} table_metadata reads; expected O(1)"
640650

641-
# _MergeAppendFiles.__init__ should add exactly one call over _FastAppendFiles.__init__
642-
spy.reset_mock()
651+
# _MergeAppendFiles.__init__ should add exactly one read over _FastAppendFiles.__init__
652+
reads[0] = 0
643653
_FastAppendFiles(operation=Operation.APPEND, transaction=txn, io=table_v2.io)
644-
fast_init = spy.call_count
645-
spy.reset_mock()
654+
fast_init = reads[0]
655+
reads[0] = 0
646656
_MergeAppendFiles(operation=Operation.APPEND, transaction=txn, io=table_v2.io)
647-
merge_init = spy.call_count
648-
# Upper bound, not equality: the cache absorbs this access when the staged state is unchanged.
649-
assert merge_init - fast_init <= 1, (
650-
f"_MergeAppendFiles.__init__ made {merge_init - fast_init} extra update_table_metadata "
651-
"calls over its superclass; expected at most 1"
657+
merge_init = reads[0]
658+
assert merge_init - fast_init == 1, (
659+
f"_MergeAppendFiles.__init__ made {merge_init - fast_init} extra table_metadata "
660+
"reads over its superclass; expected 1 (hoisted)"
652661
)

0 commit comments

Comments
 (0)