Skip to content

Commit

Permalink
Merge pull request #11 from lsst/tickets/DM-42324
Browse files Browse the repository at this point in the history
DM-42324: Fix min/max rows logic for Deduplication.
  • Loading branch information
TallJimbo committed Jan 8, 2024
2 parents 7fe8783 + 3d10fe9 commit b0a325e
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 15 deletions.
8 changes: 7 additions & 1 deletion python/lsst/daf/relation/_operations/_deduplication.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,13 @@ def __str__(self) -> str:

def applied_min_rows(self, target: Relation) -> int:
# Docstring inherited.
return 1
return 1 if target.min_rows >= 1 else 0

def applied_max_rows(self, target: Relation) -> int | None:
# Docstring inherited.
if not target.columns:
return 1 if target.max_rows is None or target.max_rows >= 1 else 0
return target.max_rows

def commute(self, current: UnaryOperationRelation) -> UnaryCommutator:
# Docstring inherited.
Expand Down
16 changes: 7 additions & 9 deletions python/lsst/daf/relation/_relation.py
Original file line number Diff line number Diff line change
Expand Up @@ -665,15 +665,13 @@ class BaseRelation:
"""

def __init_subclass__(cls) -> None:
assert (
cls.__name__
in {
"LeafRelation",
"UnaryOperationRelation",
"BinaryOperationRelation",
"MarkerRelation",
}
or cls.__base__.__name__ != "Relation"
assert cls.__name__ in {
"LeafRelation",
"UnaryOperationRelation",
"BinaryOperationRelation",
"MarkerRelation",
} or (
cls.__base__ is not None and cls.__base__.__name__ != "Relation"
), "Relation inheritance is closed to predefined types in daf_relation and MarkerRelation subclasses."

@property
Expand Down
7 changes: 2 additions & 5 deletions python/lsst/daf/relation/iteration/_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,7 @@ def convert_column_expression(
function = self.get_function(name)
if function is not None:
arg_callables = [self.convert_column_expression(arg) for arg in args]
# MyPy doesn't see 'function' as not-None for some reason.
return lambda row: function(*[c(row) for c in arg_callables]) # type: ignore
return lambda row: function(*[c(row) for c in arg_callables])
first, *rest = (self.convert_column_expression(arg) for arg in args)
return lambda row: getattr(first(row), name)(*[r(row) for r in rest])
raise AssertionError("matches should be exhaustive and all branches should return")
Expand Down Expand Up @@ -336,9 +335,7 @@ def convert_predicate(self, predicate: Predicate) -> Callable[[Mapping[ColumnTag
function = self.get_function(name)
if function is not None:
arg_callables = [self.convert_column_expression(arg) for arg in args]
# MyPy doesn't see 'function' as not-None in the capture
# for some reason.
return lambda row: function(*[c(row) for c in arg_callables]) # type: ignore
return lambda row: function(*[c(row) for c in arg_callables])
first, *rest = (self.convert_column_expression(arg) for arg in args)
return lambda row: getattr(first(row), name)(*[r(row) for r in rest])
case LogicalAnd(operands=operands):
Expand Down
10 changes: 10 additions & 0 deletions tests/test_deduplication.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ def test_attributes(self) -> None:
self.assertFalse(operation.is_order_dependent)
self.assertFalse(operation.is_count_dependent)

def test_min_max_rows(self) -> None:
"""Test min and max rows for edge-case deduplications."""
self.assertEqual(self.leaf.with_only_columns(set()).without_duplicates().min_rows, 1)
self.assertEqual(self.leaf.with_only_columns(set()).without_duplicates().max_rows, 1)
leaf0 = self.engine.make_leaf({self.a}, payload=iteration.RowSequence([]), name="leaf")
self.assertEqual(leaf0.without_duplicates().min_rows, 0)
self.assertEqual(leaf0.without_duplicates().max_rows, 0)
self.assertEqual(leaf0.with_only_columns([]).without_duplicates().min_rows, 0)
self.assertEqual(leaf0.with_only_columns([]).without_duplicates().max_rows, 0)

def test_backtracking_apply(self) -> None:
"""Test apply logic that involves reordering operations in the existing
tree to perform the new operation in a preferred engine.
Expand Down

0 comments on commit b0a325e

Please sign in to comment.