Readability pass on replace tests

Drop docstrings that restated the function name or assertions, drop
narrative inline comments where variable names already carry the
meaning, and tighten multi-paragraph docstrings to one line. Also
shorten the integration test function names to match how the rest
of the file is structured.

Author: smaheshwar-pltr

tests/catalog/test_catalog_behaviors.py

@@ -417,7 +417,6 @@ def _simple_data(num_rows: int = 2) -> pa.Table:
 
 
 def test_replace_table_preserves_uuid_and_clears_current_snapshot(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """Replace preserves table_uuid and snapshot history, but clears the main ref."""
     _create_simple_table(catalog, test_table_identifier)
     original = catalog.load_table(test_table_identifier)
     original.append(_simple_data())
@@ -441,9 +440,7 @@ def test_replace_table_preserves_uuid_and_clears_current_snapshot(catalog: Catal
 @pytest.mark.parametrize(
     "extra_fields, expected_schema_ids, expected_current, expected_fields, expected_last_col_id",
     [
-        # Identical to the existing schema → reuse the same schema_id, no new schema appended.
         ([], [0], 0, {"id": 1, "data": 2}, 2),
-        # Extra column → append a new schema; existing IDs preserved by name, new one above last_column_id.
         (
             [NestedField(field_id=99, name="extra", field_type=BooleanType(), required=False)],
             [0, 1],
@@ -463,7 +460,6 @@ def test_replace_table_schema_id_reuse(
     expected_fields: dict[str, int],
     expected_last_col_id: int,
 ) -> None:
-    """A structurally identical schema reuses its schema_id; an extended schema adds a new one."""
     _, base_schema = _create_simple_table(catalog, test_table_identifier)
     new_schema = Schema(*base_schema.fields, *extra_fields)
     replaced = catalog.replace_table(test_table_identifier, schema=new_schema)
@@ -475,7 +471,6 @@ def test_replace_table_schema_id_reuse(
 
 
 def test_replace_table_preserves_identifier_field_ids(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """Identifier-field IDs on the new schema are honored after reuse-by-name."""
     schema = Schema(
         NestedField(field_id=1, name="id", field_type=LongType(), required=True),
         NestedField(field_id=2, name="data", field_type=StringType(), required=False),
@@ -493,8 +488,6 @@ def test_replace_table_preserves_identifier_field_ids(catalog: Catalog, test_tab
 
 
 def test_replace_table_drops_identifier_field(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """Replacing with a schema that has no `identifier_field_ids` clears them on the table —
-    the previous identifier set is not silently carried forward."""
     schema_with_id = Schema(
         NestedField(field_id=1, name="id", field_type=LongType(), required=True),
         NestedField(field_id=2, name="data", field_type=StringType(), required=False),
@@ -510,7 +503,6 @@ def test_replace_table_drops_identifier_field(catalog: Catalog, test_table_ident
 
 
 def test_replace_table_reuses_partition_spec_id(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """An identical partition spec reuses its spec_id rather than appending a new one."""
     spec = PartitionSpec(PartitionField(source_id=1, field_id=1000, name="id_part", transform=IdentityTransform()))
     _, schema = _create_simple_table(catalog, test_table_identifier, partition_spec=spec)
     replaced = catalog.replace_table(test_table_identifier, schema=schema, partition_spec=spec)
@@ -519,30 +511,23 @@ def test_replace_table_reuses_partition_spec_id(catalog: Catalog, test_table_ide
 
 
 def test_replace_table_with_sort_order_changes(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """Replace can change the sort order. The new sort order is appended to the history and
-    becomes the default; a follow-up replace back to a sort already in history reuses its
-    order_id rather than appending a duplicate."""
     _, schema = _create_simple_table(catalog, test_table_identifier)
     sort = SortOrder(SortField(source_id=1, transform=IdentityTransform(), direction=SortDirection.ASC))
 
-    # unsorted → sorted: a new order is added and becomes the default.
     sorted_table = catalog.replace_table(test_table_identifier, schema=schema, sort_order=sort)
     assert sorted_table.sort_order().fields == sort.fields
     sorted_order_id = sorted_table.metadata.default_sort_order_id
     assert sorted_order_id != 0
 
-    # sorted → unsorted: reuses the unsorted order_id 0 from history.
     unsorted_table = catalog.replace_table(test_table_identifier, schema=schema)
     assert unsorted_table.sort_order().is_unsorted
     assert unsorted_table.metadata.default_sort_order_id == 0
 
-    # unsorted → original sorted form: reuses the existing sorted order_id from history.
     replayed = catalog.replace_table(test_table_identifier, schema=schema, sort_order=sort)
     assert replayed.metadata.default_sort_order_id == sorted_order_id
 
 
 def test_replace_table_inherits_existing_location(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """`location=None` keeps the existing table's location."""
     _, schema = _create_simple_table(catalog, test_table_identifier)
     existing = catalog.load_table(test_table_identifier).metadata.location
     replaced = catalog.replace_table(test_table_identifier, schema=schema)
@@ -553,7 +538,6 @@ def test_replace_table_inherits_existing_location(catalog: Catalog, test_table_i
 def test_replace_table_uses_explicit_location(
     catalog: Catalog, test_table_identifier: Identifier, tmp_path: Path, trailing_slash: bool
 ) -> None:
-    """An explicit `location` is used verbatim; trailing slash is stripped."""
     _, schema = _create_simple_table(catalog, test_table_identifier)
     bare = f"file://{tmp_path}/relocated"
     arg = bare + "/" if trailing_slash else bare
@@ -564,7 +548,6 @@ def test_replace_table_uses_explicit_location(
 def test_replace_table_merges_properties_with_overrides_and_additions(
     catalog: Catalog, test_table_identifier: Identifier
 ) -> None:
-    """Properties are merged onto existing: new values override, untouched keys are preserved."""
     schema = Schema(NestedField(field_id=1, name="id", field_type=LongType(), required=False))
     _create_simple_table(catalog, test_table_identifier, schema=schema, properties={"keep": "yes", "override": "old"})
     replaced = catalog.replace_table(test_table_identifier, schema=schema, properties={"override": "new", "new_key": "v"})
@@ -574,7 +557,6 @@ def test_replace_table_merges_properties_with_overrides_and_additions(
 
 
 def test_replace_table_upgrades_format_version(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """`properties={'format-version': '2'}` on a v1 table emits an UpgradeFormatVersion update."""
     _, schema = _create_simple_table(catalog, test_table_identifier, format_version=1)
     assert catalog.load_table(test_table_identifier).format_version == 1
     replaced = catalog.replace_table(test_table_identifier, schema=schema, properties={"format-version": "2"})
@@ -584,48 +566,40 @@ def test_replace_table_upgrades_format_version(catalog: Catalog, test_table_iden
 
 
 def test_replace_table_rejects_format_version_downgrade(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """A `format-version` lower than the existing one must be rejected to avoid silently
-    running the schema-conversion path with the wrong semantics."""
     _, schema = _create_simple_table(catalog, test_table_identifier, format_version=2)
     with pytest.raises(ValueError, match="Cannot downgrade format-version"):
         catalog.replace_table(test_table_identifier, schema=schema, properties={"format-version": "1"})
 
 
 def test_replace_table_v1_carries_forward_partition_fields_as_void(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """v1 specs are append-only; dropped partition fields must be carried forward as VoidTransform."""
+    """v1 specs are append-only; dropped partition fields are carried forward as VoidTransform."""
     spec = PartitionSpec(PartitionField(source_id=1, field_id=1000, name="id_part", transform=IdentityTransform()))
     _, schema = _create_simple_table(catalog, test_table_identifier, partition_spec=spec, format_version=1)
 
-    # Replace with default unpartitioned spec — the old field must be preserved as void.
     replaced = catalog.replace_table(test_table_identifier, schema=schema)
     new_spec = replaced.spec()
     void_field = next(f for f in new_spec.fields if f.field_id == 1000)
     assert isinstance(void_field.transform, VoidTransform)
     assert void_field.source_id == 1
-    # Name is preserved when there's no collision in the new spec (the new spec is empty here).
     assert void_field.name == "id_part"
 
 
 def test_replace_table_v2_does_not_carry_forward_void_field(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """v2 specs are not append-only — a replace that drops a partition field does not
-    carry it forward (unlike v1). The new default spec contains only the new field(s)."""
+    """v2 specs are not append-only — a dropped partition field is not carried forward (unlike v1)."""
     spec = PartitionSpec(PartitionField(source_id=1, field_id=1000, name="id_part", transform=IdentityTransform()))
     _, schema = _create_simple_table(catalog, test_table_identifier, partition_spec=spec, format_version=2)
 
-    # Replace with default unpartitioned spec — the old field is gone from the default spec.
     replaced = catalog.replace_table(test_table_identifier, schema=schema)
     new_spec = replaced.spec()
     assert new_spec.is_unpartitioned()
     assert all(not isinstance(f.transform, VoidTransform) for f in new_spec.fields)
 
 
 def test_replace_after_format_version_upgrade(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """A v1 table can be upgraded to v2 via replace and then re-replaced without issue."""
     _, schema = _create_simple_table(catalog, test_table_identifier, format_version=1)
     upgraded = catalog.replace_table(test_table_identifier, schema=schema, properties={"format-version": "2"})
     assert upgraded.format_version == 2
 
-    # Second replace on the now-v2 table should not re-trigger an upgrade or fail.
     new_schema = Schema(
         NestedField(field_id=1, name="id", field_type=LongType(), required=False),
         NestedField(field_id=2, name="data", field_type=StringType(), required=False),
@@ -643,7 +617,6 @@ def test_replace_table_raises_when_table_does_not_exist(catalog: Catalog, test_t
 
 
 def test_replace_table_transaction_can_stage_additional_changes(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """The transaction context lets callers stage extra updates (e.g. properties) before commit."""
     _, schema = _create_simple_table(catalog, test_table_identifier)
     with catalog.replace_table_transaction(test_table_identifier, schema=schema) as txn:
         txn.set_properties({"staged": "yes"})
@@ -652,12 +625,6 @@ def test_replace_table_transaction_can_stage_additional_changes(catalog: Catalog
 
 
 def test_replace_table_transaction_with_write_atomic_rtas(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """RTAS (Replace Table As Select): replace + write new data in one transaction.
-
-    The new schema and new data must land atomically. After commit:
-    - the new snapshot is current (main ref restored by the fast_append),
-    - the new snapshot's parent is None (no lineage to the pre-replace data), and
-    - the pre-replace snapshot is preserved in history for time-travel."""
     _create_simple_table(catalog, test_table_identifier)
     catalog.load_table(test_table_identifier).append(_simple_data(num_rows=1))
     old_snapshot_id = catalog.load_table(test_table_identifier).current_snapshot().snapshot_id  # type: ignore[union-attr]
@@ -680,9 +647,6 @@ def test_replace_table_transaction_with_write_atomic_rtas(catalog: Catalog, test
 
 
 def test_replace_table_transaction_rolls_back_on_failure(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """If the body of the transaction raises, no metadata change is committed.
-
-    The table's UUID, schemas, current snapshot, and current schema id must all be unchanged."""
     _create_simple_table(catalog, test_table_identifier)
     catalog.load_table(test_table_identifier).append(_simple_data())
     before = catalog.load_table(test_table_identifier).metadata
@@ -708,17 +672,13 @@ def run_failing_replace() -> None:
 
 
 def test_concurrent_replace_table(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """Two concurrent replace_table calls staged from the same base both adding a column
-    must fail on the second commit with an `assert-last-assigned-field-id` violation —
-    proving the `AssertLastAssignedFieldId` requirement actually guards against duplicate
-    field-id assignment under concurrent writers."""
+    """`AssertLastAssignedFieldId` rejects the second of two replaces staged from the same base."""
     _create_simple_table(catalog, test_table_identifier)
     new_schema = Schema(
         NestedField(field_id=1, name="id", field_type=LongType(), required=False),
         NestedField(field_id=2, name="data", field_type=StringType(), required=False),
         NestedField(field_id=3, name="extra", field_type=BooleanType(), required=False),
     )
-    # Both transactions build from the same base metadata with the same new schema.
     txn_a = catalog.replace_table_transaction(test_table_identifier, schema=new_schema)
     txn_b = catalog.replace_table_transaction(test_table_identifier, schema=new_schema)
 
@@ -728,8 +688,6 @@ def test_concurrent_replace_table(catalog: Catalog, test_table_identifier: Ident
 
 
 def test_replace_table_allows_subsequent_append(catalog: Catalog, test_table_identifier: Identifier) -> None:
-    """After `replace_table` clears the current snapshot, a separate `append` produces a new
-    snapshot containing only the post-replace data — the pre-replace rows are not visible."""
     _, schema = _create_simple_table(catalog, test_table_identifier)
     catalog.load_table(test_table_identifier).append(_simple_data(num_rows=3))
 

tests/catalog/test_rest.py

@@ -2927,8 +2927,6 @@ def test_replace_table_transaction_wire_payload(
     example_table_metadata_with_snapshot_v1_rest_json: dict[str, Any],
     example_table_metadata_no_snapshot_v1_rest_json: dict[str, Any],
 ) -> None:
-    """The replace request must carry exactly one `assert-table-uuid` requirement and the
-    Set* + Add* updates needed to transform the existing table — order is not asserted."""
     table_uuid = example_table_metadata_with_snapshot_v1_rest_json["metadata"]["table-uuid"]
     example_table_metadata_no_snapshot_v1_rest_json["metadata"]["table-uuid"] = table_uuid
     _mock_replace_endpoints(
@@ -2948,31 +2946,22 @@ def test_replace_table_transaction_wire_payload(
     catalog.replace_table_transaction(identifier=("fokko", "fokko2"), schema=new_schema).commit_transaction()
     request = rest_mock.last_request.json()
 
-    # Pin the requirement values to the fixture's last-column-id / last-partition-id so the
-    # assertion fails loudly if the fixture changes underneath us.
     fixture_metadata = example_table_metadata_with_snapshot_v1_rest_json["metadata"]
     assert request["requirements"] == [
         {"type": "assert-table-uuid", "uuid": table_uuid},
         {"type": "assert-last-assigned-field-id", "last-assigned-field-id": fixture_metadata["last-column-id"]},
         {"type": "assert-last-assigned-partition-id", "last-assigned-partition-id": fixture_metadata["last-partition-id"]},
     ]
 
-    # No duplicate actions in the payload (the dict-by-action collapses them — guard against it).
     actions = [u["action"] for u in request["updates"]]
     assert len(actions) == len(set(actions)), f"duplicate actions in request: {actions}"
     updates_by_action = {u["action"]: u for u in request["updates"]}
 
-    # main snapshot must be cleared on replace.
     assert updates_by_action["remove-snapshot-ref"] == {"action": "remove-snapshot-ref", "ref-name": "main"}
-    # A new schema is added because the column set differs; field IDs for existing columns
-    # are reused by name. The highest id (3) flows from the schema's fields, which the
-    # server uses to bump the table's last-column-id on the server side.
     added_schema = updates_by_action["add-schema"]["schema"]
     assert {f["name"]: f["id"] for f in added_schema["fields"]} == {"id": 1, "data": 2, "new_col": 3}
-    # Set* updates are emitted unconditionally even when their resulting id is reused.
     # schema-id=-1 is the wire sentinel meaning "the schema we just added in this commit".
     assert updates_by_action["set-current-schema"]["schema-id"] == -1
-    # Spec/sort-order are reused from the fixture (default ids) since the replace doesn't change them.
     assert updates_by_action["set-default-spec"]["spec-id"] == fixture_metadata["default-spec-id"]
     assert updates_by_action["set-default-sort-order"]["sort-order-id"] == fixture_metadata["default-sort-order-id"]
 
@@ -2999,9 +2988,7 @@ def test_replace_table_issues_commit_post_immediately(
     example_table_metadata_with_snapshot_v1_rest_json: dict[str, Any],
     example_table_metadata_no_snapshot_v1_rest_json: dict[str, Any],
 ) -> None:
-    """`replace_table` issues a commit POST during the call — distinguishing it from
-    `replace_table_transaction(...)` alone, which only issues the load GET (no POST until
-    the caller commits the transaction)."""
+    """`replace_table` commits during the call; `replace_table_transaction` defers the POST until the caller commits."""
     table_uuid = example_table_metadata_with_snapshot_v1_rest_json["metadata"]["table-uuid"]
     example_table_metadata_no_snapshot_v1_rest_json["metadata"]["table-uuid"] = table_uuid
     _mock_replace_endpoints(
@@ -3017,8 +3004,6 @@ def test_replace_table_issues_commit_post_immediately(
         NestedField(field_id=2, name="data", field_type=StringType(), required=False),
     )
 
-    # Baseline: opening the transaction alone makes only the load GET call; no POST is
-    # issued until the caller commits.
     catalog.replace_table_transaction(identifier=("fokko", "fokko2"), schema=new_schema)
     methods_after_open = [r.method for r in rest_mock.request_history]
     assert "POST" not in methods_after_open

tests/integration/test_rest_catalog.py

@@ -74,12 +74,8 @@ def test_create_namespace_if_already_existing(catalog: RestCatalog) -> None:
 
 @pytest.mark.integration
 @pytest.mark.parametrize("catalog", [lf("session_catalog")])
-def test_replace_table_end_to_end_against_rest_server(catalog: Catalog) -> None:
-    """End-to-end smoke test: replace_table against a real REST server.
-
-    Detailed replace_table semantics are covered against InMemoryCatalog and SqlCatalog in
-    `tests/catalog/test_catalog_behaviors.py`. This test verifies the REST wire path: server
-    accepts the commit, preserves the UUID, and clears the current snapshot."""
+def test_replace_table(catalog: Catalog) -> None:
+    """End-to-end smoke test: replace_table against a real REST server."""
     identifier = f"default.test_replace_table_e2e_{catalog.name}"
     if not catalog.namespace_exists("default"):
         catalog.create_namespace("default")
@@ -115,9 +111,8 @@ def test_replace_table_end_to_end_against_rest_server(catalog: Catalog) -> None:
 
 @pytest.mark.integration
 @pytest.mark.parametrize("catalog", [lf("session_catalog")])
-def test_replace_table_transaction_rtas_against_rest_server(catalog: Catalog) -> None:
-    """RTAS (Replace Table As Select) against a real REST server: the schema swap and the
-    new-data write must land atomically — the new snapshot is current on commit."""
+def test_replace_table_transaction(catalog: Catalog) -> None:
+    """RTAS against a real REST server: the schema swap and the new-data write land atomically."""
     identifier = f"default.test_replace_rtas_{catalog.name}"
     if not catalog.namespace_exists("default"):
         catalog.create_namespace("default")