fix(gfql): rows(table=...) must survive a named middle on both chain surfaces by lmeyerov · Pull Request #1788 · graphistry/pygraphistry

lmeyerov · 2026-07-27T11:02:11Z

The bug

The named-middle rewrite turns [...named ops..., rows()] into rows(binding_ops=<middle>) so a Cypher multi-alias RETURN lowers to a bindings table. It excluded calls already carrying binding_ops, source or alias_endpoints — but not a non-default table. So merely naming an op in the middle changed which table came back:

middle	result on master
unnamed	✅ returns the edges table
named, odd length	⚠️ silently returns the bindings table — no error, `table=` ignored
named, even length (path ending on an edge)	❌ `require ... a single connected alternating node/edge path`

The quiet one is the worse one: rows(table="edges") after MATCH (a)-[r]-(b) just hands back alias columns.

Both chain surfaces carry the rewrite independently (compute/chain.py and the native polars gfql/lazy/engine/polars/chain.py), so both need the guard — the per-surface mutation checks below show fixing one leaves the other wrong.

How it was found

Chasing why LDBC IS3 is the only interactive-short cell with a competitor number and no GFQL score (status=primitive_gap, rows=None, both engines). Its adapter runs a two-pass workaround whose edge half is exactly (person)-[r:KNOWS]- + rows(table="edges") — the even-length case above.

IS3 is what found this; the fix is not written for it. The guard keys on the call's own params, not on any query text, label, or LDBC shape, and helps every caller of rows(table=...) after a named traversal.

The guard keys on a non-default table, not on `is None`

rows() declares table: str = "nodes" and always emits it, so params.get("table") is None is never true. My first attempt used that and disabled the rewrite outright, breaking the IS6 bindings path (5 regressions). Those surfaced only because I re-baselined against current master — the baseline I had from an earlier branch no longer matched after #1781/#1785 landed.

Residual limitation, now pinned by a test rather than left to be rediscovered: an explicit rows(table="nodes") is byte-identical to a bare rows() at the params level and still rewrites. Distinguishing them would need rows() to default table=None, a wire-format change and out of scope here.

Tests

10 cases across both engines: the even-length (IS3) case, the silent odd-length case, named-vs-unnamed equivalence, the documented table="nodes" limitation, and the negative side — a bare rows() after a named middle must still get the bindings table, without which "never rewrite" would pass and break every Cypher multi-alias RETURN.

Mutation-checked per surface: removing the pandas guard fails 3 pandas cases with polars green; removing the polars guard fails 3 polars cases with pandas green.

Verification

All in-container on dgx-spark. graphistry/tests/compute with --gpus all: 7030 passed against master 3f2128ea's 7020 (+10, the new file), with an identical 9-failure set (pre-existing dask/cuDF coercion).

Scope note

This makes IS3 scoreable; it does not make it fast. The adapter still joins and sorts in Python, and that adapter_two_pass_workaround scope needs the same disclosure IS2 gets. Separately I verified the single-query IS3 form (MATCH (n {id})-[r]-(f) RETURN f.id, r.creationDate ORDER BY ...) already works on both engines on master, so moving the adapter onto it and retiring the workaround is the better follow-up. No IS3 flip is claimed here.

🤖 Generated with Claude Code

https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB

