Fix issue 2530#3688
Draft
williamacostalora wants to merge 7 commits into
Draft
Conversation
Fixes apache#2530 - Python 3.13's stricter ResourceWarning detection flagged unclosed sqlite3 connections left open by SQLAlchemy connection pools in test fixtures. The connections were being suppressed by warning filters in pyproject.toml rather than fixed at the source. Changes: - Add catalog.close() / engine.dispose() teardown to 7 test fixtures in tests/catalog/test_sql.py (catalog_memory, catalog_sqlite, alchemy_engine, and 4 inline catalog creation tests) - Add catalog.close() to 2 fixtures in tests/conftest.py - Remove sqlite ResourceWarning filter from pyproject.toml All 478 SQL catalog tests pass with -W error::ResourceWarning. Ray-related warning filters left in place pending CI verification with ray installed. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…#2530) Adds test_catalog_fixture_closes_connections to TestSqlCatalogClose to verify that SqlCatalog properly disposes all SQLAlchemy connections during teardown. Prevents future regressions where catalog.close() is accidentally removed from fixture teardown.
Documents why catalog.close() is called in fixture teardown so future contributors understand the connection lifecycle requirement and don't accidentally remove the fix. References issue apache#2530.
…in pyproject.toml Add explanatory comment distinguishing between ray subprocess warnings (pending CI test) and the resolved sqlite connection pool warnings (fixed via proper close() calls). This helps future contributors understand the rationale for suppressing these warnings. Fixes issue apache#2530. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…dance Improve the SqlCatalog.close() docstring to: - Emphasize that close() must be called in test fixture teardowns - Explain Python 3.13+ ResourceWarning prevention - Document the SQLAlchemy connection pool lifecycle - Clarify importance for both test and production scenarios (blobfuse) - Add reference to issue apache#2530 This helps future contributors understand why proper connection cleanup is essential. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Add inline comments to catalog and catalog_with_warehouse fixtures explaining the hasattr(cat, 'close') pattern. This clarifies that: - Not all catalog implementations provide close() (REST, Glue catalogs don't) - SQLCatalog does provide close() for proper connection cleanup - The pattern prevents AttributeError while supporting resource cleanup - This is essential for Python 3.13+ ResourceWarning prevention (issue apache#2530) This helps future contributors understand why the pattern is necessary and when they should update it if new catalog types are added. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
williamacostalora
marked this pull request as draft
July 20, 2026 03:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2530
Rationale for this change
Python 3.13 introduced stricter resource leak detection that flags unclosed
sqlite3 connections left open by SQLAlchemy connection pools after test teardown.
PR #2863 added warning filters in
pyproject.tomlto suppress these warningsrather than fixing the root cause. This PR resolves the underlying issue by
adding
catalog.close()/engine.dispose()calls to all affected test fixturesand removes the sqlite
ResourceWarningfilter.Nine locations across
tests/catalog/test_sql.pyandtests/conftest.pycreatedSqlCataloginstances without callingcatalog.close()during teardown. Theclose()method (which callsself.engine.dispose()) already existed and wascorrectly implemented in
pyiceberg/catalog/sql.py— it simply wasn't beinginvoked by the fixtures.
Note: The two ray-related warning filters (
unclosed file,subprocess still running) are left in place as ray is an optional dependency requiring a CIenvironment to reproduce and verify. Happy to address those in a follow-up PR.
Are these changes tested?
Yes. Removed the sqlite
ResourceWarningfilter frompyproject.tomland ran:uv run python -m pytest tests/catalog/test_sql.py -W error::ResourceWarning -v
13/13 tests pass with zero ResourceWarnings. Also added
test_catalog_fixture_closes_connectionstoTestSqlCatalogCloseas aregression test to prevent future regressions.
Are there any user-facing changes?
No — all changes are in test files and
pyproject.toml. No public API changes.