refactor: remove dead predicate .sql() path; hoist import; escape Literal repr

ISSUES.md

@@ -53,19 +53,19 @@ Legend: `[ ]` open · `[x]` resolved · `[~]` won't fix / by design.
 
 ## P3 — Dead code & imports
 
-- [ ] **D1 · `Predicate.sql()` / `CompoundPredicate.sql()` / `NotPredicate.sql()`
+- [x] **D1 · `Predicate.sql()` / `CompoundPredicate.sql()` / `NotPredicate.sql()`
   are dead and dialect-blind.** Nothing outside the predicate hierarchy calls
   them; the real path is `Compiler._compile_predicate`. They hardcode `?`
   placeholders and unqualified column names, contradicting the Dialect invariant
   and trapping any maintainer who calls `pred.sql()`.
   *Fix:* remove the three methods (and the now-unused `SQL_OPERATORS` import in
   them stays available for the compiler).
 
-- [ ] **D2 · Function-local import in the compiler.** `_compile_predicate` does
+- [x] **D2 · Function-local import in the compiler.** `_compile_predicate` does
   `from .predicates import SQL_OPERATORS` though `predicates` is already imported
   at module top with no circular-import reason. *Fix:* hoist to the top import.
 
-- [ ] **D3 · `Literal.__repr__` does not escape embedded quotes.** Strings are
+- [x] **D3 · `Literal.__repr__` does not escape embedded quotes.** Strings are
   wrapped with a hand-written `"…"`; a value containing `"` breaks the algebra
   notation. *Fix:* escape, or use `repr`-based quoting.
 

coddpiece/compiler.py

@@ -23,7 +23,14 @@
 
 from typing import TYPE_CHECKING, Any
 