…surfaces The named-middle rewrite turns `[...named ops..., rows()]` into `rows(binding_ops=<middle>)` so a Cypher multi-alias RETURN lowers to a bindings table. It excluded calls already carrying `binding_ops`, `source` or `alias_endpoints` — but not a non-default `table`. So merely NAMING an op in the middle changed which table came back: * odd-length named middle -> the rewrite fired and the caller silently got the BINDINGS table instead of the edges table. No error; `table=` ignored. * even-length named middle -> a path ending on an EDGE, so the rewritten op list is not an alternating node/edge path and it hard-errored with "require ... a single connected alternating node/edge path". * UNNAMED middle -> correct. Which is what made this look shape-specific rather than naming-specific. Found while chasing why LDBC IS3 is the only interactive-short cell with a competitor number and no GFQL score: its adapter runs a two-pass workaround whose edge half is exactly `(person)-[r:KNOWS]-` + `rows(table="edges")`. Both surfaces need the guard — `compute/chain.py` and the native polars `gfql/lazy/engine/polars/chain.py` carry the rewrite independently, and the mutation checks below show fixing one leaves the other wrong. THE GUARD KEYS ON A NON-DEFAULT TABLE, NOT ON `is None`. `rows()` declares `table: str = "nodes"` and always emits it, so `params.get("table") is None` is never true — my first attempt used that and disabled the rewrite outright, breaking the IS6 bindings path (5 regressions). Those were caught only by re-baselining against current master rather than against the stale baseline I had from an earlier branch, which no longer matched after #1781/#1785 landed. The residual cost is that an EXPLICIT `rows(table="nodes")` is byte-identical to a bare `rows()` at the params level and still rewrites; that limitation is now pinned by a test rather than left to be rediscovered. Distinguishing them would need `rows()` to default `table=None`, a wire-format change and out of scope. Tests: 10 cases across both engines — the even-length (IS3) case, the SILENT odd-length case, named-vs-unnamed equivalence, the documented `table="nodes"` limitation, and the NEGATIVE side: a bare `rows()` after a named middle must STILL get the bindings table, without which "never rewrite" would pass and break every Cypher multi-alias RETURN. Mutation-checked PER SURFACE: removing the pandas guard fails 3 pandas cases with polars green; removing the polars guard fails 3 polars cases with pandas green. Verified in-container on dgx-spark: `graphistry/tests/compute` with `--gpus all` gives 7030 passed against master 3f2128e's 7020 (+10, the new file) and an IDENTICAL 9-failure set (pre-existing dask/cuDF coercion). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB

…failure The first version of this file's engine probe wrapped a smoke query in a bare `except Exception: return False`. Two things went wrong with that, both found by trying rather than by argument (on the sibling #1788/#1790 parametrization): * a probe that runs THE SHAPE UNDER TEST disarms its own file — reverting a production guard made the probe raise and every parameter reported SKIPPED instead of failing; * a probe that swallows EVERYTHING is worse — a transient `MemoryError: ... cudaErrorMemoryAllocation` in a fresh GPU container silently dropped cuDF from a run that otherwise passed, and a skipped GPU parameter reads as evidence of passing. So the check now CLASSIFIES: a missing module skips, a recognisable GPU-stack error skips WITH ITS TEXT QUOTED in the reason, and any other failure propagates. The smoke query stays a plain traversal, never the shape under test. GPU receipts — dgx GB10, `graphistry/test-rapids-official:26.02-gfql-polars`, `docker run --gpus all`, cuDF 26.02.01 / polars 1.35.2: 43 passed, 0 skipped (this file plus the #1793 suite), repeated. The marker list is duplicated from the copy landing in `polars_test_utils.py` alongside the #1788/#1790 parametrization; collapse to one definition once both land. Kept duplicated for now so the two PRs stay independently mergeable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB

