Merge pull request #936 from lsst/tickets/DM-42324

DM-42324: refactor dimension record storage classes

python/lsst/daf/butler/_column_type_info.py

@@ -127,7 +127,6 @@ def make_relation_table_spec(
                     sql.Engine.EMPTY_COLUMNS_NAME,
                     dtype=sql.Engine.EMPTY_COLUMNS_TYPE,
                     nullable=True,
-                    default=True,
                 )
             )
         for tag in columns:

python/lsst/daf/butler/dimensions/_database.py

@@ -38,21 +38,14 @@
 from types import MappingProxyType
 from typing import TYPE_CHECKING
 
-from lsst.utils import doImportType
 from lsst.utils.classes import cached_getter
 
-from .._named import NamedKeyMapping, NamedValueAbstractSet, NamedValueSet
+from .._named import NamedValueAbstractSet, NamedValueSet
 from .._topology import TopologicalFamily, TopologicalSpace
 from ._elements import Dimension, DimensionCombination, DimensionElement, KeyColumnSpec, MetadataColumnSpec
 from .construction import DimensionConstructionBuilder, DimensionConstructionVisitor
 
 if TYPE_CHECKING:
-    from ..registry.interfaces import (
-        Database,
-        DatabaseDimensionRecordStorage,
-        GovernorDimensionRecordStorage,
-        StaticTablesContext,
-    )
     from ._governor import GovernorDimension
     from ._universe import DimensionUniverse
 
@@ -215,17 +208,22 @@ def metadata_columns(self) -> NamedValueAbstractSet[MetadataColumnSpec]:
         return self._metadata_columns
 
     @property
-    def viewOf(self) -> str | None:
+    def implied_union_target(self) -> DimensionElement | None:
         # Docstring inherited from DimensionElement.
-        # This is a bit encapsulation-breaking; these storage config values
-        # are supposed to be opaque here, and just forwarded on to some
-        # DimensionRecordStorage implementation.  The long-term fix is to
-        # move viewOf entirely, by changing the code that relies on it to rely
-        # on the DimensionRecordStorage object instead.
+        # This is a bit encapsulation-breaking, but it'll all be cleaned up
+        # soon when we get rid of the storage objects entirely.
         storage = self._storage.get("nested")
         if storage is None:
             storage = self._storage
-        return storage.get("view_of")
+        name = storage.get("view_of")
+        return self.universe[name] if name is not None else None
+
+    @property
+    def is_cached(self) -> bool:
+        # Docstring inherited.
+        # This is a bit encapsulation-breaking, but it'll all be cleaned up
+        # soon when we get rid of the storage objects entirely.
+        return "caching" in self._storage["cls"]
 
     @property
     def documentation(self) -> str:
@@ -247,51 +245,6 @@ def temporal(self) -> DatabaseTopologicalFamily | None:
         # Docstring inherited from TopologicalRelationshipEndpoint
         return self.topology.get(TopologicalSpace.TEMPORAL)
 
-    def makeStorage(
-        self,
-        db: Database,
-        *,
-        context: StaticTablesContext | None = None,
-        governors: NamedKeyMapping[GovernorDimension, GovernorDimensionRecordStorage],
-        view_target: DatabaseDimensionRecordStorage | None = None,
-    ) -> DatabaseDimensionRecordStorage:
-        """Make the dimension record storage instance for this database.
-
-        Constructs the `DimensionRecordStorage` instance that should
-        be used to back this element in a registry.
-
-        Parameters
-        ----------
-        db : `Database`
-            Interface to the underlying database engine and namespace.
-        context : `StaticTablesContext`, optional
-            If provided, an object to use to create any new tables.  If not
-            provided, ``db.ensureTableExists`` should be used instead.
-        governors : `NamedKeyMapping`
-            Mapping from `GovernorDimension` to the record storage backend for
-            that dimension, containing all governor dimensions.
-        view_target : `DatabaseDimensionRecordStorage`, optional
-            Storage object for the element this target's storage is a view of
-            (i.e. when `viewOf` is not `None`).
-
-        Returns
-        -------
-        storage : `DatabaseDimensionRecordStorage`
-            Storage object that should back this element in a registry.
-        """
-        from ..registry.interfaces import DatabaseDimensionRecordStorage
-
-        cls = doImportType(self._storage["cls"])
-        assert issubclass(cls, DatabaseDimensionRecordStorage)
-        return cls.initialize(
-            db,
-            self,
-            context=context,
-            config=self._storage,
-            governors=governors,
-            view_target=view_target,
-        )
-
 
 class DatabaseDimension(Dimension, DatabaseDimensionElement):
     """A `Dimension` class that maps directly to a database table or query.
@@ -431,6 +384,11 @@ def alwaysJoin(self) -> bool:
         # Docstring inherited from DimensionElement.
         return self._alwaysJoin
 
+    @property
+    def defines_relationships(self) -> bool:
+        # Docstring inherited from DimensionElement.
+        return self._alwaysJoin or bool(self.implied)
+
     @property
     def populated_by(self) -> Dimension | None:
         # Docstring inherited.

python/lsst/daf/butler/dimensions/_elements.py

@@ -219,7 +219,7 @@ def hasTable(self) -> bool:
         Return `True` if this element is associated with a table
         (even if that table "belongs" to another element).
         """