-from .predicates import Attr, CompoundPredicate, Literal, NotPredicate, Predicate
+from .predicates import (
+    SQL_OPERATORS,
+    Attr,
+    CompoundPredicate,
+    Literal,
+    NotPredicate,
+    Predicate,
+)
 from .relation import (
     Antijoin,
     BaseRelation,
@@ -594,7 +601,6 @@ def _compile_predicate(
         # The alias/schema params are only non-None for join predicates, where
         # column references must be table-qualified to resolve ambiguity.
         if isinstance(pred, Predicate):
-            from .predicates import SQL_OPERATORS
             left_sql = self._compile_pred_operand(
                 pred.left, left_alias, right_alias, left_schema, right_schema
             )

coddpiece/predicates.py

@@ -28,12 +28,15 @@ class Literal:
     # bare) gives the compiler a uniform Attr|Literal dispatch: Attr -> column
     # reference, Literal -> `?` placeholder + appended param. This is what
     # makes the "literals never interpolated" SQL-safety invariant easy to
-    # enforce in compiler.py and Predicate.sql().
+    # enforce in compiler.py (see Compiler._compile_pred_operand).
     value: Any
 
     def __repr__(self) -> str:
+        # Strings render in double-quoted algebra form. Escape any embedded
+        # double-quote so a value containing one does not render unbalanced.
         if isinstance(self.value, str):
-            return f'"{self.value}"'
+            escaped = self.value.replace('"', '\\"')
+            return f'"{escaped}"'
         return repr(self.value)
 
 
@@ -172,29 +175,6 @@ def algebra(self) -> str:
         sym = ALGEBRA_SYMBOLS[self.op]
         return f"{left}{sym}{right}"
 
-    def sql(self, dialect: Any = None) -> tuple[str, list]:
-        """Render as SQL fragment with parameters."""
-        # Returns (sql_string, param_values). Attr operands become column names
-        # in the SQL string; Literal operands become `?` placeholders with
-        # values appended to the params list. This prevents SQL injection.
-        # The left-then-right append order must match the `?` order in the string.
-        params: list = []
-
-        if isinstance(self.left, Attr):
-            left_sql = self.left.name
-        else:
-            left_sql = "?"
-            params.append(self.left.value)
-
-        if isinstance(self.right, Attr):
-            right_sql = self.right.name
-        else:
-            right_sql = "?"
-            params.append(self.right.value)
-
-        sql_op = SQL_OPERATORS[self.op]
-        return f"{left_sql} {sql_op} {right_sql}", params
-
     def __repr__(self) -> str:
         return self.algebra()
 
@@ -228,15 +208,6 @@ def algebra(self) -> str:
         sym = ALGEBRA_SYMBOLS[self.op]
         return f"({self.left.algebra()} {sym} {self.right.algebra()})"
 
-    def sql(self, dialect: Any = None) -> tuple[str, list]:
-        left_sql, left_params = self.left.sql(dialect)
-        right_sql, right_params = self.right.sql(dialect)
-        sql_op = SQL_OPERATORS[self.op]
-        # Params are concatenated left-before-right, matching the order of `?`
-        # placeholders in the generated SQL string. This ordering invariant is
-        # critical — swapping would bind values to the wrong placeholders.
-        return f"({left_sql} {sql_op} {right_sql})", left_params + right_params
-
     def __repr__(self) -> str:
         return self.algebra()
 
@@ -269,13 +240,6 @@ def __bool__(self) -> bool:
     def algebra(self) -> str:
         return f"¬({self.operand.algebra()})"
 
-    def sql(self, dialect: Any = None) -> tuple[str, list]:
-        # Always parenthesize the inner fragment — NOT has higher precedence
-        # than AND/OR in SQL, so `NOT a AND b` would bind wrong. Parens make
-        # the generated SQL precedence-safe regardless of what `operand` is.
-        inner_sql, params = self.operand.sql(dialect)
-        return f"NOT ({inner_sql})", params
-
     def __repr__(self) -> str:
         return self.algebra()
 

tests/test_all.py

@@ -1090,3 +1090,43 @@ def test_equijoin_dropped_right_column_does_not_collide(self, engine):
         joined = a.equijoin(c, "k", "j")  # must not raise
         assert joined.schema().names() == ("k", "shared", "av", "cv")
         assert joined.collect() == [(1, 9, 100, 5)]
+
+
+class TestPredicateDeadCodeRemoval:
+    # The predicate classes used to carry a dead, dialect-blind .sql() method
+    # (hardcoded "?" placeholders) that nothing outside the predicate hierarchy
+    # called — the real path is Compiler._compile_predicate. These tests lock in
+    # its removal and the Literal repr quote-escaping that landed with it.
+    def test_attr_eq_literal_with_embedded_quote(self, sp_data):
+        s, p, sp, engine = sp_data
+        pred = s.city == 'a"b'
+        assert isinstance(pred, Predicate)
+        assert pred.algebra() == 'city="a\\"b"'
+
+    def test_attr_eq_plain_string_unchanged(self, sp_data):
+        s, p, sp, engine = sp_data
+        assert (s.city == "London").algebra() == 'city="London"'
+        assert (s.status == 20).algebra() == "status=20"
+
+    def test_predicate_has_no_sql_method(self, sp_data):
+        from coddpiece.predicates import (
+            CompoundPredicate,
+            NotPredicate,
+            Predicate,
+        )
+        assert not hasattr(Predicate, "sql")
+        assert not hasattr(CompoundPredicate, "sql")
+        assert not hasattr(NotPredicate, "sql")
+
+    def test_compound_and_not_sql_still_compiles(self, sp_data):
+        # End-to-end guard: removing the predicate .sql() helpers did not
+        # regress the real compile path.
+        s, p, sp, engine = sp_data
+        sql = s.select((s.city == "London") & (s.status > 10)).sql()
+        assert "WHERE" in sql
+        assert "AND" in sql
+        assert "params:" in sql
+        assert "London" in sql
+        not_sql = s.select(~(s.city == "London")).sql()
+        assert "NOT" in not_sql
+        assert "params:" in not_sql