… that exists (CB3 a+b) Both suites tested the semantics of WHICH TABLE a query returns — engine-agnostic semantics — on pandas + polars only, and neither file was in `bin/test-polars.sh`. `test_rows_table_named_middle.py` also carried a module-level `pytest.importorskip("polars")`, and the `test-gfql-core` lane installs no polars, so that file ran in NO CI LANE AT ALL. `test_rewrite_param_discard.py` named cuDF but skipped polars-gpu outright. Now: fixed engine list (pandas / cuDF / polars / polars-gpu), classified availability check, both files in the polars lane, module-level importorskip dropped (5 pandas cases become live in test-gfql-core). TWO PROBE TRAPS, BOTH HIT BY TRYING RATHER THAN REASONED ABOUT: * A probe that runs THE SHAPE UNDER TEST disarms its own file. Reverting the #1788 `table` guard made the probe raise, and all 20 parameters reported SKIPPED instead of failing. The smoke query is now a plain traversal with no `rows()` call in it. * A probe that SWALLOWS every exception is worse than none. A transient `MemoryError: ... cudaErrorMemoryAllocation` in a fresh GPU container silently dropped cuDF from a run that otherwise passed — and a skipped GPU parameter reads as evidence of passing. `engine_skip_reason` now CLASSIFIES: a missing module skips, a recognisable GPU-stack error skips with its text quoted, and ANY other failure propagates. It is deliberately not `available_nonpandas_engines()`, which builds its list by importability, so a missing engine vanishes from the report rather than showing as SKIPPED. GPU RECEIPTS — dgx GB10, `graphistry/test-rapids-official:26.02-gfql-polars`, `docker run --gpus all`, cuDF 26.02.01 / polars 1.35.2: 45 passed, 3 xfailed, 0 skipped (repeated; all four engines running) MUTATION-CHECKED on the same box by reverting the #1788 and #1790 `table` guards: 24 failed / 0 skipped, spread EVENLY over the four engines (6 each) so the added parameters are not decorative. TWO REAL GAPS SURFACED BY THE NEW PARAMETERS, pinned strict-xfail, not papered over: * #1803 — the indexed bindings bypass gate admits only (PANDAS, CUDF, POLARS), while the native polars chain hands it `Engine.POLARS_GPU` whenever the GPU target is active. So polars-gpu ALWAYS reports `served: False, reason: 'unsupported_engine'` and silently runs the canonical scan. Values are unaffected; the optimization is lost with no signal. * #1804 — the native polars bindings builder never receives `alias_prefilters` (5 rows on pandas/cuDF vs 12 on polars). Already xfailed before this change, but with no issue number, so there was nothing to close and nothing to find. RUNTIME DELTA: zero. No production line changed — the diff is two test files, a shared test helper, `bin/test-polars.sh` and the CHANGELOG — so no pyg-bench lane run is required (CB5), and that claim is checkable from the diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB

…failure The first version of this file's engine probe wrapped a smoke query in a bare `except Exception: return False`. Two things went wrong with that, both found by trying rather than by argument (on the sibling #1788/#1790 parametrization): * a probe that runs THE SHAPE UNDER TEST disarms its own file — reverting a production guard made the probe raise and every parameter reported SKIPPED instead of failing; * a probe that swallows EVERYTHING is worse — a transient `MemoryError: ... cudaErrorMemoryAllocation` in a fresh GPU container silently dropped cuDF from a run that otherwise passed, and a skipped GPU parameter reads as evidence of passing. So the check now CLASSIFIES: a missing module skips, a recognisable GPU-stack error skips WITH ITS TEXT QUOTED in the reason, and any other failure propagates. The smoke query stays a plain traversal, never the shape under test. GPU receipts — dgx GB10, `graphistry/test-rapids-official:26.02-gfql-polars`, `docker run --gpus all`, cuDF 26.02.01 / polars 1.35.2: 43 passed, 0 skipped (this file plus the #1793 suite), repeated. The marker list is duplicated from the copy landing in `polars_test_utils.py` alongside the #1788/#1790 parametrization; collapse to one definition once both land. Kept duplicated for now so the two PRs stay independently mergeable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB

