diff --git a/ISSUES.md b/ISSUES.md index e6a302a..e5686d9 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -35,7 +35,7 @@ Legend: `[ ]` open · `[x]` resolved · `[~]` won't fix / by design. ## P2 — API design / node semantics -- [ ] **A1 · Expression-tree nodes have value-equality enabled, and it raises.** +- [x] **A1 · Expression-tree nodes have value-equality enabled, and it raises.** The node dataclasses use the default `eq=True`. The generated `__eq__` compares fields tuple-wise, reaching `Attr.__eq__`, which returns a `Predicate`; `bool()` on it raises `PredicateError`. So `node == node`, `node in [...]`, and @@ -44,7 +44,7 @@ Legend: `[ ]` open · `[x]` resolved · `[~]` won't fix / by design. *Fix:* `eq=False` on the node dataclasses (identity equality + identity hash — what the immutable tree actually wants). Regression test: `==`/`hash` behave. -- [ ] **A2 · `Equijoin` defers its output-collision check to `_schema()`.** +- [x] **A2 · `Equijoin` defers its output-collision check to `_schema()`.** `Equijoin.__post_init__` validates only join-attribute existence; the ambiguous-output-name `SchemaError` surfaces only when `_schema()` is later evaluated, unlike every sibling node. Eager-validation invariant hole. diff --git a/coddpiece/relation.py b/coddpiece/relation.py index 82c43d3..c16a04d 100644 --- a/coddpiece/relation.py +++ b/coddpiece/relation.py @@ -330,9 +330,17 @@ def relation_name(self) -> str: # frozen=True: immutability is critical because expression trees may share # subtrees across multiple expressions, and the compiler assumes the tree # is stable during traversal. +# eq=False: fall back to object identity for __eq__ AND __hash__. The default +# eq=True would generate an __eq__ that compares fields, reaching Attr.__eq__ +# (predicates.py) which returns a Predicate whose __bool__ raises — so +# node1 == node2 and `node in [...]` would blow up. It would also derive a +# __hash__ from the fields, which raises TypeError on the dict-valued fields of +# Grouping (aggs) and Rename (mapping). Expression-tree equivalence is identity, +# not structure, so object identity is the correct (and only safe) semantics. +# This applies to every node dataclass below, not just Selection. # repr=False: inherit BaseRelation.__repr__ (shows class name, relation_name, # schema) instead of the dataclass default which would expose internal fields. -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Selection(BaseRelation): """σ (Selection): Filter rows by a predicate. @@ -356,7 +364,7 @@ def relation_name(self) -> str: return self.child.relation_name -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Projection(BaseRelation): """π (Projection): Keep only specified attributes. @@ -390,7 +398,7 @@ def relation_name(self) -> str: # --------------------------------------------------------------------------- -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Rename(BaseRelation): """ρ (Rename): Rename attributes. mapping is {new_name: old_name}. @@ -497,7 +505,7 @@ def __init__(self, left: BaseRelation, right: BaseRelation): # --------------------------------------------------------------------------- -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class CrossProduct(BaseRelation): """× (Cross Product): Cartesian product. @@ -527,7 +535,7 @@ def relation_name(self) -> str: return f"({self.left.relation_name} × {self.right.relation_name})" -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class NaturalJoin(BaseRelation): """⋈ (Natural Join): Join on common attributes. @@ -567,7 +575,7 @@ def relation_name(self) -> str: return f"({self.left.relation_name} ⋈ {self.right.relation_name})" -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class ThetaJoin(BaseRelation): """⋈θ (Theta Join): Join with an arbitrary predicate. @@ -598,7 +606,7 @@ def relation_name(self) -> str: return f"({self.left.relation_name} ⋈θ {self.right.relation_name})" -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Equijoin(BaseRelation): """⋈= (Equijoin): Join where left_attr = right_attr. @@ -625,6 +633,20 @@ def __post_init__(self): f"Right relation has no attribute {self.right_attr!r}. " f"Available: {', '.join(self.right._schema().names())}" ) + # Eager-validation invariant: the ambiguous-output-name check must run + # at construction, not lazily in _schema(). The right join column is + # dropped from the output (left_attr = right_attr makes it redundant), + # so we test the *remaining* right attrs against the left names. Mirrors + # _schema()'s derivation exactly so the two paths can never diverge. + right_attrs = tuple( + a for a in self.right._schema().attributes if a.name != self.right_attr + ) + collisions = {a.name for a in right_attrs} & set(self.left._schema().names()) + if collisions: + raise SchemaError( + f"EQUIJOIN result has ambiguous attribute names: {collisions}. " + f"Hint: Use RENAME on one relation first." + ) @property def _engine(self) -> Engine: @@ -633,19 +655,14 @@ def _engine(self) -> Engine: def _schema(self) -> Schema: # Unlike cross product, equijoin drops the duplicate join column from # the right side (since left_attr = right_attr, keeping both is - # redundant). Then we check for remaining name collisions. + # redundant). The name-collision check lives in __post_init__ (eager + # validation), so by the time this node exists the result is known to + # be unambiguous and we only need to shape the schema here. left_s = self.left._schema() right_s = self.right._schema() right_attrs = tuple( a for a in right_s.attributes if a.name != self.right_attr ) - left_names = set(left_s.names()) - collisions = {a.name for a in right_attrs} & left_names - if collisions: - raise SchemaError( - f"EQUIJOIN result has ambiguous attribute names: {collisions}. " - f"Hint: Use RENAME on one relation first." - ) return Schema(left_s.attributes + right_attrs) @property @@ -658,7 +675,7 @@ def relation_name(self) -> str: # --------------------------------------------------------------------------- -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Semijoin(BaseRelation): """⋉ (Semijoin): Left tuples that have a match in right. @@ -692,7 +709,7 @@ def relation_name(self) -> str: return f"({self.left.relation_name} ⋉ {self.right.relation_name})" -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Antijoin(BaseRelation): """▷ (Antijoin): Left tuples with NO match in right. @@ -726,7 +743,7 @@ def relation_name(self) -> str: return f"({self.left.relation_name} ▷ {self.right.relation_name})" -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class OuterJoin(BaseRelation): """⟕⟖⟗ (Outer Join): Join preserving unmatched tuples. @@ -777,7 +794,7 @@ def relation_name(self) -> str: # --------------------------------------------------------------------------- -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Grouping(BaseRelation): """γ (Grouping/Aggregation). @@ -843,7 +860,7 @@ def relation_name(self) -> str: # --------------------------------------------------------------------------- -@dataclass(frozen=True, repr=False) +@dataclass(frozen=True, eq=False, repr=False) class Division(BaseRelation): """÷ (Division): Tuples associated with ALL tuples in the divisor. diff --git a/tests/test_all.py b/tests/test_all.py index 6028ffb..b16b4b9 100644 --- a/tests/test_all.py +++ b/tests/test_all.py @@ -979,3 +979,114 @@ def test_real_agg_attrs_still_work(self, sp_data): result = sp.group("sno", n=count("pno"), t=sum_("qty")).collect() result_dict = {row[0]: (row[1], row[2]) for row in result} assert result_dict["S1"] == (6, 1300) + + +class TestNodeIdentity: + # Expression-node dataclasses use eq=False, so == and hash() are identity- + # based. Without it, the generated __eq__ reaches Attr.__eq__ (which returns + # a Predicate whose __bool__ raises) and the generated __hash__ chokes on + # the dict fields of Grouping/Rename. These tests pin the identity semantics. + + def test_node_equals_itself(self, sp_data): + s, p, sp, engine = sp_data + node = s.select(s.city == "London") + assert (node == node) is True + + def test_distinct_nodes_not_equal_no_raise(self, sp_data): + # Two structurally-identical Selections off the SAME leaf used to reach + # Attr.__eq__ and raise PredicateError on ==. Identity makes them unequal. + s, p, sp, engine = sp_data + a = s.select(s.city == "London") + b = s.select(s.city == "London") + assert a is not b + assert (a == b) is False + assert (a != b) is True + + def test_nested_predicate_child_equality_no_raise(self, sp_data): + # Worst case: a Grouping whose child is itself a Selection holding an + # Attr comparison. eq=True would recurse into Attr.__eq__ and raise. + s, p, sp, engine = sp_data + ga = sp.select(sp.sno == "S1").group("sno", n=count()) + gb = sp.select(sp.sno == "S1").group("sno", n=count()) + assert (ga == gb) is False + + def test_membership_check_no_raise(self, sp_data): + s, p, sp, engine = sp_data + a = s.select(s.city == "London") + b = s.select(s.city == "London") + assert a not in [b] + assert a in [a, b] + + def test_grouping_hashable_and_identity_keyed(self, sp_data): + # Grouping.aggs is a dict; the generated __hash__ raised TypeError. + s, p, sp, engine = sp_data + g1 = sp.group("sno", n=count()) + g2 = sp.group("sno", n=count()) + assert isinstance(hash(g1), int) + assert len({g1, g2}) == 2 + assert g1 in {g1, g2} + + def test_rename_hashable_and_identity_keyed(self, sp_data): + # Rename.mapping is a dict; same TypeError-on-hash failure mode. + s, p, sp, engine = sp_data + r1 = s.rename(town="city") + r2 = s.rename(town="city") + assert isinstance(hash(r1), int) + assert len({r1, r2}) == 2 + d = {r1: "first"} + assert d[r1] == "first" + assert r2 not in d + + def test_every_node_dataclass_uses_identity_eq_and_hash(self): + # Belt-and-suspenders: any future decorator regression that re-enables + # structural eq/hash on a node class trips this immediately. + from coddpiece.relation import ( + Antijoin, + CrossProduct, + Division, + Equijoin, + Grouping, + NaturalJoin, + OuterJoin, + Projection, + Rename, + Selection, + Semijoin, + ThetaJoin, + ) + node_classes = [ + Selection, Projection, Rename, CrossProduct, NaturalJoin, + ThetaJoin, Equijoin, Semijoin, Antijoin, OuterJoin, Grouping, + Division, + ] + for cls in node_classes: + assert cls.__eq__ is object.__eq__, f"{cls.__name__} has structural __eq__" + assert cls.__hash__ is object.__hash__, f"{cls.__name__} has structural __hash__" + + +class TestEquijoinEagerValidation: + # Equijoin's ambiguous-output-name check now runs at construction + # (__post_init__), matching every other node's eager-validation contract, + # instead of firing lazily on a later _schema()/schema() call. + def test_equijoin_collision_raises_at_construction(self, engine): + # Both relations carry a non-join 'shared' column, so the equijoin + # output would have two columns named 'shared' -> ambiguous. + a = engine.create( + "eq_a", {"k": int, "shared": int, "av": int}, rows=[(1, 9, 100)] + ) + b = engine.create( + "eq_b", {"j": int, "shared": int, "bv": int}, rows=[(1, 9, 200)] + ) + with pytest.raises(SchemaError, match="ambiguous"): + a.equijoin(b, "k", "j") + + def test_equijoin_dropped_right_column_does_not_collide(self, engine): + # The dropped right join column ('j') must not be counted as a + # collision; construction succeeds with the correct schema and rows. + a = engine.create( + "eq_c", {"k": int, "shared": int, "av": int}, rows=[(1, 9, 100)] + ) + c = engine.create("eq_d", {"j": int, "cv": int}, rows=[(1, 5)]) + joined = a.equijoin(c, "k", "j") # must not raise + assert joined.schema().names() == ("k", "shared", "av", "cv") + assert joined.collect() == [(1, 9, 100, 5)]