Copilot commented on code in PR #2515: URL: https://github.com/apache/sedona/pull/2515#discussion_r2993257909
########## python/sedona/spark/__init__.py: ########## @@ -71,3 +71,27 @@ from sedona.spark.utils.adapter import Adapter from sedona.spark.utils.spatial_rdd_parser import GeoData from sedona.spark.utils.structured_adapter import StructuredAdapter + +from pyspark.sql import DataFrame + + +def to_sedonadb(self, connection=None): + """ + Converts a SedonaSpark DataFrame to a SedonaDB DataFrame. + :param connection: Optional SedonaDB connection object. If None, a new connection will be created. + :return: SedonaDB DataFrame + """ + try: + import sedona.db + except ImportError: + raise ImportError( + "SedonaDB is not installed. Please install it using `pip install sedona-db`." + ) Review Comment: The new `ImportError` branch isn’t covered by the added tests. Consider adding a test that ensures `to_sedonadb()` raises the expected `ImportError` (and message) when `sedona.db` is unavailable, so regressions in dependency handling are caught. ########## python/sedona/spark/__init__.py: ########## @@ -71,3 +71,27 @@ from sedona.spark.utils.adapter import Adapter from sedona.spark.utils.spatial_rdd_parser import GeoData from sedona.spark.utils.structured_adapter import StructuredAdapter + +from pyspark.sql import DataFrame + + +def to_sedonadb(self, connection=None): Review Comment: The function imports `sedona.db` unconditionally, even when a `connection` is explicitly provided. That makes the method fail with `ImportError` in cases where a caller passes a compatible connection object (or in environments doing partial installs) even though `sedona.db` isn’t required on that code path. Move the `import sedona.db` + `connect()` logic inside the `if connection is None:` branch so only the auto-connect path requires the SedonaDB package. ########## python/tests/test_to_sedonadb.py: ########## @@ -0,0 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import unittest +import sys +from unittest.mock import MagicMock, patch +from pyspark.sql import SparkSession, DataFrame +from sedona.spark import * + + +class TestToSedonaDB(unittest.TestCase): + + def setUp(self): + # Mock sedona.db to avoid ImportError + self.mock_sedona_db = MagicMock() + sys.modules["sedona.db"] = self.mock_sedona_db + import sedona + + sedona.db = self.mock_sedona_db + self.spark = SparkSession.builder.getOrCreate() + + def tearDown(self): + if "sedona.db" in sys.modules: + del sys.modules["sedona.db"] + import sedona + + if hasattr(sedona, "db"): Review Comment: The test mutates global import state (`sys.modules` and `sedona.db`) without restoring the prior values. If another test in the same process relies on the real `sedona.db` module (or if it was already imported), deleting it can cause order-dependent failures. Store any prior `sys.modules['sedona.db']` / `sedona.db` values in `setUp` and restore them in `tearDown`, or use `unittest.mock.patch.dict(sys.modules, ...)` and `patch.object(sedona, 'db', ...)` to ensure clean, reversible patching. ```suggestion # Preserve any existing sedona.db module and sedona.db attribute self._original_sedona_db_module = sys.modules.get("sedona.db") import sedona self._had_sedona_db_attr = hasattr(sedona, "db") self._original_sedona_db_attr = getattr(sedona, "db", None) # Mock sedona.db to avoid ImportError self.mock_sedona_db = MagicMock() sys.modules["sedona.db"] = self.mock_sedona_db sedona.db = self.mock_sedona_db self.spark = SparkSession.builder.getOrCreate() def tearDown(self): # Restore prior sys.modules['sedona.db'] state if self._original_sedona_db_module is not None: sys.modules["sedona.db"] = self._original_sedona_db_module elif "sedona.db" in sys.modules: del sys.modules["sedona.db"] import sedona # Restore prior sedona.db attribute state if self._had_sedona_db_attr: sedona.db = self._original_sedona_db_attr elif hasattr(sedona, "db"): ``` ########## python/sedona/spark/__init__.py: ########## @@ -71,3 +71,27 @@ from sedona.spark.utils.adapter import Adapter from sedona.spark.utils.spatial_rdd_parser import GeoData from sedona.spark.utils.structured_adapter import StructuredAdapter + +from pyspark.sql import DataFrame + + +def to_sedonadb(self, connection=None): + """ + Converts a SedonaSpark DataFrame to a SedonaDB DataFrame. + :param connection: Optional SedonaDB connection object. If None, a new connection will be created. + :return: SedonaDB DataFrame + """ + try: + import sedona.db + except ImportError: + raise ImportError( + "SedonaDB is not installed. Please install it using `pip install sedona-db`." + ) + + if connection is None: Review Comment: The function imports `sedona.db` unconditionally, even when a `connection` is explicitly provided. That makes the method fail with `ImportError` in cases where a caller passes a compatible connection object (or in environments doing partial installs) even though `sedona.db` isn’t required on that code path. Move the `import sedona.db` + `connect()` logic inside the `if connection is None:` branch so only the auto-connect path requires the SedonaDB package. ```suggestion if connection is None: try: import sedona.db except ImportError: raise ImportError( "SedonaDB is not installed. Please install it using `pip install sedona-db`." ) ``` ########## python/tests/test_to_sedonadb.py: ########## @@ -0,0 +1,91 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import unittest +import sys +from unittest.mock import MagicMock, patch +from pyspark.sql import SparkSession, DataFrame +from sedona.spark import * + + +class TestToSedonaDB(unittest.TestCase): + + def setUp(self): + # Mock sedona.db to avoid ImportError + self.mock_sedona_db = MagicMock() + sys.modules["sedona.db"] = self.mock_sedona_db + import sedona + + sedona.db = self.mock_sedona_db + self.spark = SparkSession.builder.getOrCreate() + + def tearDown(self): + if "sedona.db" in sys.modules: + del sys.modules["sedona.db"] + import sedona + + if hasattr(sedona, "db"): + del sedona.db Review Comment: The test creates/gets a SparkSession in `setUp` but never stops it in `tearDown`. This can leak resources across the test process and cause flakiness/hangs in CI. Add `self.spark.stop()` (or use the repo’s existing Spark test fixture/session management if one exists) so the session lifecycle is properly bounded per test/module. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
