Skip to content

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

Merged
lmeyerov merged 1 commit into
masterfrom
fix/gfql-rewrite-fastpath-param-discard
Jul 27, 2026
Merged

fix(gfql): the indexed bypass and the named-middle rewrite must not discard rows() params#1790
lmeyerov merged 1 commit into
masterfrom
fix/gfql-rewrite-fastpath-param-discard

Conversation

@lmeyerov

Copy link
Copy Markdown
Contributor

Stacked on #1788 (base = fix/gfql-rows-table-named-middle). The table half of this
only becomes reachable once #1788 lands; the attach_prop_aliases half is independent
and reproduces on master today.

What I went looking for

The generalized question behind #1788: where else does a rewrite or fast path silently
discard an explicit request the caller made?
Two more instances, both around the same
two functions #1788 touched, neither caught by its cases.

1. The indexed bindings bypass ignored table — wrong values

The bypass exists to skip the canonical traversal, so whatever it hands the suffix is
the pre-traversal graph. That is sound for a bindings table (the indexed path bag already
is the answer) but not for rows(table='edges'), which then reads the full edge frame.

scan path indexed path (index_policy='force')
(a {id:1})-[r]->(b) + rows(table='edges') 3 rows 12 rows — the whole graph
same, undirected 4 rows 12 rows
… + select(['type']) [A,A,A] every edge type in the graph

pandas, cuDF and native polars alike. No error. It survives a following projection, so it
is wrong values, not schema noise.

_plan_indexed_middle (generic) and _try_indexed_middle_polars (native polars) declined
for source / alias_endpoints / alias_prefilters but not for a non-default table.
Both now do, keyed on != "nodes" for the same reason #1788's guard is — rows() defaults
table to "nodes" and always emits it, so is None would disable the bypass outright.

Why #1788 didn't see it: before that fix, the rewrite converted the call on the indexed
and the scan path, so the two agreed — on the wrong table. Making the scan path right is
what exposed the indexed one reading the whole graph. This is the same class as the
already-shipped "indexed bypass could return every node for rows() over an unnamed
pattern"
, one param over.

2. Both rewrites threw away the caller's other rows() params

suffix = [rows_fn(binding_ops=...)] + suffix[1:] builds a fresh call rather than
adding binding_ops to the one the caller wrote, so every param the rewrite has no opinion
about is dropped. Observable for attach_prop_aliases (the #1711 projection pushdown):

g.gfql([n({'id':1}, name='a'), e_forward(name='r'), n(name='b'),
        rows(attach_prop_aliases=['a'])])          # -> attaches b.id, b.kind too
g.gfql([rows(binding_ops=<those three ops>, attach_prop_aliases=['a'])])   # -> does not

Same query, same bindings, different output schema — decided only by how it was spelled.
alias_prefilters was dropped the same way. Both are now carried through, on both surfaces.
Cypher is unaffected either way: gfql_unified always emits attach_prop_aliases with
binding_ops, so the rewrite never fired on those plans.

3. Found but NOT fixed — pinned by a strict xfail

The native polars row op calls its bindings builder with binding_ops and
attach_prop_aliases only, so alias_prefilters never reaches it, while the generic path
threads it into _gfql_binding_ops_row_table. Measured on the same fixture:
pandas/cuDF 5 rows, polars 12 for an identical query.

Not fixed here because the hint is documented as advisory — the Cypher lowering that
emits it always keeps the equivalent post-join filter, so Cypher answers agree across
engines and only the pushdown is lost; honouring it natively means porting the whole
prefilter evaluator, which is a feature, not this fix. Hand-written GFQL passing it without
a post-filter does get engine-dependent row counts. xfail(strict=True) so whoever
ports it is told to delete the marker rather than leaving a stale "known gap".

Tests

21 cases across pandas / cuDF / native polars: the leak on both middle directions, the leak
surviving a select, the param carry-through by schema and by value, and the negative
side — a bare rows() over a named middle must still engage the bypass, asserted on the
index trace rather than on the answer, because the answer is identical either way and that
is exactly what would make "just never take the bypass" pass silently.

Mutation-checked per surface, all four guards independently:

mutation fails other engines
drop generic table guard 6 (pandas + cuDF) polars green
drop polars table guard 3 (polars) pandas + cuDF green
revert generic param carry 2 (pandas + cuDF) polars green
revert polars param carry 1 (polars) pandas + cuDF green

Verification

All in-container on dgx-spark. graphistry/tests/compute with --gpus all:

  • master 3f2128ea: 9 failed, 7020 passed, 90 skipped, 15 xfailed
  • this branch: 9 failed, 7050 passed, 90 skipped, 16 xfailed

Identical 9-failure set (pre-existing igraph/cuDF round-trip + dask coercion). +30 = the
10 cases #1788 adds plus my 20; +1 xfail is the polars gap above. ruff clean; mypy 173
errors before and after, in the same 12 files — neither changed file appears.

🤖 Generated with Claude Code

https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB

@lmeyerov
lmeyerov changed the base branch from fix/gfql-rows-table-named-middle to master July 27, 2026 17:19
@lmeyerov
lmeyerov force-pushed the fix/gfql-rewrite-fastpath-param-discard branch from 69c84ab to 23e9fce Compare July 27, 2026 17:21
…discarding rows() params

Amplification of the rows(table=...) named-middle fix: the same CLASS of defect --
an interception that answers a different question than the one asked -- appears twice
more around the same two functions.

1. The indexed bindings bypass ignored `table`. Serving it SKIPS the canonical
   traversal, so the suffix runs against the pre-traversal graph; that is sound for a
   bindings table but not for rows(table='edges'), which then read the FULL edge frame
   (12 rows instead of 3 on the fixture, pandas/cuDF/native polars, and the wrong
   values survive a following select). Both gates now decline a non-default table.
   Only reachable once the rewrite stopped firing for a non-default table -- before
   that, the rewrite converted the call on the indexed AND the scan path, so the two
   agreed on the wrong table.

2. Both rewrites built a FRESH rows(binding_ops=...) rather than adding binding_ops to
   the call the caller wrote, throwing away every other param. attach_prop_aliases (the
   projection pushdown) and alias_prefilters are now carried through, so naming a middle
   no longer widens the output schema relative to the hand-spelled binding_ops form.

Pinned on pandas, cuDF and native polars; mutation-checked per surface (removing the
generic table guard fails 6 with polars green, the polars one fails 3 with pandas green;
removing the generic param carry fails 2, the polars one 1). A strict-xfail records the
one instance NOT fixed here: the native polars bindings builder never receives
alias_prefilters at all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
@lmeyerov
lmeyerov force-pushed the fix/gfql-rewrite-fastpath-param-discard branch from 23e9fce to 645c60d Compare July 27, 2026 17:35
@lmeyerov
lmeyerov merged commit 4415116 into master Jul 27, 2026
77 checks passed
lmeyerov added a commit that referenced this pull request Jul 28, 2026
…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
lmeyerov added a commit that referenced this pull request Jul 28, 2026
… 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
lmeyerov added a commit that referenced this pull request Jul 28, 2026
…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
lmeyerov added a commit that referenced this pull request Jul 28, 2026
* 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>
lmeyerov added a commit that referenced this pull request Jul 28, 2026
…-1790

test(gfql): run the #1788/#1790 suites on all four engines, in a lane that exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant