From 921482fabf615aabeace33590b8373dbd9b3290b Mon Sep 17 00:00:00 2001 From: chalmer lowe Date: Tue, 29 Apr 2025 13:07:26 +0000 Subject: [PATCH 1/9] fix: updates json array registration --- db_dtypes/json.py | 9 ++++-- tests/unit/test_json.py | 68 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/db_dtypes/json.py b/db_dtypes/json.py index 37aad83..6159316 100644 --- a/db_dtypes/json.py +++ b/db_dtypes/json.py @@ -277,5 +277,10 @@ def to_pandas_dtype(self): # Register the type to be included in RecordBatches, sent over IPC and received in -# another Python process. -pa.register_extension_type(JSONArrowType()) +# another Python process. Also handle potential pre-registration +try: + pa.register_extension_type(JSONArrowType()) +except pa.ArrowKeyError: + # Type 'dbjson' might already be registered if the module is reloaded, + # which is okay. + pass diff --git a/tests/unit/test_json.py b/tests/unit/test_json.py index d15cfc7..ca19a09 100644 --- a/tests/unit/test_json.py +++ b/tests/unit/test_json.py @@ -13,6 +13,7 @@ # limitations under the License. import json +import sys import numpy as np import pandas as pd @@ -224,3 +225,70 @@ def test_json_arrow_record_batch(): == '{"null_field":null,"order":{"address":{"city":"Anytown","street":"123 Main St"},"items":["book","pen","computer"],"total":15}}' ) assert s[6] == "null" + + +@pytest.fixture +def cleanup_json_module_for_reload(): + """ + Fixture to ensure db_dtypes.json is registered and then removed + from sys.modules to allow testing the registration except block via reload. + """ + + json_module_name = "db_dtypes.json" + original_module = sys.modules.get(json_module_name) + + # Ensure the type is registered initially (usually by the first import) + try: + # Make sure the module is loaded so the type exists + import db_dtypes.json + + # Explicitly register just in case it wasn't, or was cleaned up elsewhere. + # This might raise ArrowKeyError itself if already registered, which is fine here. + pa.register_extension_type(db_dtypes.json.JSONArrowType()) + except pa.ArrowKeyError: + pass # Already registered is the state we want before the test runs + except ImportError: + pytest.skip("Could not import db_dtypes.json to set up test.") + + # Remove the module from sys.modules so importlib.reload re-executes it + if json_module_name in sys.modules: + del sys.modules[json_module_name] + + yield # Run the test that uses this fixture + + # Cleanup: Put the original module back if it existed + # This helps isolate from other tests that might import db_dtypes.json + if original_module: + sys.modules[json_module_name] = original_module + elif json_module_name in sys.modules: + # If the test re-imported it but it wasn't there originally, remove it + del sys.modules[json_module_name] + + # Note: PyArrow doesn't have a public API to unregister types easily, + # thus we are using the testing pattern of module isolation/reloading. + + +def test_json_arrow_type_reregistration_is_handled(cleanup_json_module_for_reload): + """ + Verify that attempting to re-register JSONArrowType via module reload + is caught by the except block and does not raise an error. + """ + + try: + # Re-importing the module after the fixture removed it from sys.modules + # forces Python to execute the module's top-level code again. + # This includes the pa.register_extension_type call. + import db_dtypes.json # noqa: F401 + + assert ( + True + ), "Module re-import completed without error, except block likely worked." + + except pa.ArrowKeyError: + # If this exception escapes, the except block in db_dtypes/json.py failed. + pytest.fail( + "pa.ArrowKeyError was raised during module reload, " + "indicating the except block failed." + ) + except Exception as e: + pytest.fail(f"An unexpected exception occurred during module reload: {e}") From d82d4017cc340bf1966f705ca6ee1f87f7237ae6 Mon Sep 17 00:00:00 2001 From: chalmer lowe Date: Wed, 30 Apr 2025 13:23:13 +0000 Subject: [PATCH 2/9] Uploading current code for some discussion --- tests/unit/test_json.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/unit/test_json.py b/tests/unit/test_json.py index ca19a09..0f42693 100644 --- a/tests/unit/test_json.py +++ b/tests/unit/test_json.py @@ -247,20 +247,18 @@ def cleanup_json_module_for_reload(): pa.register_extension_type(db_dtypes.json.JSONArrowType()) except pa.ArrowKeyError: pass # Already registered is the state we want before the test runs - except ImportError: - pytest.skip("Could not import db_dtypes.json to set up test.") # Remove the module from sys.modules so importlib.reload re-executes it - if json_module_name in sys.modules: + if json_module_name in sys.modules: # COVERAGE FAIL: 252->255 del sys.modules[json_module_name] yield # Run the test that uses this fixture # Cleanup: Put the original module back if it existed # This helps isolate from other tests that might import db_dtypes.json - if original_module: + if original_module: # COVERAGE FAIL: 259-261 sys.modules[json_module_name] = original_module - elif json_module_name in sys.modules: + elif json_module_name in sys.modules: # COVERAGE FAIL: 261->exit # If the test re-imported it but it wasn't there originally, remove it del sys.modules[json_module_name] @@ -274,21 +272,21 @@ def test_json_arrow_type_reregistration_is_handled(cleanup_json_module_for_reloa is caught by the except block and does not raise an error. """ + # Re-importing the module after the fixture removed it from sys.modules + # forces Python to execute the module's top-level code again. + # This includes the pa.register_extension_type call. + + assert "db_dtypes.json" not in sys.modules try: - # Re-importing the module after the fixture removed it from sys.modules - # forces Python to execute the module's top-level code again. - # This includes the pa.register_extension_type call. import db_dtypes.json # noqa: F401 assert ( True ), "Module re-import completed without error, except block likely worked." - except pa.ArrowKeyError: + except pa.ArrowKeyError: # COVERAGE FAIL: 287-294 # If this exception escapes, the except block in db_dtypes/json.py failed. pytest.fail( "pa.ArrowKeyError was raised during module reload, " "indicating the except block failed." ) - except Exception as e: - pytest.fail(f"An unexpected exception occurred during module reload: {e}") From c8101e096b176227e758fcba4a93555cc8e0d29a Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Wed, 30 Apr 2025 13:29:31 +0000 Subject: [PATCH 3/9] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- tests/unit/test_json.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_json.py b/tests/unit/test_json.py index 0f42693..1e77d73 100644 --- a/tests/unit/test_json.py +++ b/tests/unit/test_json.py @@ -249,16 +249,16 @@ def cleanup_json_module_for_reload(): pass # Already registered is the state we want before the test runs # Remove the module from sys.modules so importlib.reload re-executes it - if json_module_name in sys.modules: # COVERAGE FAIL: 252->255 + if json_module_name in sys.modules: # COVERAGE FAIL: 252->255 del sys.modules[json_module_name] yield # Run the test that uses this fixture # Cleanup: Put the original module back if it existed # This helps isolate from other tests that might import db_dtypes.json - if original_module: # COVERAGE FAIL: 259-261 + if original_module: # COVERAGE FAIL: 259-261 sys.modules[json_module_name] = original_module - elif json_module_name in sys.modules: # COVERAGE FAIL: 261->exit + elif json_module_name in sys.modules: # COVERAGE FAIL: 261->exit # If the test re-imported it but it wasn't there originally, remove it del sys.modules[json_module_name] @@ -275,7 +275,7 @@ def test_json_arrow_type_reregistration_is_handled(cleanup_json_module_for_reloa # Re-importing the module after the fixture removed it from sys.modules # forces Python to execute the module's top-level code again. # This includes the pa.register_extension_type call. - + assert "db_dtypes.json" not in sys.modules try: import db_dtypes.json # noqa: F401 @@ -284,7 +284,7 @@ def test_json_arrow_type_reregistration_is_handled(cleanup_json_module_for_reloa True ), "Module re-import completed without error, except block likely worked." - except pa.ArrowKeyError: # COVERAGE FAIL: 287-294 + except pa.ArrowKeyError: # COVERAGE FAIL: 287-294 # If this exception escapes, the except block in db_dtypes/json.py failed. pytest.fail( "pa.ArrowKeyError was raised during module reload, " From 320a02b3e460514d91d6e5f94f175aa45fdd56b7 Mon Sep 17 00:00:00 2001 From: chalmer lowe Date: Thu, 1 May 2025 12:54:40 +0000 Subject: [PATCH 4/9] updates tests for coverage reasons --- db_dtypes/__init__.py | 49 ++++++++++++------------------- tests/unit/test__init__.py | 59 ++++++++++++++++++++++++++++++++++++++ tests/unit/test_json.py | 42 ++++++++++++++++----------- 3 files changed, 102 insertions(+), 48 deletions(-) diff --git a/db_dtypes/__init__.py b/db_dtypes/__init__.py index 6656671..2629411 100644 --- a/db_dtypes/__init__.py +++ b/db_dtypes/__init__.py @@ -21,7 +21,6 @@ import warnings import numpy -import packaging.version import pandas import pandas.api.extensions from pandas.errors import OutOfBoundsDatetime @@ -29,7 +28,7 @@ import pyarrow.compute from db_dtypes import core -from db_dtypes.version import __version__ +from db_dtypes.json import JSONArray, JSONDtype, JSONArrowType # noqa: F401 from . import _versions_helpers @@ -47,15 +46,6 @@ _NP_BOX_DTYPE = "datetime64[us]" -# To use JSONArray and JSONDtype, you'll need Pandas 1.5.0 or later. With the removal -# of Python 3.7 compatibility, the minimum Pandas version will be updated to 1.5.0. -if packaging.version.Version(pandas.__version__) >= packaging.version.Version("1.5.0"): - from db_dtypes.json import JSONArray, JSONArrowType, JSONDtype -else: - JSONArray = None - JSONDtype = None - - @pandas.api.extensions.register_extension_dtype class TimeDtype(core.BaseDatetimeDtype): """ @@ -347,6 +337,22 @@ def __sub__(self, other): return super().__sub__(other) +def _determine_all(json_array_type, json_dtype_type): + """Determines the list for __all__ based on JSON type availability.""" + base_all = [ + "__version__", + "DateArray", + "DateDtype", + "TimeArray", + "TimeDtype", + ] + # Check if both JSON types are available (truthy) + if json_array_type and json_dtype_type: + return base_all + ["JSONDtype", "JSONArray", "JSONArrowType"] + else: + return base_all + + def _check_python_version(): """Checks the runtime Python version and issues a warning if needed.""" sys_major, sys_minor, sys_micro = _versions_helpers.extract_runtime_version() @@ -364,23 +370,4 @@ def _check_python_version(): _check_python_version() - -if not JSONArray or not JSONDtype: - __all__ = [ - "__version__", - "DateArray", - "DateDtype", - "TimeArray", - "TimeDtype", - ] -else: - __all__ = [ - "__version__", - "DateArray", - "DateDtype", - "JSONDtype", - "JSONArray", - "JSONArrowType", - "TimeArray", - "TimeDtype", - ] +__all__ = _determine_all(JSONArray, JSONDtype) diff --git a/tests/unit/test__init__.py b/tests/unit/test__init__.py index 4b86d54..4dc6209 100644 --- a/tests/unit/test__init__.py +++ b/tests/unit/test__init__.py @@ -83,3 +83,62 @@ def test_check_python_version_does_not_warn_on_supported(mock_version_tuple): # Assert that warnings.warn was NOT called mock_warn_call.assert_not_called() + + +def test_determine_all_includes_json_when_available(): + """ + Test that _determine_all includes JSON types when both are truthy. + """ + + from db_dtypes import _determine_all + + # Simulate available types (can be any truthy object) + mock_json_array = object() + mock_json_dtype = object() + + result = _determine_all(mock_json_array, mock_json_dtype) + + expected_all = [ + "__version__", + "DateArray", + "DateDtype", + "TimeArray", + "TimeDtype", + "JSONDtype", + "JSONArray", + "JSONArrowType", + ] + assert set(result) == set(expected_all) + assert "JSONDtype" in result + assert "JSONArray" in result + assert "JSONArrowType" in result + + +@pytest.mark.parametrize( + "mock_array, mock_dtype", + [ + (None, object()), # JSONArray is None + (object(), None), # JSONDtype is None + (None, None), # Both are None + ], +) +def test_determine_all_excludes_json_when_unavailable(mock_array, mock_dtype): + """ + Test that _determine_all excludes JSON types if either is falsy. + """ + + from db_dtypes import _determine_all + + result = _determine_all(mock_array, mock_dtype) + + expected_all = [ + "__version__", + "DateArray", + "DateDtype", + "TimeArray", + "TimeDtype", + ] + assert set(result) == set(expected_all) + assert "JSONDtype" not in result + assert "JSONArray" not in result + assert "JSONArrowType" not in result diff --git a/tests/unit/test_json.py b/tests/unit/test_json.py index 1e77d73..02f7012 100644 --- a/tests/unit/test_json.py +++ b/tests/unit/test_json.py @@ -21,6 +21,7 @@ import pytest import db_dtypes +import db_dtypes.json # Check for minimum Pandas version. pytest.importorskip("pandas", minversion="1.5.0") @@ -245,20 +246,21 @@ def cleanup_json_module_for_reload(): # Explicitly register just in case it wasn't, or was cleaned up elsewhere. # This might raise ArrowKeyError itself if already registered, which is fine here. pa.register_extension_type(db_dtypes.json.JSONArrowType()) + except pa.ArrowKeyError: pass # Already registered is the state we want before the test runs # Remove the module from sys.modules so importlib.reload re-executes it - if json_module_name in sys.modules: # COVERAGE FAIL: 252->255 + if json_module_name in sys.modules: del sys.modules[json_module_name] yield # Run the test that uses this fixture # Cleanup: Put the original module back if it existed # This helps isolate from other tests that might import db_dtypes.json - if original_module: # COVERAGE FAIL: 259-261 + if original_module: sys.modules[json_module_name] = original_module - elif json_module_name in sys.modules: # COVERAGE FAIL: 261->exit + elif json_module_name in sys.modules: # If the test re-imported it but it wasn't there originally, remove it del sys.modules[json_module_name] @@ -266,6 +268,23 @@ def cleanup_json_module_for_reload(): # thus we are using the testing pattern of module isolation/reloading. +# Test specifically for the fixture's pre-yield removal logic +def test_fixture_removes_module_if_present(cleanup_json_module_for_reload): + """ + Tests that the cleanup_json_module_for_reload fixture removes + db_dtypes.json from sys.modules before yielding to the test. + This specifically targets the 'if json_module_name in sys.modules:' block. + """ + # This test runs *after* the fixture's `yield`. + # The fixture should have removed the module if it was present. + + json_module_name = "db_dtypes.json" + + assert ( + json_module_name not in sys.modules + ), f"The fixture cleanup_json_module_for_reload should have removed {json_module_name}" + + def test_json_arrow_type_reregistration_is_handled(cleanup_json_module_for_reload): """ Verify that attempting to re-register JSONArrowType via module reload @@ -276,17 +295,6 @@ def test_json_arrow_type_reregistration_is_handled(cleanup_json_module_for_reloa # forces Python to execute the module's top-level code again. # This includes the pa.register_extension_type call. - assert "db_dtypes.json" not in sys.modules - try: - import db_dtypes.json # noqa: F401 - - assert ( - True - ), "Module re-import completed without error, except block likely worked." - - except pa.ArrowKeyError: # COVERAGE FAIL: 287-294 - # If this exception escapes, the except block in db_dtypes/json.py failed. - pytest.fail( - "pa.ArrowKeyError was raised during module reload, " - "indicating the except block failed." - ) + import db_dtypes.json # noqa: F401 + + assert True, "Module re-import completed without error, except block likely worked." From a64477754cc5f3d3fe1c4e924fb605cef8b08ecd Mon Sep 17 00:00:00 2001 From: chalmer lowe Date: Thu, 1 May 2025 12:59:52 +0000 Subject: [PATCH 5/9] Remove refs to 3.8 to help control github testing --- .github/sync-repo-settings.yaml | 1 - noxfile.py | 2 -- 2 files changed, 3 deletions(-) diff --git a/.github/sync-repo-settings.yaml b/.github/sync-repo-settings.yaml index bae6e96..55cd410 100644 --- a/.github/sync-repo-settings.yaml +++ b/.github/sync-repo-settings.yaml @@ -11,7 +11,6 @@ branchProtectionRules: - 'cla/google' - 'docs' - 'lint' - - 'unit (3.8)' - 'unit (3.9)' - 'unit (3.10)' - 'unit (3.11)' diff --git a/noxfile.py b/noxfile.py index b3c9450..ed7209a 100644 --- a/noxfile.py +++ b/noxfile.py @@ -35,8 +35,6 @@ DEFAULT_PYTHON_VERSION = "3.8" UNIT_TEST_PYTHON_VERSIONS: List[str] = [ - "3.7", - "3.8", "3.9", "3.10", "3.11", From dcfcfefc0912b177c627a991cfb4b2bfb0a445be Mon Sep 17 00:00:00 2001 From: chalmer lowe Date: Thu, 1 May 2025 14:36:57 +0000 Subject: [PATCH 6/9] removes more references to 3.8 --- .github/workflows/lint.yml | 2 +- .github/workflows/unittest.yml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 4866193..35b2ef7 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -12,7 +12,7 @@ jobs: - name: Setup Python uses: actions/setup-python@v5 with: - python-version: "3.8" + python-version: "3.9" - name: Install nox run: | python -m pip install --upgrade setuptools pip wheel diff --git a/.github/workflows/unittest.yml b/.github/workflows/unittest.yml index 699045c..bdd18f9 100644 --- a/.github/workflows/unittest.yml +++ b/.github/workflows/unittest.yml @@ -11,7 +11,7 @@ jobs: runs-on: ubuntu-22.04 strategy: matrix: - python: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13'] + python: ['3.9', '3.10', '3.11', '3.12', '3.13'] steps: - name: Checkout uses: actions/checkout@v4 @@ -103,7 +103,7 @@ jobs: - name: Setup Python uses: actions/setup-python@v5 with: - python-version: "3.8" + python-version: "3.9" - name: Install coverage run: | python -m pip install --upgrade setuptools pip wheel From d75eed4ed56baf25e647be8e7b1ca7f16d0872fe Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Thu, 1 May 2025 14:39:16 +0000 Subject: [PATCH 7/9] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20po?= =?UTF-8?q?st-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- .github/workflows/lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 35b2ef7..4866193 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -12,7 +12,7 @@ jobs: - name: Setup Python uses: actions/setup-python@v5 with: - python-version: "3.9" + python-version: "3.8" - name: Install nox run: | python -m pip install --upgrade setuptools pip wheel From 4abdff4d8f6c9d297b3b3819b83f4c94f3d2420a Mon Sep 17 00:00:00 2001 From: chalmer lowe Date: Thu, 1 May 2025 14:59:11 +0000 Subject: [PATCH 8/9] Removes several more refs to 3.8 --- noxfile.py | 4 ++-- owlbot.py | 2 +- setup.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/noxfile.py b/noxfile.py index ed7209a..e0d60a1 100644 --- a/noxfile.py +++ b/noxfile.py @@ -32,7 +32,7 @@ ISORT_VERSION = "isort==5.11.0" LINT_PATHS = ["docs", "db_dtypes", "tests", "noxfile.py", "setup.py"] -DEFAULT_PYTHON_VERSION = "3.8" +DEFAULT_PYTHON_VERSION = "3.9" UNIT_TEST_PYTHON_VERSIONS: List[str] = [ "3.9", @@ -54,7 +54,7 @@ UNIT_TEST_EXTRAS: List[str] = [] UNIT_TEST_EXTRAS_BY_PYTHON: Dict[str, List[str]] = {} -SYSTEM_TEST_PYTHON_VERSIONS: List[str] = ["3.8"] +SYSTEM_TEST_PYTHON_VERSIONS: List[str] = ["3.9"] SYSTEM_TEST_STANDARD_DEPENDENCIES: List[str] = [ "mock", "pytest", diff --git a/owlbot.py b/owlbot.py index 18bd623..04664d8 100644 --- a/owlbot.py +++ b/owlbot.py @@ -28,7 +28,7 @@ # Add templated files # ---------------------------------------------------------------------------- templated_files = common.py_library( - system_test_python_versions=["3.8"], + system_test_python_versions=["3.9"], cov_level=100, intersphinx_dependencies={ "pandas": "https://pandas.pydata.org/pandas-docs/stable/" diff --git a/setup.py b/setup.py index 98bed9d..549ee6d 100644 --- a/setup.py +++ b/setup.py @@ -75,6 +75,6 @@ def readme(): ], platforms="Posix; MacOS X; Windows", install_requires=dependencies, - python_requires=">=3.7", + python_requires=">=3.8", tests_require=["pytest"], ) From 9dba683ce3bac3e4be43a2df7852892ac8c9e2bb Mon Sep 17 00:00:00 2001 From: Chalmer Lowe Date: Thu, 1 May 2025 11:00:25 -0400 Subject: [PATCH 9/9] Update setup.py --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 549ee6d..036fcaa 100644 --- a/setup.py +++ b/setup.py @@ -75,6 +75,6 @@ def readme(): ], platforms="Posix; MacOS X; Windows", install_requires=dependencies, - python_requires=">=3.8", + python_requires=">=3.9", tests_require=["pytest"], )