refactor(#1213 sub-PR D+E): drop WhereClause.expr field#1220
Merged
refactor(#1213 sub-PR D+E): drop WhereClause.expr field#1220
Conversation
Final slice of #1200 / #1213. Removes the legacy raw-text WHERE field now that all production readers (binder, lowering, ast_normalizer) consume expr_tree-derived text via boolean_expr_to_text() per sub-PR B/C. Sub-PR D — drop expr= from writer construction sites: - parser.py: _mixed_where_clause, where_clause non-structured branch, generic_where_clause fallback (the three sites that synthesized BooleanExpr atoms in #1214); also strip expr=None from the three structured-path constructions - ast_normalizer.py: drop expr= forwarder + switch gate to expr_tree Sub-PR E — remove the field: - ast.py: drop WhereClause.expr; update docstring to two-shape contract (structured vs tree, with mixed pattern+expr as the third combination) - test_where_clause_expr_tree_invariant.py: rewrite from the now-vacuous (expr is None) == (expr_tree is None) symmetry to the surviving routing-shape contract (structured / tree / mixed) - test_parser.py, test_boolean_expr.py, test_ast_normalizer.py, test_where_bool_conformance.py: replace `where.expr is None/not None` with `where.expr_tree is None/not None`; replace `where.expr.text` with `boolean_expr_to_text(where.expr_tree)`; drop the slice-1 backward-compat test that asserted both fields were populated NOT YET REBASED ONTO POST-C MASTER. This commit produces the expected 93 test failures from lowering/ast_normalizer readers that still access .expr; sub-PR C migrates them. Rebase onto post-C master before pushing or running CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…calls Sub-PR C (#1218) introduced two ``replace(where, expr=rewritten, expr_tree=new_tree)`` calls in ``ast_normalizer.py::_rewrite_where`` and ``lowering.py::_rewrite_where_clause_and_resync``. These passed both fields because the field still existed at C's merge time. After this PR removes the field, the ``expr=`` arg becomes invalid keyword to the dataclass constructor. Drop it from both sites; ``expr_tree`` is the single source of truth post-D+E. Also drops the now-unused ``expr=...`` and ``expr=None`` args from three hand-built ``WhereClause(...)`` test fixtures in ``test_binder_expr_tree.py`` that broke for the same reason, plus the now-unused ``ExpressionText`` import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rage
Two review waves (focus + general per the new review policy) converged on:
1. **WhereClause docstring missing the "mixed" routing shape** (IMPORTANT, both
waves). The new docstring listed only 2 shapes; reality is 3 (and arguably
subdivided further by which kind of structured `predicates` populated).
Rewrote to enumerate all three observable shapes, distinguishing
`WherePredicate` from `WherePatternPredicate` in the structured case.
2. **Invariant test missing parser construction paths** (IMPORTANT, focus
wave). Added `MATCH (a) WHERE (a)-[]->(:Admin) RETURN a` covering
`where_pattern_only_clause` (structured, no AND, single
`WherePatternPredicate`). `expr_and_where_pattern_clause` (expr-first
mixed) doesn't currently parse; `where_clause` _ExpressionSlice branch is
Lark-internal routing already exercised by integration tests.
3. **Stale binder comment claiming `predicates`/`expr_tree` mutually exclusive**
(SUGGESTION, general wave). They aren't — the mixed shape populates
both — and the binder body correctly handles all three shapes. Updated
the comment to match the AST docstring's 3-shape contract.
4. **CHANGELOG polish** (SUGGESTIONS, both waves): clarified count
("9 query forms across the 3 shapes" rather than the Cartesian-reading
"3 × 8"), added an API note that `WhereClause` is exported and external
constructors must drop `expr=` kwarg.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final slice of #1213 (umbrella #1200). Removes the legacy raw-text
WhereClause.expr: Optional[ExpressionText]field now that all production readers consumeexpr_tree-derived text viaboolean_expr_to_text().Sub-PR D (writers): drop
expr=from everyWhereClause(...)construction site.parser.pythat synthesizedBooleanExpratoms in refactor(#1213 sub-PR A): lift boolean_expr_to_text + establish expr_tree invariant #1214 (_mixed_where_clause,where_clausenon-structured branch,generic_where_clausefallback)expr=Noneast_normalizer.py:509forwarder; gate switched fromquery.where.expr is not Nonetoquery.where.expr_tree is not NoneSub-PR E (field): drop the field declaration from
cypher/ast.py. Rewrote theWhereClausedocstring to enumerate all 3 observable routing shapes:predicatespopulated,expr_tree is None(eitherWherePredicateentries or a singleWherePatternPredicate)predicates == (),expr_treepopulatedWHERE pattern AND expr)Combined as one PR because the invariant test (
test_where_clause_expr_tree_invariant.py, from #1214) becomes vacuous when the field is gone — keeping D + E together avoids landing a degenerate intermediate state.Test surface:
test_where_clause_expr_tree_invariant.pyrewritten from the now-vacuous(expr is None) == (expr_tree is None)symmetry to a routing-shape contract (9 query forms across the 3 shapes)test_parser.py,test_boolean_expr.py,test_ast_normalizer.py,test_where_bool_conformance.pymigrated toboolean_expr_to_text(where.expr_tree)test_expr_tree_and_expr_both_populated_for_compat) deleted as obsoletetest_binder_expr_tree.pyupdated to dropexpr=kwargIncidental cleanup (commit
44d46ca96): Sub-PR C (#1218) introduced tworeplace(where, expr=rewritten, expr_tree=new_tree)calls inast_normalizer.py::_rewrite_whereandlowering.py::_rewrite_where_clause_and_resync— valid at C's merge time, broken once D+E removes the field. Drops theexpr=arg from both.ExpressionTextitself stays — other AST nodes still use it (ReturnItem,WithItem,OrderByItem, page values, etc.).Review
Reviewed via the project review skill (
~/Work/graphistry/.agents/skills/review/SKILL.md) under the new double-wave protocol (focus + full general, parallel). Findings synthesized in commita1822b8c4:where_pattern_only_clausecoverage (focus) → addedpredicates/expr_treemutually exclusive (general) → fixedAdversarial pass cleared: span fidelity, literal-atom caveat, state-loss in
replace()drops, test substitution semantics, deleted backward-compat test.Test plan
graphistry/tests/compute/gfql/WhereClause.expraccesses (only comments and unrelatedplan.expr)Closes
Refs
expr_treeexposed): feat(gfql/ir): expose Lark boolean-expression tree as WhereClause.expr_tree (#1200 slice 1/N) #1202🤖 Generated with Claude Code