-        return True
+        return self.has_own_table or self.implied_union_target is not None
 
     universe: DimensionUniverse
     """The universe of all compatible dimensions with which this element is
@@ -383,7 +383,7 @@ def viewOf(self) -> str | None:
 
         (`str` or `None`).
         """
-        return None
+        return self.implied_union_target.name if self.implied_union_target is not None else None
 
     @property
     def alwaysJoin(self) -> bool:
@@ -395,6 +395,37 @@ def alwaysJoin(self) -> bool:
         """
         return False
 
+    @property
+    def has_own_table(self) -> bool:
+        """Whether this element should have its own table in the database."""
+        return self.implied_union_target is None
+
+    @property
+    def implied_union_target(self) -> DimensionElement | None:
+        """If not `None`, the name of an element whose implied values for
+        this element form the set of allowable values.
+
+        For example, in the default dimension universe, the allowed values for
+        ``band`` is the union of all ``band`` values in the
+        ``physical_filter`` table, so the `implied_union_target` for ``band``
+        is ``physical_filter``.
+        """
+        return None
+
+    @property
+    def defines_relationships(self) -> bool:
+        """Whether this element's records define one or more relationships that
+        must be satisfied in rows over dimensions that include it.
+        """
+        return bool(self.implied)
+
+    @property
+    def is_cached(self) -> bool:
+        """Whether this element's records should be aggressively cached,
+        because they are small in number and rarely inserted.
+        """
+        return False
+
     @property
     @abstractmethod
     def populated_by(self) -> Dimension | None:

python/lsst/daf/butler/dimensions/_governor.py

@@ -31,18 +31,12 @@
 
 from collections.abc import Iterable, Mapping, Set
 from types import MappingProxyType
-from typing import TYPE_CHECKING
-
-from lsst.utils import doImportType
 
 from .._named import NamedValueAbstractSet, NamedValueSet
 from .._topology import TopologicalFamily, TopologicalSpace
 from ._elements import Dimension, KeyColumnSpec, MetadataColumnSpec
 from .construction import DimensionConstructionBuilder, DimensionConstructionVisitor
 
-if TYPE_CHECKING:
-    from ..registry.interfaces import Database, GovernorDimensionRecordStorage, StaticTablesContext
-
 
 class GovernorDimension(Dimension):
     """Governor dimension.
@@ -150,41 +144,16 @@ def unique_keys(self) -> NamedValueAbstractSet[KeyColumnSpec]:
         # Docstring inherited from Dimension.
         return self._unique_keys
 
+    @property
+    def is_cached(self) -> bool:
+        # Docstring inherited.
+        return True
+
     @property
     def documentation(self) -> str:
         # Docstring inherited from DimensionElement.
         return self._doc
 
