diff --git a/ISSUES.md b/ISSUES.md index e5686d9..5f3039f 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -53,7 +53,7 @@ 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 @@ -61,11 +61,11 @@ Legend: `[ ]` open · `[x]` resolved · `[~]` won't fix / by design. *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. diff --git a/coddpiece/compiler.py b/coddpiece/compiler.py index 2dab7c3..1f2e9cf 100644 --- a/coddpiece/compiler.py +++ b/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 ) diff --git a/coddpiece/predicates.py b/coddpiece/predicates.py index 30a8758..7ba482b 100644 --- a/coddpiece/predicates.py +++ b/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() diff --git a/tests/test_all.py b/tests/test_all.py index b16b4b9..276c62e 100644 --- a/tests/test_all.py +++ b/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