Maintain conjunctive normal form in predicate trees.

python/lsst/daf/butler/queries/expression_factory.py

@@ -44,7 +44,7 @@
 
 # This module uses ExpressionProxy and its subclasses to wrap ColumnExpression,
 # but it just returns OrderExpression and Predicate objects directly, because
-# we don't need to overload an operators or define any methods on those.
+# we don't need to overload any operators or define any methods on those.
 
 
 class ExpressionProxy:
@@ -135,7 +135,7 @@ def in_iterable(self, others: Iterable) -> rt.Predicate:
 
         Parameters
         ----------
-        others : `Iterable`
+        others : `collections.abc.Iterable`
             An iterable of `ExpressionProxy` or values to be interpreted as
             literals.
 
@@ -268,7 +268,7 @@ class DimensionProxy(ScalarExpressionProxy, DimensionElementProxy):
 
     Parameters
     ----------
-    element : `DimensionElement`
+    dimension : `DimensionElement`
         Element this object wraps.
 
     Notes
@@ -355,22 +355,61 @@ def __getitem__(self, name: str | EllipsisType) -> DatasetTypeProxy:
         return DatasetTypeProxy(name)
 
     def not_(self, operand: rt.Predicate) -> rt.Predicate:
-        """Apply a logical NOT operation to a boolean expression."""
-        return rt.LogicalNot.model_construct(operand=operand)
+        """Apply a logical NOT operation to a boolean expression.
+
+        Parameters
+        ----------
+        operand : `relation_tree.Predicate`
+            Expression to invert.
+
+        Returns
+        -------
+        logical_not : `relation_tree.Predicate`
+            A boolean expression that evaluates to the opposite of ``operand``.
+        """
+        return operand.logical_not()
 
-    def all(self, *args: rt.Predicate) -> rt.Predicate:
-        """Combine a sequence of boolean expressions with logical AND."""
-        operands: list[rt.Predicate] = []
+    def all(self, first: rt.Predicate, /, *args: rt.Predicate) -> rt.Predicate:
+        """Combine a sequence of boolean expressions with logical AND.
+
+        Parameters
+        ----------
+        first : `relation_tree.Predicate`
+            First operand (required).
+        *args
+            Additional operands.
+
+        Returns
+        -------
+        logical_and : `relation_tree.Predicate`
+            A boolean expression that evaluates to `True` only if all operands
+            evaluate to `True.
+        """
+        result = first
         for arg in args:
-            operands.extend(arg._flatten_and())
-        return rt.LogicalAnd.model_construct(operands=tuple(operands))
+            result = result.logical_and(arg)
+        return result
+
+    def any(self, first: rt.Predicate, /, *args: rt.Predicate) -> rt.Predicate:
+        """Combine a sequence of boolean expressions with logical OR.
+
+        Parameters
+        ----------
+        first : `relation_tree.Predicate`
+            First operand (required).
+        *args
+            Additional operands.
 
-    def any(self, *args: rt.Predicate) -> rt.Predicate:
-        """Combine a sequence of boolean expressions with logical OR."""
-        operands: list[rt.Predicate] = []
+        Returns
+        -------
+        logical_or : `relation_tree.Predicate`
+            A boolean expression that evaluates to `True` if any operand
+            evaluates to `True.
+        """
+        result = first
         for arg in args:
-            operands.extend(arg._flatten_or())
-        return rt.LogicalOr.model_construct(operands=tuple(operands))
+            result = result.logical_or(arg)
+        return result
 
     @staticmethod
     def literal(value: object) -> ExpressionProxy:
@@ -379,6 +418,16 @@ def literal(value: object) -> ExpressionProxy:
         Expression proxy objects obtained from this factory can generally be
         compared directly to literals, so calling this method directly in user
         code should rarely be necessary.
+
+        Parameters
+        ----------
+        value : `object`
+            Value to include as a literal in an expression tree.
+
+        Returns
+        -------
+        expression : `ExpressionProxy`
+            Expression wrapper for this literal.
         """
         expression = rt.make_column_literal(value)
         match expression.expression_type:

python/lsst/daf/butler/queries/relation_tree/_base.py

@@ -39,7 +39,7 @@
 
 from abc import ABC, abstractmethod
 from types import EllipsisType
-from typing import TYPE_CHECKING, Annotated, Literal, TypeAlias, cast
+from typing import TYPE_CHECKING, Annotated, Any, Literal, TypeAlias, cast
 
 import pydantic
 
@@ -279,6 +279,11 @@ class PredicateBase(RelationTreeBase, ABC):
     """Base class for objects that represent boolean column expressions in a
     relation tree.
 
+    A `Predicate` tree is always in conjunctive normal form (ANDs of ORs of
+    NOTs).  This is enforced by type annotations (and hence Pydantic
+    validation) and the `logical_and`, `logical_or`, and `logical_not` factory
+    methods.
+
     This is a closed hierarchy whose concrete, `~typing.final` derived classes
     are members of the `Predicate` union.  That union should generally be used
     in type annotations rather than the formally-open base class.
@@ -308,14 +313,51 @@ def gather_required_columns(self) -> set[ColumnReference]:
         """
         return set()
 
-    def _flatten_and(self) -> tuple[Predicate, ...]:
-        """Convert this expression to a sequence of predicates that should be
-        combined with logical AND.
+    # The 'other' arguments of the methods below are annotated as Any because
+    # MyPy doesn't correctly recognize subclass implementations that use
+    # @overload, and the signature of this base class doesn't really matter,
+    # since it's the union of all concrete implementations that's public;
+    # the base class exists largely as a place to hang docstrings.
+
+    @abstractmethod
+    def logical_and(self, other: Any) -> Predicate:
+        """Return the logical AND of this predicate and another.
+
+        Parameters
+        ----------
+        other : `Predicate`
+            Other operand.
+
+        Returns
+        -------
+        result : `Predicate`
+            A predicate presenting the logical AND.
         """
-        return (self,)  # type: ignore[return-value]
+        raise NotImplementedError()
 
-    def _flatten_or(self) -> tuple[Predicate, ...]:
-        """Convert this expression to a sequence of predicates that should be
-        combined with logical OR.
+    @abstractmethod
+    def logical_or(self, other: Any) -> Predicate:
+        """Return the logical OR of this predicate and another.
+
+        Parameters
+        ----------
+        other : `Predicate`
+            Other operand.
+
+        Returns
+        -------
+        result : `Predicate`
+            A predicate presenting the logical OR.
+        """
+        raise NotImplementedError()
+
+    @abstractmethod
+    def logical_not(self) -> Predicate:
+        """Return the logical NOTof this predicate.
+
+        Returns
+        -------
+        result : `Predicate`
+            A predicate presenting the logical OR.
         """
-        return (self,)  # type: ignore[return-value]
+        raise NotImplementedError()