-    def makeStorage(
-        self,
-        db: Database,
-        *,
-        context: StaticTablesContext | None = None,
-    ) -> GovernorDimensionRecordStorage:
-        """Make storage record.
-
-        Constructs the `DimensionRecordStorage` instance that should
-        be used to back this element in a registry.
-
-        Parameters
-        ----------
-        db : `Database`
-            Interface to the underlying database engine and namespace.
-        context : `StaticTablesContext`, optional
-            If provided, an object to use to create any new tables.  If not
-            provided, ``db.ensureTableExists`` should be used instead.
-
-        Returns
-        -------
-        storage : `GovernorDimensionRecordStorage`
-            Storage object that should back this element in a registry.
-        """
-        from ..registry.interfaces import GovernorDimensionRecordStorage
-
-        cls = doImportType(self._storage["cls"])
-        assert issubclass(cls, GovernorDimensionRecordStorage)
-        return cls.initialize(db, self, context=context, config=self._storage)
-
 
 class GovernorDimensionConstructionVisitor(DimensionConstructionVisitor):
     """A construction visitor for `GovernorDimension`.

python/lsst/daf/butler/dimensions/_record_set.py

@@ -379,13 +379,16 @@ def find_with_required_values(
             self._by_required_values[required_values] = result
         return result
 
-    def add(self, value: DimensionRecord) -> None:
+    def add(self, value: DimensionRecord, replace: bool = True) -> None:
         """Add a new record to the set.
 
         Parameters
         ----------
         value : `DimensionRecord`
             Record to add.
+        replace : `bool`, optional
+            If `True` (default) replace any existing record with the same data
+            ID.  If `False` the existing record will be kept.
 
         Raises
         ------
@@ -396,23 +399,29 @@ def add(self, value: DimensionRecord) -> None:
             raise ValueError(
                 f"Cannot add record {value} for {value.definition.name!r} to set for {self.element!r}."
             )
-        self._by_required_values[value.dataId.required_values] = value
+        if replace:
+            self._by_required_values[value.dataId.required_values] = value
+        else:
+            self._by_required_values.setdefault(value.dataId.required_values, value)
 
-    def update(self, values: Iterable[DimensionRecord]) -> None:
+    def update(self, values: Iterable[DimensionRecord], replace: bool = True) -> None:
         """Add new records to the set.
 
         Parameters
         ----------
         values : `~collections.abc.Iterable` [ `DimensionRecord` ]
-            Record to add.
+            Records to add.
+        replace : `bool`, optional
+            If `True` (default) replace any existing records with the same data
+            IDs.  If `False` the existing records will be kept.
 
         Raises
         ------
         ValueError
             Raised if ``value.element != self.element``.
         """
         for value in values:
-            self.add(value)
+            self.add(value, replace=replace)
 
     def update_from_data_coordinates(self, data_coordinates: Iterable[DataCoordinate]) -> None:
         """Add records to the set by extracting and deduplicating them from

python/lsst/daf/butler/dimensions/_schema.py

@@ -261,7 +261,7 @@ def addDimensionForeignKey(
     tableSpec.fields.add(fieldSpec)
     # Also add a foreign key constraint on the dependency table, but only if
     # there actually is one and we weren't told not to.
-    if dimension.hasTable() and dimension.viewOf is None and constraint:
+    if dimension.has_own_table and constraint:
         tableSpec.foreignKeys.append(_makeForeignKeySpec(dimension))
     return fieldSpec
 

python/lsst/daf/butler/dimensions/_skypix.py

@@ -47,7 +47,6 @@
 from .construction import DimensionConstructionBuilder, DimensionConstructionVisitor
 
 if TYPE_CHECKING:
-    from ..registry.interfaces import SkyPixDimensionRecordStorage
     from ._universe import DimensionUniverse
 
 
@@ -163,20 +162,10 @@ def hasTable(self) -> bool:
         # Docstring inherited from DimensionElement.hasTable.
         return False
 
-    def makeStorage(self) -> SkyPixDimensionRecordStorage:
-        """Make the storage record.
-
-        Constructs the `DimensionRecordStorage` instance that should
-        be used to back this element in a registry.
-
-        Returns
-        -------
-        storage : `SkyPixDimensionRecordStorage`
-            Storage object that should back this element in a registry.
-        """
-        from ..registry.dimensions.skypix import BasicSkyPixDimensionRecordStorage
-
-        return BasicSkyPixDimensionRecordStorage(self)
+    @property
+    def has_own_table(self) -> bool:
+        # Docstring inherited from DimensionElement.
+        return False
 
     @property
     def unique_keys(self) -> NamedValueAbstractSet[KeyColumnSpec]: