(texera) branch gh-readonly-queue/main/pr-8048-8b3aefec9dadd2933fa63b53c05e6a152f435175 created (now 761a0d8c1f)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-8048-8b3aefec9dadd2933fa63b53c05e6a152f435175
in repository https://gitbox.apache.org/repos/asf/texera.git


      at 761a0d8c1f test(pyamber): cover IcebergDocument uri, count and clear paths (#8048)

This branch includes the following new commits:

     new 761a0d8c1f test(pyamber): cover IcebergDocument uri, count and clear paths (#8048)

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

(texera) 01/01: test(pyamber): cover IcebergDocument uri, count and clear paths (#8048)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8048-8b3aefec9dadd2933fa63b53c05e6a152f435175
in repository https://gitbox.apache.org/repos/asf/texera.git

commit 761a0d8c1f97e6b025b4be5000ee6caad2819dfe
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Fri Aug 28 02:42:05 2026 +0000

    test(pyamber): cover IcebergDocument uri, count and clear paths (#8048)
    
    ### What changes were proposed in this PR?
    
    A new `TestIcebergDocumentWithMockCatalog` class in
    `test_iceberg_document.py`, taking the file from 9 collected items to
    22.
    
    | Metric | Before | After |
    |---|---|---|
    | Codecov fully-covered lines **today** | 118/124 | **124/124** |
    | Codecov fully-covered lines **after #8040** | 115/124 | **124/124** |
    | Branch arms | 21/26 | **26/26** |
    
    **`iceberg_document.py` reaches 100% on both lines and branches.** +6
    fully-covered lines today, +9 after #8040 — lines 90, 108 and 189 are
    `50% (1/2)` partials, invisible to the current branch-less report.
    
    The nine lines were exactly {79, 80, 81, 84, 90, 108, 109, 189, 190}:
    `get_uri`, `get_count`, `clear`, and the `get_after` skip guard.
    
    ### The measurement problem, and how it was handled
    
    The existing `TestIcebergDocument` needs a live postgres Iceberg
    catalog. CI supplies one; on this machine the `texera` role cannot read
    `iceberg_tables`, so **all 9 of those tests fail locally** and a raw
    local coverage run under-reports the file badly — enough to suggest ~50
    winnable lines instead of 9.
    
    A throwaway sqlite-backed `SqlCatalog` proxy was used to reproduce the
    same code paths, applied identically to both sides and then deleted. The
    honest gap is 9.
    
    **The new tests need none of that.** They patch
    `IcebergCatalogInstance.get_instance` rather than mutating it, so they
    touch no process-global state and need no postgres, filesystem or
    network. They run in 1.25s.
    
    ### Two hazards found in the existing fixture
    
    - **`StorageConfig.initialize` is a process-wide one-shot** that raises
    on a second call, and `test_iceberg_document.py:45` calls it **unguarded
    at module level**. That works only because this module happens to be the
    first module-level initializer imported — two sibling modules guard with
    `if not StorageConfig._initialized`. A new test module initializing at
    module level and sorting before it would break collection for the whole
    suite. Appending a class to the existing file avoids this; a separate
    file would have been a trap.
    - **`IcebergCatalogInstance._catalogs` is a class-level dict** and
    `replace_instance` mutates it for the rest of the session, poisoning
    every later suite in the shared process. The new tests sidestep it by
    patching `get_instance`.
    
    ### Honest discounts
    
    - **`get_uri` and `get_count` have zero callers anywhere in
    `src/main/python`**, and `clear` has none either — the Scala side has
    its own implementation. These are real statements with real logic, not
    `@abstractmethod` stubs, but the value is interface-contract conformance
    rather than live behaviour.
    - **Lines 189–190 need a negative `from_index`.**
    `num_of_skipped_records` is provably 0 when the guard first runs, so the
    only route is `get_after(-1)`. It is drivable through the public API,
    but no production caller produces a negative offset — a defensive-guard
    test. Counted strictly, the bundle is 7 lines and 4 arms.
    - **One mutant survives this class**: `sum(f.file.record_count ...)` to
    `sum(1 ...)` at line 110, killed only by the pre-existing postgres-gated
    `test_get_counts`. Line 110 is already covered in CI so it is not a
    coverage gap, but the kill is not this bundle's and one was not
    manufactured.
    
    ### Verification
    
    Two reviewers returned seven findings; all repaired. **The repair pass
    added zero coverage lines and zero arms** — the builder's bundle already
    reached 124/124. Its entire value was mutation strength: 14 mutants that
    passed the delivered spec now fail. Stated plainly rather than sold as
    more coverage.
    
    Full pyamber suite: `5 failed, 1172 passed, 7 errors` against main's
    `1159 passed`. Failure sets sorted and diffed — **identical by
    identity**, 12 entries. `ruff check` and `ruff format --check` pass on
    CI's scope. The throwaway proxy is deleted and `git status --porcelain`
    is clean.
    
    No production file is touched; `git diff -- 'amber/src/main/*'` is
    empty.
    
    ### Any related issues, documentation, discussions?
    
    Closes #8044
    
    ### How was this PR tested?
    
    ```
    cd amber && python -m pytest -m "not integration" -q src/test/python/core/storage/iceberg/test_iceberg_document.py -k MockCatalog
    ```
    
    ```
    13 passed, 1 warning in 1.25s
    ```
    
    The 9 pre-existing `TestIcebergDocument` tests still require the
    postgres catalog that CI provides.
    
    ### Was this PR authored or co-authored using generative AI tooling?
    
    Generated-by: Claude Code (Opus 5)
---
 .../core/storage/iceberg/test_iceberg_document.py  | 191 +++++++++++++++++++++
 1 file changed, 191 insertions(+)

diff --git a/amber/src/test/python/core/storage/iceberg/test_iceberg_document.py b/amber/src/test/python/core/storage/iceberg/test_iceberg_document.py
index 5e61b4ed68..4e2df0f34c 100644
--- a/amber/src/test/python/core/storage/iceberg/test_iceberg_document.py
+++ b/amber/src/test/python/core/storage/iceberg/test_iceberg_document.py
@@ -22,10 +22,21 @@ import tempfile
 import uuid
 from concurrent.futures import as_completed
 from concurrent.futures.thread import ThreadPoolExecutor
+from unittest.mock import MagicMock, Mock, patch
+from urllib.parse import urlparse
+
+from pyiceberg import types as iceberg_types
+from pyiceberg.schema import Schema as IcebergSchema
 
 from core.models import Schema, Tuple
 from core.models.state import State
 from core.storage.document_factory import DocumentFactory
+from core.storage.iceberg import iceberg_document
+from core.storage.iceberg.iceberg_document import IcebergDocument
+from core.storage.iceberg.iceberg_utils import (
+    amber_tuples_to_arrow_table,
+    arrow_table_to_amber_tuples,
+)
 from core.storage.storage_config import StorageConfig
 from core.storage.vfs_uri_factory import VFSURIFactory
 from proto.org.apache.texera.amber.core import (
@@ -411,3 +422,183 @@ class TestIcebergDocument:
             key=lambda state: state["loop_counter"],
         )
         assert actual_states == states
+
+
+class TestIcebergDocumentWithMockCatalog:
+    """
+    The catalog-facing paths of IcebergDocument that need no catalog service:
+    the table-location lookup, the "table is absent" arms of clear() and
+    get_count(), the locks clear() and the read path take, what each read entry
+    point hands the iterator, and both sides of the iterator's seek guard. The
+    catalog is a mock, so unlike TestIcebergDocument above these run on any host
+    without a live Iceberg catalog, and they mutate no cached catalog state.
+    """
+
+    @pytest.fixture
+    def iceberg_schema(self):
+        return IcebergSchema(
+            iceberg_types.NestedField(
+                field_id=1,
+                name="col-int",
+                field_type=iceberg_types.IntegerType(),
+                required=False,
+            )
+        )
+
+    @pytest.fixture
+    def document(self, iceberg_schema):
+        """
+        An IcebergDocument for `ns.tbl` whose catalog is a mock. `get_instance`
+        is patched only for the duration of the construction, so the real
+        per-warehouse catalog cache is never touched.
+        """
+        catalog = Mock()
+        with patch.object(
+            iceberg_document.IcebergCatalogInstance,
+            "get_instance",
+            return_value=catalog,
+        ):
+            return IcebergDocument(
+                "ns",
+                "tbl",
+                iceberg_schema,
+                amber_tuples_to_arrow_table,
+                arrow_table_to_amber_tuples,
+            )
+
+    def test_get_uri_returns_the_parsed_table_location(self, document):
+        # The location is unique per run and the expectation is derived from it,
+        # so a get_uri that returned a fixed URI instead of parsing the loaded
+        # table's own location could not accidentally match.
+        location = f"file:///warehouse/{uuid.uuid4().hex}/ns.db/tbl"
+        table = Mock()
+        table.location.return_value = location
+
+        with patch.object(
+            iceberg_document, "load_table_metadata", return_value=table
+        ) as load_table_metadata:
+            uri = document.get_uri()
+
+        expected = urlparse(location)
+        assert (uri.scheme, uri.path) == (expected.scheme, expected.path)
+        table.location.assert_called_once_with()
+        assert load_table_metadata.call_args.args == (document.catalog, "ns", "tbl")
+
+    def test_get_uri_rejects_a_table_that_does_not_exist(self, document):
+        with patch.object(iceberg_document, "load_table_metadata", return_value=None):
+            with pytest.raises(Exception, match=r"table ns\.tbl doesn't exist\."):
+                document.get_uri()
+
+    def test_clear_drops_a_table_that_exists(self, document):
+        document.catalog.table_exists.return_value = True
+
+        document.clear()
+
+        document.catalog.drop_table.assert_called_once_with("ns.tbl")
+
+    def test_clear_leaves_an_absent_table_alone(self, document):
+        document.catalog.table_exists.return_value = False
+
+        document.clear()
+
+        document.catalog.table_exists.assert_called_once_with("ns.tbl")
+        document.catalog.drop_table.assert_not_called()
+
+    def test_get_count_is_zero_when_the_table_does_not_exist(self, document):
+        with patch.object(
+            iceberg_document, "load_table_metadata", return_value=None
+        ) as load_table_metadata:
+            assert document.get_count() == 0
+
+        assert load_table_metadata.call_args.args == (document.catalog, "ns", "tbl")
+
+    def test_a_negative_offset_is_rejected_rather_than_read_as_zero(self, document):
+        """
+        IcebergIterator guards its file seek against having already skipped past
+        `from_index`. The only way the guard can fire is a negative offset, since
+        the skip counter is still 0 when the seek generator first runs.
+
+        Neither the guard's message ("seek operation should not be called", which
+        describes a re-entrant seek rather than a bad argument) nor its exception
+        type is pinned: rejecting a negative offset as a ValueError would be the
+        better behaviour, so accepting either type here keeps that fix open while
+        still requiring that the offset is rejected rather than read as zero.
+        """
+        iterator = document.get_after(-1)
+
+        with pytest.raises((RuntimeError, ValueError)):
+            next(iterator)
+
+    @pytest.mark.parametrize("offset", [0, 5])
+    def test_a_non_negative_offset_does_not_trip_the_seek_guard(self, document, offset):
+        """
+        The other side of the seek guard's boundary: a legal offset must reach
+        the table lookup and then end the iteration cleanly, not raise. Without
+        this, the guard's comparison is unconstrained on hosts that cannot run
+        TestIcebergDocument above.
+        """
+        with patch.object(
+            iceberg_document, "load_table_metadata", return_value=None
+        ) as load_table_metadata:
+            with pytest.raises(StopIteration):
+                next(document.get_after(offset))
+
+        assert load_table_metadata.call_args.args == (document.catalog, "ns", "tbl")
+
+    @pytest.mark.parametrize(
+        "read, from_index, until_index, total",
+        [
+            (lambda document: document.get(), 0, None, float("inf")),
+            (lambda document: document.get_range(3, 7), 3, 7, 4),
+            (lambda document: document.get_after(4), 4, None, float("inf")),
+        ],
+    )
+    def test_the_read_entry_points_delegate_to_the_iterator(
+        self, document, read, from_index, until_index, total
+    ):
+        """
+        Each read entry point hands a specific [from, until) range -- and the
+        document's own catalog, table identity, schema and deserde -- to the
+        iterator. The seek generator's body does not run at construction, so
+        this needs no catalog.
+
+        `total` is spelled out per case rather than recomputed from the range,
+        so the expectation does not restate the production formula.
+        """
+        iterator = read(document)
+
+        assert (iterator.from_index, iterator.until_index) == (from_index, until_index)
+        assert iterator.total_records_to_return == total
+        assert (iterator.table_namespace, iterator.table_name) == ("ns", "tbl")
+        assert iterator.catalog is document.catalog
+        assert iterator.table_schema is document.table_schema
+        assert iterator.deserde is document.deserde
+
+    def test_the_read_path_takes_the_shared_read_lock(self, document):
+        """
+        The counterpart of test_clear_takes_the_write_lock: reads must take the
+        shared read lock, so that concurrent reads are not serialised behind
+        each other. MagicMock (not Mock) is required: the lock is used as a
+        context manager.
+        """
+        document.lock = MagicMock()
+
+        document.get()
+
+        document.lock.gen_rlock.assert_called_once_with()
+        document.lock.gen_wlock.assert_not_called()
+
+    def test_clear_takes_the_write_lock(self, document):
+        """
+        clear() drops the table, so it must hold the write lock rather than the
+        shared read lock the readers take. MagicMock (not Mock) is required: the
+        lock is used as a context manager.
+        """
+        document.catalog.table_exists.return_value = True
+        document.lock = MagicMock()
+
+        document.clear()
+
+        document.lock.gen_wlock.assert_called_once_with()
+        document.lock.gen_rlock.assert_not_called()
+        document.catalog.drop_table.assert_called_once_with("ns.tbl")