Conversation
…WhereClause.expr → expr_tree readers (#1213 sub-PR C) Second read-side slice of #1213. Migrate all `ExpressionText`-passthrough readers of `WhereClause.expr` to consume `WhereClause.expr_tree`-derived values via two new helpers in `cypher/lowering.py`: - `_where_clause_expr_text(where)` — synthesizes a fresh `ExpressionText` from `where.expr_tree` using `where.span` (which equals the original `where.expr.span` for parser-produced WhereClauses, both derived from the same `_span_from_meta(meta)` of `generic_where_clause`) so error positions match master's behavior. - `_rewrite_where_clause_and_resync(where, rewrite, field)` — applies a text rewrite via the existing `(ExpressionText, str) -> ExpressionText` callback shape and resynchronizes `expr_tree` to a single-atom `BooleanExpr` carrying the rewritten text (Option B of the C decision posted on #1213; documented in plan.md). Side-effect fix: closes the latent text/tree staleness bug on master post-#1214 where `replace(where, expr=rewrite(where.expr, ...))` left `expr_tree` pointing at the pre-rewrite text. Now both fields stay in sync after rewriting. Sites migrated: - `lowering.py:6322` — dynamic row-where extraction (`_extract_relationship_type_where(query.where.expr, ...)` + `row_where` passthrough → use `_where_clause_expr_text`). - `lowering.py:7530-7538` — reentry-where rewrite of remaining wheres (use `_rewrite_where_clause_and_resync`). - `lowering.py:7550-7553` — reentry-where rewrite of primary `query.reentry_where` (same helper). - `lowering.py:7998` — `pre_join_filters.append(query.where.expr)` → append the synthesized ExpressionText. - `lowering.py:8176-8191` — connected-OPTIONAL-MATCH where lowering (`_extract_relationship_type_where` + `_unsupported` error reporting with `.text` and `.span.line/.column`) → all consume the synthesized ExpressionText uniformly. - `ast_normalizer.py::_rewrite_where` — shortest-path expr rewrite via `_rewrite_shortest_path_expr_text` (inline equivalent of the lowering helper since this module only has one callsite). Out of scope (deferred to D + E): - `ast_normalizer.py:520-526` constructor passthrough (`expr=query.where.expr` inside a `WhereClause(...)` re-construction) — left for sub-PR D to drop along with the other writer-side construction-site updates in parser.py. The gate at L520 is migrated (`expr is not None` → `expr_tree is not None`). Verified: - 1538/1538 GFQL tests pass; no regressions. - `mypy` clean on `cypher/lowering.py` and `cypher/ast_normalizer.py`. Coordination: this is sub-PR C per the split agreed on #1213. Ready for the colleague's combined sub-PRs D + E (drop `expr=` from `WhereClause(...)` constructions; remove the field) once this lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
PR Review: #1218 — refactor(gfql/lowering + ast_normalizer): ExpressionText-passthrough WhereClause.expr → expr_tree readers (#1213 sub-PR C)Branch: BlockersNone. ImportantNone. Suggestions
(Wave 1 also surfaced an Option-B structural-collapse concern and a missing-direct-unit-test concern; both rejected/downgraded under adversarial pressure with code-level proof. See Human checks required
Rejected / False positives
MethodologyPer RecommendationApprove and merge with |
This was referenced Apr 26, 2026
Closed
lmeyerov
added a commit
that referenced
this pull request
Apr 26, 2026
* refactor(#1213 sub-PR D+E): drop WhereClause.expr field 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> * docs(#1213 sub-PR D+E): pre-stage CHANGELOG entry for field removal Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#1213 sub-PR D+E): drop leftover expr= from sub-PR C's replace() 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> * docs(#1213 sub-PR D+E): apply review findings — docstring + test coverage 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> --------- 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
Sub-PR C of #1213. Migrate all ExpressionText-passthrough readers of
WhereClause.exprto consumeWhereClause.expr_tree-derived values. Builds on sub-PR B (#1216, merged) which handled the text-only readers.Adds two helpers in
cypher/lowering.py:_where_clause_expr_text(where) -> Optional[ExpressionText]— synthesizes a freshExpressionTextfromwhere.expr_treeusingwhere.span(which equals the originalwhere.expr.spanfor parser-produced WhereClauses, both derived from the same_span_from_meta(meta)of thegeneric_where_clauserule). Preserves master's error-position semantics._rewrite_where_clause_and_resync(where, rewrite, field) -> WhereClause— applies a text rewrite via the existing(ExpressionText, str) -> ExpressionTextcallback shape and resynchronizesexpr_treeto a single-atomBooleanExprcarrying the rewritten text. Option B of the C decision (text round-trip + single-atom resynthesis) per coordination on gfql/ir: migrate WhereClause.expr readers to expr_tree, then remove the field (#1200 slice 5) #1213.Side-effect fix
Closes the latent text/tree staleness bug that existed on master post-#1214:
replace(where, expr=rewrite(where.expr, ...))used to leaveexpr_treepointing at the pre-rewrite text. The invariant(expr is None) == (expr_tree is None)still passed (both still non-None), but text and tree were semantically out of sync. Now both stay in sync after rewriting.Sites migrated
lowering.py:6322(dynamic row-where)_extract_relationship_type_where(query.where.expr, ...)+row_where = query.where.expr_where_clause_expr_textlowering.py:7530-7538(reentry remaining wheres)replace(where_clause, expr=rewrite_expr(where_clause.expr, "where"))_rewrite_where_clause_and_resynclowering.py:7550-7553(reentry primary where)query.reentry_where_rewrite_where_clause_and_resynclowering.py:7998(pre_join_filters)pre_join_filters.append(query.where.expr)_where_clause_expr_text(synthesize then append)lowering.py:8176-8191(connected OPTIONAL MATCH)_extract_relationship_type_where(where.expr, ...)+ entangled_unsupported(value=.text, line=.span.line, column=.span.column)_where_clause_expr_text(uniform synthesis covers both sub-uses)ast_normalizer.py::_rewrite_wherereplace(where, expr=_rewrite_shortest_path_expr_text(where.expr, ...))Out of scope (deferred to D + E)
ast_normalizer.py:520-526— constructor passthrough (expr=query.where.exprinside aWhereClause(...)re-construction). Sub-PR D drops theexpr=arg as part of the writer-side migration. The gate at L520 is migrated (expr is not None→expr_tree is not None).Test plan
pytest graphistry/tests/compute/gfql/— 1538 passed, 80 skipped, 15 xfailed. No regressions.mypy graphistry/compute/gfql/cypher/lowering.py graphistry/compute/gfql/cypher/ast_normalizer.py— clean.Coordination
Sub-PR C per the ownership split agreed on #1213:
expr=from constructionsexprfieldAfter this lands, the colleague (D + E owner) is unblocked. Will post coordination signal on #1213 when this merges.
Closes
Partial closure of #1213; full closure on D + E.
Related
expr_tree)expr_tree)