Skip to content

Commit 645c60d

Browse files
lmeyerovclaude
andcommitted
fix(gfql): stop the indexed bypass and the named-middle rewrite from 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
1 parent d9fa0a9 commit 645c60d

4 files changed

Lines changed: 302 additions & 2 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
2929

3030
### Fixed
3131
- **`rows(table='edges')` silently returned the wrong table after a NAMED traversal**: the named-middle rewrite — which turns `[...named ops..., rows()]` into `rows(binding_ops=...)` so a Cypher multi-alias `RETURN` lowers to a bindings table — skipped itself when the call already carried `binding_ops`, `source` or `alias_endpoints`, but not when it carried a non-default `table`. So naming any op in the middle changed which table came back: an odd-length middle silently produced the BINDINGS table instead of the requested edges (no error, `table=` simply ignored), and an even-length middle — a path ending on an edge, e.g. `(person)-[r:KNOWS]-` — produced a non-alternating op list and raised "require ... a single connected alternating node/edge path". An unnamed middle was unaffected, which is what made it look shape-specific rather than naming-specific. Both chain surfaces are fixed (the generic chain and the native polars chain carry the rewrite independently, so fixing one left the other wrong). Note the guard keys on a NON-DEFAULT table: `rows()` declares `table='nodes'` and always emits it, so an explicit `rows(table='nodes')` is indistinguishable from a bare `rows()` and still rewrites — pinned as a known limitation rather than left to be rediscovered.
32+
- **Indexed bindings bypass returned the WHOLE edge table for `rows(table='edges')`**: 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 the gate declined only for `source` / `alias_endpoints` / `alias_prefilters`, not for a non-default `table`, so a `rows(table='edges')` after a named middle read the full edge frame instead of the traversal-narrowed one (measured on a 12-edge fixture: 12 rows instead of 3, on pandas, cuDF and native polars, and the wrong values survive a following `select`). Both gates now decline a non-default `table` (`chain._plan_indexed_middle` and the native polars `_try_indexed_middle_polars`), matching the named-middle rewrite's guard. This became reachable only once that rewrite stopped firing for a non-default `table` — before it, the rewrite converted the call on BOTH the indexed and the scan path, so the two agreed on the wrong table. Same class as the earlier "indexed bypass could return every node for `rows()` over an unnamed pattern".
33+
- **The named-middle rewrite discarded the caller's other `rows()` params**: both rewrites built a FRESH `rows(binding_ops=...)` instead of adding `binding_ops` to the call the caller wrote, so every param the rewrite has no opinion about was thrown away. For `attach_prop_aliases` — the projection pushdown — that is observable: spelling a pattern as a NAMED middle rather than as explicit `binding_ops` silently attached every alias's properties instead of the requested ones, i.e. the same query returned a wider schema depending only on how it was written. `alias_prefilters` was dropped the same way. Both are now carried through, on both chain surfaces. Known remaining gap, pinned by a strict-xfail test rather than left to be rediscovered: the native polars bindings builder never receives `alias_prefilters` at all, so hand-written GFQL passing that hint without an equivalent post-filter still gets engine-dependent row counts (Cypher-generated plans always keep the post-filter, so they agree across engines and only lose the pushdown).
3234
- **Native polars variable-length bindings dropped zero-hop rows (`-[*0..k]->`)**: the polars `rows(binding_ops=...)` builder rebuilt the bindings table from the CHAIN OUTPUT, while the pandas oracle rebuilds from the pre-chain base graph. The traversal prunes to what IT considers matched, which is not the same set the bindings builder matches — a zero-hop variable-length segment binds a seed that has no matching outgoing edge, and the traversal drops that node entirely. So `MATCH (a)-[*0..2]->(b)` could silently return fewer rows than pandas (no error, just missing rows). The polars builder now sources its node/edge tables from the same pre-chain base graph pandas uses (which is also what the indexed builder was already handed). Found by differential fuzzing while adding unbounded support (#1709); pinned by a zero-hop regression test and by `-[*0..2]->` parity cases.
3335

3436
- **Indexed bypass could return every node for `rows()` over an unnamed pattern**: the boundary accepted a `rows()` call carrying no binding ops over a middle with no aliases. That call reads the traversal-narrowed node table, but the bypass hands it the full graph, so the query would have returned all nodes. The gate now requires that the rows call actually consumes the whole middle as binding ops — either it already carries them, or the named-middle rewrite installs exactly them.

graphistry/compute/chain.py

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,15 @@ def _plan_indexed_middle(
586586
call.params.get("source") is not None
587587
or call.params.get("alias_endpoints") is not None
588588
or call.params.get("alias_prefilters")
589+
# Serving the bypass SKIPS the canonical traversal, so the suffix runs against the
590+
# PRE-traversal graph. That is sound for a bindings table (the path bag already is
591+
# the answer) but not for `rows(table="edges")`, which would read the whole edge
592+
# table instead of the traversal-narrowed one. Non-default `table` therefore keeps
593+
# the scan path, for the same reason `source`/`alias_endpoints` do above. Compared
594+
# against "nodes", not None: `rows()` defaults `table` to "nodes" and always emits
595+
# it, so an `is None` test would disable the bypass outright — the same trap the
596+
# named-middle rewrite's guard documents below.
597+
or call.params.get("table", "nodes") != "nodes"
589598
or not all(isinstance(op, (ASTNode, ASTEdge)) for op in middle)
590599
):
591600
return None
@@ -747,7 +756,19 @@ def _handle_boundary_calls(
747756
and suffix[0].params.get("table", "nodes") == "nodes"
748757
and all(isinstance(op, (ASTNode, ASTEdge)) for op in middle)
749758
):
750-
suffix = [rows_fn(binding_ops=serialize_binding_ops(middle))] + list(suffix[1:])
759+
# ADD binding_ops to the call the caller wrote; do not build a fresh one. The
760+
# rewrite's job is to say WHERE the bindings come from, and every other param
761+
# on `rows()` is still the caller's — `attach_prop_aliases` (the #1711
762+
# projection pushdown) and the advisory `alias_prefilters` both survive into
763+
# the binding_ops builder, which is the only place they are read. Rebuilding
764+
# threw them away, so merely naming the middle silently attached every alias's
765+
# properties. The excluded params above cannot be present here.
766+
prev_params = suffix[0].params
767+
suffix = [rows_fn(
768+
binding_ops=serialize_binding_ops(middle),
769+
alias_prefilters=prev_params.get("alias_prefilters"),
770+
attach_prop_aliases=prev_params.get("attach_prop_aliases"),
771+
)] + list(suffix[1:])
751772
g_temp = _chain_impl(
752773
g_temp,
753774
suffix,

graphistry/compute/gfql/lazy/engine/polars/chain.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,15 @@ def _run_calls_polars(g_cur, calls, start_nodes, base_graph, middle):
528528
and calls[0].params.get("table", "nodes") == "nodes"
529529
and all(isinstance(op, (_ASTNode, _ASTEdge)) for op in middle)
530530
):
531-
calls = [rows_fn(binding_ops=serialize_binding_ops(middle))] + list(calls[1:])
531+
# See the twin in compute/chain.py: ADD binding_ops to the caller's call rather
532+
# than building a fresh one, so the params the rewrite has no opinion about
533+
# (`attach_prop_aliases`, `alias_prefilters`) reach the binding_ops builder.
534+
prev_params = calls[0].params
535+
calls = [rows_fn(
536+
binding_ops=serialize_binding_ops(middle),
537+
alias_prefilters=prev_params.get("alias_prefilters"),
538+
attach_prop_aliases=prev_params.get("attach_prop_aliases"),
539+
)] + list(calls[1:])
532540

533541
# Per-op NATIVE-OR-DEFER. Ops that don't lower:
534542
# - non-native ROW op (correlated-subquery semi_apply/anti_semi_apply/join_apply):
@@ -801,6 +809,11 @@ def _try_indexed_middle_polars(
801809
or suffix[0].params.get("source") is not None
802810
or suffix[0].params.get("alias_endpoints") is not None
803811
or suffix[0].params.get("alias_prefilters")
812+
# See the twin guard in chain._plan_indexed_middle: serving the bypass skips the
813+
# canonical traversal, so a non-default `table` would read the PRE-traversal edge
814+
# table (the whole graph) instead of the narrowed one. "nodes", not None — `rows()`
815+
# always emits `table`.
816+
or suffix[0].params.get("table", "nodes") != "nodes"
804817
or not all(isinstance(op, (ASTNode, ASTEdge)) for op in middle)
805818
):
806819
return None, False

0 commit comments

Comments
 (0)