From 26a7ad9860ffbc7ce7bc14e891c45a7ca48d0acb Mon Sep 17 00:00:00 2001 From: Andy Salnikov Date: Tue, 9 Jul 2024 13:27:27 -0700 Subject: [PATCH] Fix unit test for spatial overlaps (DM-44362) Using non-common skypix is going to be deprecated in favor of a direct use of regions. New query system raises NotImplementedError for non-common skypix, this patch replaces that exception with a new exception class to support exception propagation from remote butler to client. `test_skypix_constraint_queries` unit test is updated to handle exceptions from the new query system. When direct butler switches to the new query system, that test can be removed or changed to always expect the exceptions. --- python/lsst/daf/butler/_exceptions.py | 9 ++++ .../daf/butler/direct_query_driver/_driver.py | 2 +- .../daf/butler/registry/dimensions/static.py | 5 +- .../daf/butler/registry/tests/_registry.py | 50 ++++++++++++------- tests/test_remote_butler.py | 5 -- 5 files changed, 44 insertions(+), 27 deletions(-) diff --git a/python/lsst/daf/butler/_exceptions.py b/python/lsst/daf/butler/_exceptions.py index 746b4c361c..aff03de7de 100644 --- a/python/lsst/daf/butler/_exceptions.py +++ b/python/lsst/daf/butler/_exceptions.py @@ -155,6 +155,14 @@ class MissingCollectionError(CollectionError, ButlerUserError): error_type = "missing_collection" +class UnimplementedQueryError(NotImplementedError, ButlerUserError): + """Exception raised when the query system does not support the query + specified by the user. + """ + + error_type = "unimplemented_query" + + class MissingDatasetTypeError(DatasetTypeError, KeyError, ButlerUserError): """Exception raised when a dataset type does not exist.""" @@ -216,6 +224,7 @@ class UnknownButlerUserError(ButlerUserError): InvalidQueryError, MissingCollectionError, MissingDatasetTypeError, + UnimplementedQueryError, UnknownButlerUserError, ) _USER_ERROR_MAPPING = {e.error_type: e for e in _USER_ERROR_TYPES} diff --git a/python/lsst/daf/butler/direct_query_driver/_driver.py b/python/lsst/daf/butler/direct_query_driver/_driver.py index 6e4bc8c09c..68ccc0e52d 100644 --- a/python/lsst/daf/butler/direct_query_driver/_driver.py +++ b/python/lsst/daf/butler/direct_query_driver/_driver.py @@ -484,7 +484,7 @@ def analyze_query( Column expressions to sort by. find_first_dataset : `str` or `None`, optional Name of a dataset type for which only one result row for each data - ID should be returned, with the colletions searched in order. + ID should be returned, with the collections searched in order. Returns ------- diff --git a/python/lsst/daf/butler/registry/dimensions/static.py b/python/lsst/daf/butler/registry/dimensions/static.py index 69f1801750..ae449dd9dd 100644 --- a/python/lsst/daf/butler/registry/dimensions/static.py +++ b/python/lsst/daf/butler/registry/dimensions/static.py @@ -40,6 +40,7 @@ from ... import ddl from ..._column_tags import DimensionKeyColumnTag, DimensionRecordColumnTag from ..._column_type_info import LogicalColumn +from ..._exceptions import UnimplementedQueryError from ..._named import NamedKeyDict from ...dimensions import ( DatabaseDimensionElement, @@ -1100,7 +1101,7 @@ def visit_spatial_join( # Reject spatial joins that are nested inside OR or NOT, because the # postprocessing needed for those would be a lot harder. if flags & PredicateVisitFlags.INVERTED or flags & PredicateVisitFlags.HAS_OR_SIBLINGS: - raise NotImplementedError("Spatial overlap joins nested inside OR or NOT are not supported.") + raise UnimplementedQueryError("Spatial overlap joins nested inside OR or NOT are not supported.") # Delegate to super to check for invalid joins and record this # "connection" for use when seeing whether to add an automatic join # later. @@ -1137,7 +1138,7 @@ def visit_spatial_join( # tile is necessary but not sufficient for that. self.builder.postprocessing.spatial_join_filtering.append((a, b)) case _: - raise NotImplementedError(f"Unsupported combination for spatial join: {a, b}.") + raise UnimplementedQueryError(f"Unsupported combination for spatial join: {a, b}.") return qt.Predicate.from_bool(True) def _join_common_skypix_overlap(self, element: DatabaseDimensionElement) -> None: diff --git a/python/lsst/daf/butler/registry/tests/_registry.py b/python/lsst/daf/butler/registry/tests/_registry.py index 789c19069c..8ef96d5746 100644 --- a/python/lsst/daf/butler/registry/tests/_registry.py +++ b/python/lsst/daf/butler/registry/tests/_registry.py @@ -30,6 +30,7 @@ __all__ = ["RegistryTests"] +import contextlib import datetime import itertools import os @@ -3644,27 +3645,38 @@ def test_skypix_constraint_queries(self) -> None: break else: raise RuntimeError("Could not find usable skypix ID for this dimension configuration.") - self.assertEqual( - { - (data_id["tract"], data_id["patch"]) - for data_id in registry.queryDataIds( - ["patch"], - dataId={skypix_dimension.name: skypix_id}, - ) - }, - overlapping_patches, - ) + # New query system does not support non-common skypix constraints + # and we are deprecating it to replace with region-based constrints. + # TODO: Drop this tests once we remove support for non-common skypix. + with contextlib.suppress(NotImplementedError): + self.assertEqual( + { + (data_id["tract"], data_id["patch"]) + for data_id in registry.queryDataIds( + ["patch"], + dataId={skypix_dimension.name: skypix_id}, + ) + }, + overlapping_patches, + ) # Test that a three-way join that includes the common skypix system in # the dimensions doesn't generate redundant join terms in the query. - full_data_ids = set( - registry.queryDataIds( - ["tract", "visit", "htm7"], skymap="hsc_rings_v1", instrument="HSC" - ).expanded() - ) - self.assertGreater(len(full_data_ids), 0) - for data_id in full_data_ids: - self.assertFalse(data_id.records["tract"].region.isDisjointFrom(data_id.records["htm7"].region)) - self.assertFalse(data_id.records["visit"].region.isDisjointFrom(data_id.records["htm7"].region)) + # TODO: In the new query system this raises InvalidQueryError, change + # the test to assertRaises when direct butler switches to new queries. + with contextlib.suppress(InvalidQueryError): + full_data_ids = set( + registry.queryDataIds( + ["tract", "visit", "htm7"], skymap="hsc_rings_v1", instrument="HSC" + ).expanded() + ) + self.assertGreater(len(full_data_ids), 0) + for data_id in full_data_ids: + self.assertFalse( + data_id.records["tract"].region.isDisjointFrom(data_id.records["htm7"].region) + ) + self.assertFalse( + data_id.records["visit"].region.isDisjointFrom(data_id.records["htm7"].region) + ) def test_spatial_constraint_queries(self) -> None: """Test queries in which one spatial dimension in the constraint (data diff --git a/tests/test_remote_butler.py b/tests/test_remote_butler.py index 0cd55ad9ef..41b1a119ff 100644 --- a/tests/test_remote_butler.py +++ b/tests/test_remote_butler.py @@ -199,11 +199,6 @@ def test_query_projection_drop_postprocessing(self): # RemoteButler. pass - @unittest.expectedFailure - def test_skypix_constraint_queries(self) -> None: - # TODO DM-44362 - return super().test_skypix_constraint_queries() - @unittest.skipIf(create_test_server is None, "Server dependencies not installed.") class RemoteButlerSqliteRegistryTests(RemoteButlerRegistryTests, unittest.TestCase):