refactor(gfql/binder + lowering): text-only WhereClause.expr → expr_tree readers (#1213 sub-PR B)

[lmeyerov]

Summary

Sub-PR B of #1213 (umbrella #1200 slice 5). Migrate all text-only readers of WhereClause.expr.text to consume boolean_expr_to_text(WhereClause.expr_tree) instead, leveraging the parser invariant (expr is None) == (expr_tree is None) established by #1214.

Reader-side only. Writer-side (drop expr= from WhereClause(...) constructions) and final field removal are sub-PRs D + E, owned by the colleague who landed #1214.

Sites migrated

• graphistry/compute/gfql/frontends/cypher/binder.py::_where_predicates
  • Dropped the dead elif where.expr is not None fallback. Per refactor(#1213 sub-PR A): lift boolean_expr_to_text + establish expr_tree invariant #1214's invariant the branch is unreachable for parser-produced WhereClauses; it was defensive code for hand-built fixtures only.
• graphistry/compute/gfql/cypher/lowering.py::_reject_unsupported_where_expr_forms (gate + .text.strip()).
• graphistry/compute/gfql/cypher/lowering.py::_reject_unsupported_variable_length_where_pattern_predicates (error-reporting value=.text).
• graphistry/compute/gfql/cypher/lowering.py::_check_expr callers (3 sites — query.where, reentry_where, and the post-binding query.where re-check).

Diff: 17 insertions, 30 deletions across 4 files.

Out of scope (sub-PR C)

• ExpressionText passthrough callers (rewrite_expr in lowering at L7515/7533; _extract_relationship_type_where at L8131; pre_join_filters.append at L7998; row_where assignment at L6322-6329; _rewrite_shortest_path_expr_text at ast_normalizer.py:432-436).
• Span-access readers at lowering.py:8143-8144 (entangled with the passthrough at L8131; migrate together in C).

Test changes

Removed test_where_predicates_with_expr_tree_none_falls_back_to_expr_text from test_binder_expr_tree.py. That test exercised the now-dropped binder fallback via a hand-built WhereClause(expr=..., expr_tree=None) fixture — a shape that violates the parser invariant established by #1214 and is unreachable from any real parser output. The sibling test test_where_predicates_with_no_expr_at_all_returns_empty still covers the legitimate no-expr-tree case.

Test plan

• pytest graphistry/tests/compute/gfql/ — 1538 passed (was 1539, minus the deleted test); 80 skipped, 15 xfailed.
• mypy graphistry/compute/gfql/cypher/lowering.py graphistry/compute/gfql/frontends/cypher/binder.py — clean.

Coordination

Sub-PR B per the ownership split agreed on #1213:

Sub-PR	Owner	State
A — helper lift + invariant	colleague	✅ merged (#1214)
B — text-only readers	us	this PR
C — passthrough readers	us	next
D — drop expr= from constructions	colleague	after C lands
E — drop the expr field	colleague	likely combined with D

C will follow on a fresh branch off this one's merge into master, and will handle the rewrite_expr integration plus the latent text/tree staleness bug at lowering.py:7515/7533 (where replace(where_clause, expr=rewrite_expr(...)) currently leaves expr_tree pointing at pre-rewrite text — passes the invariant but is semantically inconsistent).

Closes

Partial closure of #1213; full closure deferred until D + E.

Related

• gfql/ir: expose Lark and_op/or_op tree in WhereClause so passes walk structure instead of re-parsing text #1200 — umbrella IR cleanup (Lark = single source of truth for boolean structure)
• gfql/ir: migrate WhereClause.expr readers to expr_tree, then remove the field (#1200 slice 5) #1213 — this slice (WhereClause.expr field removal)
• refactor(#1213 sub-PR A): lift boolean_expr_to_text + establish expr_tree invariant #1214 — sub-PR A (helper lift + invariant)
• feat(gfql/binder): walk expr_tree, emit one BoundPredicate per top-level AND (#1200 slice 2/N) #1207 — slice 2 (binder walks expr_tree)
• fix(cypher): walk expr_tree to lift label-only WHERE predicates (#1194) #1209 — slice 3a (parser walks expr_tree, closes gfql/cypher: resolve Lark ambiguity between where_predicates and expr (grammar-level follow-up to #1125) #1194)

[lmeyerov]

PR Review: #1216 — refactor(gfql/binder + lowering): text-only WhereClause.expr → expr_tree readers (#1213 sub-PR B)

Branch: m4-1213-where-expr-readers @ 8f3fbd146
Base: master @ bb72ca0ae (post-#1209 + #1214)
Stack: not stacked, no downstream PRs
Files changed: binder.py, lowering.py, test_binder_expr_tree.py, CHANGELOG.md
LOC: +17 / −30 (1 commit)
CI: 131/131 conclusive checks pass; mergeable; awaiting REVIEW_REQUIRED.

---

Blockers

None.

Important

None.

Suggestions

1. [Quality, trivial] Import placement of from graphistry.compute.gfql.cypher._boolean_expr_text import boolean_expr_to_text at lowering.py:107 is alphabetically correct but the leading underscore on _boolean_expr_text.py is an inherited convention from refactor(#1213 sub-PR A): lift boolean_expr_to_text + establish expr_tree invariant #1214. Cosmetic — not worth a follow-up.

(Wave 1 also surfaced a literal-atom fidelity concern and a deleted-test coverage concern; both were rejected/downgraded under adversarial pressure with code-level proof. See adversarial/wave-1.md.)

Human checks required

1. Approve and merge. No code-level concerns.
2. Confirm Closes #1213 is not set (this PR only completes sub-PR B; full closure deferred until D + E land).
3. Post coordination signal on gfql/ir: migrate WhereClause.expr readers to expr_tree, then remove the field (#1200 slice 5) #1213 thread when merged: "B merged, C in flight."

Rejected / False positives

Wave 1 finding	Disposition	Proof
boolean_expr_to_text literal-atom fidelity ("True" vs "true") breaking _check_expr / reject*	DOWNGRADED	None of the 5 migrated callsites distinguishes case in boolean-literal handling; _check_expr matches alias-dot-property/function syntax, not literals; _reject_* uses fullmatch on grouped-alias pattern. Fidelity loss invisible.
Deleted test_where_predicates_with_expr_tree_none_falls_back_to_expr_text leaves coverage gap	REJECTED	The fixture shape it constructed is invariant-impossible (per #1214's test_where_clause_expr_tree_invariant.py); covering unreachable behavior is dead-test work. The invariant test is the load-bearing guard.
IR-direct construction of WhereClause could violate invariant + silently drop predicates	NO FINDING	Search confirms WhereClause is constructed only in parser.py (5 sites) and ast_normalizer.py (1 site); all cypher-routed. No IR-direct construction in repo. Future violations caught by the invariant test.

Methodology

Per ~/Work/graphistry/.agents/skills/review/SKILL.md. Single wave (justified by tight scope: 4 files, +17/-30 LOC, mechanical migration with full test parity and identical structure to the prior PR #1209 review). 11 dimensions assessed in waves/wave-1/report.md. Adversarial pressure test in adversarial/wave-1.md. Convergence on Wave 1.

Recommendation

Approve and merge with --admin. No defects. The single confirmed SUGGESTION is a trivial style nit not worth blocking. After merge, branch off post-merge master for sub-PR C and post coordination signal on #1213 thread for the colleague to start D.