fix: identity equality for expression nodes; eager equijoin collision check

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.

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.
 

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)]