You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "paultmathew (via GitHub)" <gi...@apache.org> on 2026/05/06 22:08:49 UTC

[PR] fix(tests): isolate state in test_write_optional_list [iceberg-python]

paultmathew opened a new pull request, #3334:
URL: https://github.com/apache/iceberg-python/pull/3334

   # Rationale for this change
   
   `test_write_optional_list` uses `create_table_if_not_exists` and then asserts exact row counts (2 after the first append, 4 after the second). The table is reused across runs, so a second invocation against the same docker-compose stack accumulates rows — counts become 6, 10, 14, … and the asserts fail.
   
   CI hides this because it spins up a fresh `docker-compose-integration` stack for every job. Locally it bites anyone who runs `make test-integration-exec` twice without `make test-integration-cleanup` in between.
   
   Fix: drop the table at the start so the test always starts from a clean slate, matching the `drop-then-create` pattern used by `_create_table()` for every other write test in this module.
   
   # Are these changes tested?
   
   Yes — verified locally that the test now passes when run twice back-to-back without resetting the docker stack:
   
   ```bash
   docker compose -f dev/docker-compose-integration.yml up -d --wait
   uv run python dev/provision.py
   uv run python -m pytest tests/integration/test_writes/test_writes.py::test_write_optional_list -m integration  # PASS
   uv run python -m pytest tests/integration/test_writes/test_writes.py::test_write_optional_list -m integration  # PASS (was previously: 8 == 2)
   ```
   
   Full `tests/integration/test_writes/test_writes.py` suite: 122 passed, 1 skipped.
   
   # Are there any user-facing changes?
   
   No.
   
   ---
   
   Noticed while preparing the PR for #2152 (`RecordBatchReader` streaming append).


-- 
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(tests): isolate state in test_write_optional_list [iceberg-python]

Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3334:
URL: https://github.com/apache/iceberg-python/pull/3334#discussion_r3216332870


##########
tests/integration/test_writes/test_writes.py:
##########
@@ -2027,7 +2027,15 @@ def test_write_optional_list(session_catalog: Catalog) -> None:
             required=False,
         ),
     )
-    session_catalog.create_table_if_not_exists(identifier, schema)
+    # Ensure a clean slate: the test asserts exact row counts after each
+    # append, so a stale table from a previous run would accumulate rows and
+    # fail. Other tests in this module use _create_table() which does the same
+    # drop-if-exists step.
+    try:
+        session_catalog.drop_table(identifier=identifier)
+    except NoSuchTableError:
+        pass
+    session_catalog.create_table(identifier, schema)

Review Comment:
   Thanks!



-- 
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(tests): isolate state in test_write_optional_list [iceberg-python]

Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #3334:
URL: https://github.com/apache/iceberg-python/pull/3334#issuecomment-4417573554

   Thanks for cleaning this up @paultmathew 🚀 


-- 
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(tests): isolate state in test_write_optional_list [iceberg-python]

Posted by "paultmathew (via GitHub)" <gi...@apache.org>.
paultmathew commented on code in PR #3334:
URL: https://github.com/apache/iceberg-python/pull/3334#discussion_r3201928520


##########
tests/integration/test_writes/test_writes.py:
##########
@@ -2027,7 +2027,15 @@ def test_write_optional_list(session_catalog: Catalog) -> None:
             required=False,
         ),
     )
-    session_catalog.create_table_if_not_exists(identifier, schema)
+    # Ensure a clean slate: the test asserts exact row counts after each
+    # append, so a stale table from a previous run would accumulate rows and
+    # fail. Other tests in this module use _create_table() which does the same
+    # drop-if-exists step.
+    try:
+        session_catalog.drop_table(identifier=identifier)
+    except NoSuchTableError:
+        pass
+    session_catalog.create_table(identifier, schema)

Review Comment:
   Done. Switched to _create_table(). Diff is now a one-liner. Thanks for the cleanup pointer!



-- 
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(tests): isolate state in test_write_optional_list [iceberg-python]

Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #3334:
URL: https://github.com/apache/iceberg-python/pull/3334


-- 
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(tests): isolate state in test_write_optional_list [iceberg-python]

Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3334:
URL: https://github.com/apache/iceberg-python/pull/3334#discussion_r3201523094


##########
tests/integration/test_writes/test_writes.py:
##########
@@ -2027,7 +2027,15 @@ def test_write_optional_list(session_catalog: Catalog) -> None:
             required=False,
         ),
     )
-    session_catalog.create_table_if_not_exists(identifier, schema)
+    # Ensure a clean slate: the test asserts exact row counts after each
+    # append, so a stale table from a previous run would accumulate rows and
+    # fail. Other tests in this module use _create_table() which does the same
+    # drop-if-exists step.
+    try:
+        session_catalog.drop_table(identifier=identifier)
+    except NoSuchTableError:
+        pass
+    session_catalog.create_table(identifier, schema)

Review Comment:
   How about re-using the `_create_table` util: 
   
   https://github.com/apache/iceberg-python/blob/d008a04a7cf8ec6c73855285117dca1ad25b35d1/tests/integration/test_writes/utils.py#L62-L81



-- 
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