* test(gfql): decide #1793's clear-vs-restore question, and pin it (#1793 review) Review asked whether `clear_row_exec_context` should RESTORE the value the graph carried on entry rather than NULLing it — a fair question, because `attach_row_exec_context` INHERITS on the way in (a `None` argument keeps what `g` already carries), so an outer scope's value can reach an inner execution and be dropped by it. Determination: NULL is correct and restore would reintroduce #1786. Three reasons, each now carried by a test rather than by argument: 1. `clear` is PURE. It returns `g.bind()` and never writes through to the object it was handed, so an outer scope that set the field still has it afterwards. There is no caller state to save. That is exactly what separates this from #1786, which WAS an in-place write onto the caller's own graph — restore is the fix for a mutation, and there is no mutation. 2. The only channel restore would change is the RETURN VALUE, and putting the seed back there IS the second half of #1786 ("the result of a WITH query carries the seed, and a follow-up query on that result is answered against it"). Measured: hand-restoring the seed onto a result changes the answer of the next query on it (7 -> 2 -> 1 rows). 3. No execution frame inherits a context it did not set. Instrumenting `attach` over `graphistry/tests/compute` recorded 3907 calls and ZERO inheriting ones. 53 DID enter on a graph already carrying a seed (nested boundary frames) but each was handed the IDENTICAL `start_nodes` parameter, so the frame still owns what it sets. The cross-segment WITH seed travels as the explicit `start_nodes` PARAMETER (`chain_impl(..., start_nodes=)`, `_compiled_query_reentry_state`), never through the graph field, so the field's lifetime is exactly one boundary-call run. `test_exec_context_scoping.py` re-runs that ownership measurement as an assertion over a corpus of the shapes that reach every attach site, so a future path that starts relying on inheritance reopens this decision loudly instead of silently losing an outer value. MUTATION-CHECKED IN THE DECIDING DIRECTION: implementing save/restore at all three attach sites (chain, native polars chain, gfql_unified) fails 4 of the new tests on every runnable engine — while the existing #1793 suite (`test_reentry_caller_graph_immutability.py`) passes UNCHANGED. Those tests could not distinguish the two designs; this file can, which is why it exists. Engine-parametrized over pandas/polars/cuDF/polars-gpu with a fixed engine list plus a runtime probe (not `available_nonpandas_engines()`, which silently shrinks): a non-runnable engine reports SKIPPED, it does not vanish from the report. Also added to `bin/test-polars.sh` — the file has no module-level `importorskip`, so nothing would otherwise have flagged that its polars params run in no lane (#1795 class). RUNTIME DELTA: zero. No production line changed; the only non-test edits are the `clear_row_exec_context` docstring and a CHANGELOG entry, so no pyg-bench lane run is required (CB5). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB * test(gfql): classify engine availability instead of swallowing every failure The first version of this file's engine probe wrapped a smoke query in a bare `except Exception: return False`. Two things went wrong with that, both found by trying rather than by argument (on the sibling #1788/#1790 parametrization): * a probe that runs THE SHAPE UNDER TEST disarms its own file — reverting a production guard made the probe raise and every parameter reported SKIPPED instead of failing; * a probe that swallows EVERYTHING is worse — a transient `MemoryError: ... cudaErrorMemoryAllocation` in a fresh GPU container silently dropped cuDF from a run that otherwise passed, and a skipped GPU parameter reads as evidence of passing. So the check now CLASSIFIES: a missing module skips, a recognisable GPU-stack error skips WITH ITS TEXT QUOTED in the reason, and any other failure propagates. The smoke query stays a plain traversal, never the shape under test. GPU receipts — dgx GB10, `graphistry/test-rapids-official:26.02-gfql-polars`, `docker run --gpus all`, cuDF 26.02.01 / polars 1.35.2: 43 passed, 0 skipped (this file plus the #1793 suite), repeated. The marker list is duplicated from the copy landing in `polars_test_utils.py` alongside the #1788/#1790 parametrization; collapse to one definition once both land. Kept duplicated for now so the two PRs stay independently mergeable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

…-1790 test(gfql): run the #1788/#1790 suites on all four engines, in a lane that exists

lmeyerov merged commit f89e637 into master Jul 27, 2026
144 of 151 checks passed

This was referenced Jul 27, 2026

fix(gfql): the indexed bypass and the named-middle rewrite must not discard rows() params #1790

Merged

test(gfql): run the #1788/#1790 suites on all four engines, in a lane that exists #1807

Merged

lmeyerov mentioned this pull request Jul 28, 2026

docs(changelog): the two entries missing from #1792 and #1793 #1809

Open

lmeyerov added a commit that referenced this pull request Jul 28, 2026

Merge pull request #1807 from graphistry/audit/gpu-engine-params-1788…

d0515d3

…-1790 test(gfql): run the #1788/#1790 suites on all four engines, in a lane that exists

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(gfql): rows(table=...) must survive a named middle on both chain surfaces - #1788

fix(gfql): rows(table=...) must survive a named middle on both chain surfaces#1788
lmeyerov merged 1 commit into
masterfrom
fix/gfql-rows-table-named-middle

lmeyerov commented Jul 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

lmeyerov commented Jul 27, 2026

The bug

How it was found

The guard keys on a non-default table, not on is None

Tests

Verification

Scope note

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

The guard keys on a non-default table, not on `is None`