Skip to content

Commit

Permalink
Relax constraint on governors in query WHERE expressions.
Browse files Browse the repository at this point in the history
We want to raise when somebody says "visit = 42" as a query constraint
without also specifying the instrument that makes that visit ID
meaningful.  But a multi-instrument "visit.region OVERLAPS {region}"
query (for example) should be perfectly fine.
  • Loading branch information
TallJimbo committed Sep 9, 2024
1 parent 379f45d commit 5adaf49
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 3 deletions.
6 changes: 3 additions & 3 deletions python/lsst/daf/butler/direct_query_driver/_driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -934,9 +934,9 @@ def _analyze_query_tree(self, tree: qt.QueryTree) -> tuple[QueryJoinsPlan, Query
# without constraining their governor dimensions, since that's a
# particularly easy mistake to make and it's almost never intentional.
# We also allow the registry data ID values to provide governor values.
where_columns = qt.ColumnSet(self.universe.empty)
result.predicate.gather_required_columns(where_columns)
for governor in where_columns.dimensions.governors:
where_governors: set[str] = set()
result.predicate.gather_governors(where_governors)
for governor in where_governors:
if governor not in result.constraint_data_id:
if governor in self._default_data_id.dimensions:
result.constraint_data_id[governor] = self._default_data_id[governor]
Expand Down
16 changes: 16 additions & 0 deletions python/lsst/daf/butler/queries/tree/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,18 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
"""
raise NotImplementedError()

@abstractmethod
def gather_governors(self, governors: set[str]) -> None:
"""Add any governor dimensions that need to be fully identified for
this column expression to be sound.
Parameters
----------
governors : `set` [ `str` ]
Set of governor dimension names to modify in place.
"""
raise NotImplementedError()

@abstractmethod
def visit(self, visitor: ColumnExpressionVisitor[_T]) -> _T:
"""Invoke the visitor interface.
Expand Down Expand Up @@ -177,6 +189,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
pass

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
pass

@property
def column_type(self) -> ColumnType:
# Docstring inherited.
Expand Down
12 changes: 12 additions & 0 deletions python/lsst/daf/butler/queries/tree/_column_expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
self.operand.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.operand.gather_governors(governors)

@property
def column_type(self) -> ColumnType:
# Docstring inherited.
Expand Down Expand Up @@ -146,6 +150,11 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
self.a.gather_required_columns(columns)
self.b.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.a.gather_governors(governors)
self.b.gather_governors(governors)

@property
def column_type(self) -> ColumnType:
# Docstring inherited.
Expand Down Expand Up @@ -208,6 +217,9 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
self.operand.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
self.operand.gather_governors(governors)

Check warning on line 221 in python/lsst/daf/butler/queries/tree/_column_expression.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/daf/butler/queries/tree/_column_expression.py#L221

Added line #L221 was not covered by tests

@property
def column_type(self) -> ColumnType:
# Docstring inherited.
Expand Down
21 changes: 21 additions & 0 deletions python/lsst/daf/butler/queries/tree/_column_reference.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
columns.update_dimensions(self.dimension.minimal_group)

def gather_governors(self, governors: set[str]) -> None:
if self.dimension.governor is not None:
governors.add(self.dimension.governor.name)

@property
def column_type(self) -> ColumnType:
# Docstring inherited.
Expand Down Expand Up @@ -95,6 +99,19 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
columns.update_dimensions(self.element.minimal_group)
columns.dimension_fields[self.element.name].add(self.field)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
# We assume metadata fields (and certainly region and timespan fields)
# don't need to be qualified with (e.g.) an instrument or skymap name
# to make sense, but keys like visit IDs or detector names do need to
# e qualified.
if (
isinstance(self.element, Dimension)
and self.field in self.element.alternate_keys.names
and self.element.governor is not None
):
governors.add(self.element.governor.name)

@property
def column_type(self) -> ColumnType:
# Docstring inherited.
Expand Down Expand Up @@ -134,6 +151,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
columns.dataset_fields[self.dataset_type].add(self.field)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
pass

@property
def column_type(self) -> ColumnType:
# Docstring inherited.
Expand Down
58 changes: 58 additions & 0 deletions python/lsst/daf/butler/queries/tree/_predicate.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,18 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
"""
raise NotImplementedError()

@abstractmethod
def gather_governors(self, governors: set[str]) -> None:
"""Add any governor dimensions that need to be fully identified for
this column expression to be sound.
Parameters
----------
governors : `set` [ `str` ]
Set of governor dimension names to modify in place.
"""
raise NotImplementedError()

def invert(self) -> PredicateLeaf:
"""Return a new leaf that is the logical not of this one."""
return LogicalNot.model_construct(operand=cast("LogicalNotOperand", self))
Expand Down Expand Up @@ -293,6 +305,19 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
for operand in or_group:
operand.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
"""Add any governor dimensions that need to be fully identified for
this column expression to be sound.
Parameters
----------
governors : `set` [ `str` ]
Set of governor dimension names to modify in place.
"""
for or_group in self.operands:
for operand in or_group:
operand.gather_governors(governors)

def logical_and(self, *args: Predicate) -> Predicate:
"""Construct a predicate representing the logical AND of this predicate
and one or more others.
Expand Down Expand Up @@ -421,6 +446,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
self.operand.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.operand.gather_governors(governors)

def __str__(self) -> str:
return f"NOT {self.operand}"

Expand All @@ -445,6 +474,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
self.operand.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.operand.gather_governors(governors)

def __str__(self) -> str:
return f"{self.operand}"

Expand All @@ -466,6 +499,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
self.operand.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.operand.gather_governors(governors)

def __str__(self) -> str:
return f"{self.operand} IS NULL"

Expand Down Expand Up @@ -496,6 +533,11 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
self.a.gather_required_columns(columns)
self.b.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.a.gather_governors(governors)
self.b.gather_governors(governors)

def __str__(self) -> str:
return f"{self.a} {self.operator.upper()} {self.b}"

Expand Down Expand Up @@ -556,6 +598,12 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
for item in self.container:
item.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.member.gather_governors(governors)
for item in self.container:
item.gather_governors(governors)

def __str__(self) -> str:
return f"{self.member} IN [{', '.join(str(item) for item in self.container)}]"

Expand Down Expand Up @@ -598,6 +646,10 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# Docstring inherited.
self.member.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
self.member.gather_governors(governors)

def __str__(self) -> str:
s = f"{self.start if self.start else ''}:{self.stop if self.stop is not None else ''}"
if self.step != 1:
Expand Down Expand Up @@ -644,6 +696,12 @@ def gather_required_columns(self, columns: ColumnSet) -> None:
# attached to, not `self.column`, which belongs to `self.query_tree`.
self.member.gather_required_columns(columns)

def gather_governors(self, governors: set[str]) -> None:
# Docstring inherited.
# We're only gathering governors from the query_tree this predicate is
# attached to, not `self.column`, which belongs to `self.query_tree`.
self.member.gather_governors(governors)

def __str__(self) -> str:
return f"{self.member} IN (query).{self.column}"

Expand Down

0 comments on commit 5adaf49

Please sign in to comment.