Skip to content

Commit 50749cb

Browse files
lmeyerovclaude
andcommitted
fix(gfql): rows(table=...) must survive a named middle on both chain 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
1 parent 3f2128e commit 50749cb

4 files changed

Lines changed: 166 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
2727
- **GFQL execution context is declared rather than attached at runtime**: the private `_gfql_*` per-execution fields (index policy and registry, row-pipeline base graph, carried seed nodes, edge aliases, shortest-path backend, and the indexed-bindings handoff) are now declared on `Plottable` with defaults on `PlotterBase`, instead of being set with `setattr` and read back with `getattr(..., default)`. No public API or behaviour change; internal call sites are typed, and hand-rolled `Plottable` stand-ins must now construct the full context.
2828

2929
### Fixed
30+
- **`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.
3031
- **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.
3132

3233
- **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: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -734,6 +734,17 @@ def _handle_boundary_calls(
734734
and suffix[0].params.get("binding_ops") is None
735735
and suffix[0].params.get("source") is None
736736
and suffix[0].params.get("alias_endpoints") is None
737+
# A NON-DEFAULT `table` names the table the caller wants. Rewriting that into a
738+
# bindings table silently answers a different question — and on an even-length
739+
# middle (a path ending on an EDGE, e.g. LDBC IS3's edge lookup) the rewritten
740+
# op list is not an alternating node/edge path at all, so it hard-errors. Same
741+
# reason `source` and `alias_endpoints` are excluded above.
742+
# Tested against `"nodes"` rather than None because `rows()` DEFAULTS table to
743+
# "nodes" and always emits it, so `is None` is never true and would disable the
744+
# rewrite outright (measured: it breaks the IS6 bindings path). The cost is that
745+
# an EXPLICIT `rows(table="nodes")` is indistinguishable from a bare `rows()` at
746+
# the params level, so it still rewrites; only a non-default table opts out.
747+
and suffix[0].params.get("table", "nodes") == "nodes"
737748
and all(isinstance(op, (ASTNode, ASTEdge)) for op in middle)
738749
):
739750
suffix = [rows_fn(binding_ops=serialize_binding_ops(middle))] + list(suffix[1:])

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,12 @@ def _run_calls_polars(g_cur, calls, start_nodes, base_graph, middle):
494494
and calls[0].params.get("binding_ops") is None
495495
and calls[0].params.get("source") is None
496496
and calls[0].params.get("alias_endpoints") is None
497+
# See the twin guard in compute/chain.py: a NON-DEFAULT `table` names the table the
498+
# caller wants, so the bindings rewrite must not override it. Both surfaces need
499+
# the check — this one is the native polars chain, that one the generic chain.
500+
# `== "nodes"`, not `is None`: `rows()` defaults table to "nodes" and always emits
501+
# it, so an `is None` test disables the rewrite entirely.
502+
and calls[0].params.get("table", "nodes") == "nodes"
497503
and all(isinstance(op, (_ASTNode, _ASTEdge)) for op in middle)
498504
):
499505
calls = [rows_fn(binding_ops=serialize_binding_ops(middle))] + list(calls[1:])
Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
"""`rows(table=...)` must survive a NAMED middle — on both chain surfaces.
2+
3+
The named-middle rewrite turns `[... named ops ..., rows()]` into
4+
`rows(binding_ops=<middle>)` so a Cypher multi-alias RETURN lowers to a bindings table.
5+
It skipped that rewrite when the call already carried `binding_ops`, `source` or
6+
`alias_endpoints` — but NOT when it carried `table`, which names the output table just as
7+
explicitly. Two distinct failures followed, and the quiet one is the worse:
8+
9+
* ODD-length named middle -> the rewrite fired and the caller got the BINDINGS table
10+
instead of the edges table. No error; `table=` was simply ignored.
11+
* EVEN-length named middle (a path ending on an EDGE — LDBC IS3's edge lookup is exactly
12+
`(person)-[r:KNOWS]-`) -> the rewritten op list is not an alternating node/edge path, so
13+
validation hard-errored with "require ... a single connected alternating node/edge path".
14+
15+
Pinned on both engines because the rewrite is duplicated in `compute/chain.py` (generic) and
16+
`gfql/lazy/engine/polars/chain.py` (native polars); fixing one alone leaves the other wrong.
17+
"""
18+
import pandas as pd
19+
import pytest
20+
21+
import graphistry
22+
from graphistry.compute import ast
23+
24+
pl = pytest.importorskip("polars")
25+
26+
27+
NODES = pd.DataFrame({"id": [0, 1, 2, 3], "firstName": ["a", "b", "c", "d"]})
28+
EDGES = pd.DataFrame({"s": [0, 0, 1], "d": [1, 2, 3],
29+
"type": ["KNOWS"] * 3, "creationDate": [30, 10, 20]})
30+
31+
EDGE_COLS = {"s", "d", "type", "creationDate"}
32+
33+
34+
def _graph(engine):
35+
if engine == "polars":
36+
return graphistry.nodes(pl.from_pandas(NODES), "id").edges(
37+
pl.from_pandas(EDGES), "s", "d")
38+
return graphistry.nodes(NODES, "id").edges(EDGES, "s", "d")
39+
40+
41+
def _cols(frame):
42+
return set(frame.columns)
43+
44+
45+
@pytest.mark.parametrize("engine", ["pandas", "polars"])
46+
def test_rows_table_edges_after_named_middle_ending_on_an_edge(engine):
47+
"""LDBC IS3's edge lookup: `(n {id})-[r:KNOWS]-` then `rows(table='edges')`.
48+
49+
The middle is [node, edge] — EVEN length, so the rewrite produced a non-alternating
50+
binding_ops list and the query raised instead of returning the edges.
51+
"""
52+
g = _graph(engine)
53+
out = g.gfql([
54+
ast.n({"id": 0}, name="n"),
55+
ast.e_undirected({"type": "KNOWS"}, name="r"),
56+
ast.rows(table="edges"),
57+
], engine=engine)._nodes
58+
assert EDGE_COLS <= _cols(out), \
59+
f"[{engine}] expected the EDGES table, got columns {sorted(_cols(out))}"
60+
assert len(out) == 2, f"[{engine}] expected node 0's two KNOWS edges, got {len(out)}"
61+
62+
63+
@pytest.mark.parametrize("engine", ["pandas", "polars"])
64+
def test_rows_table_edges_after_named_middle_of_odd_length(engine):
65+
"""The SILENT half: an odd-length named middle returned the bindings table instead.
66+
67+
This is the case that produced no error at all — `table='edges'` was ignored and the
68+
caller got alias columns (`n`, `r.type`, `f`, ...). Asserting the edge columns are
69+
present AND the alias columns are absent is what distinguishes the two tables.
70+
"""
71+
g = _graph(engine)
72+
out = g.gfql([
73+
ast.n({"id": 0}, name="n"),
74+
ast.e_undirected({"type": "KNOWS"}, name="r"),
75+
ast.n(name="f"),
76+
ast.rows(table="edges"),
77+
], engine=engine)._nodes
78+
cols = _cols(out)
79+
assert EDGE_COLS <= cols, \
80+
f"[{engine}] expected the EDGES table, got columns {sorted(cols)}"
81+
assert not any(c.startswith("r.") for c in cols), \
82+
f"[{engine}] bindings columns leaked into a rows(table='edges') result: {sorted(cols)}"
83+
84+
85+
@pytest.mark.parametrize("engine", ["pandas", "polars"])
86+
def test_rows_table_edges_is_unaffected_by_whether_the_middle_is_named(engine):
87+
"""Naming an op is a projection concern; it must not change WHICH table comes back."""
88+
g = _graph(engine)
89+
named = g.gfql([
90+
ast.n({"id": 0}, name="n"),
91+
ast.e_undirected({"type": "KNOWS"}, name="r"),
92+
ast.rows(table="edges"),
93+
], engine=engine)._nodes
94+
unnamed = g.gfql([
95+
ast.n({"id": 0}),
96+
ast.e_undirected({"type": "KNOWS"}),
97+
ast.rows(table="edges"),
98+
], engine=engine)._nodes
99+
assert EDGE_COLS <= _cols(unnamed)
100+
assert EDGE_COLS <= _cols(named)
101+
assert len(named) == len(unnamed), \
102+
f"[{engine}] naming the middle changed the row count: {len(named)} vs {len(unnamed)}"
103+
104+
105+
@pytest.mark.parametrize("engine", ["pandas", "polars"])
106+
def test_bare_rows_after_a_named_middle_still_gets_the_bindings_table(engine):
107+
"""The NEGATIVE side of the boundary: the rewrite must still fire without `table=`.
108+
109+
Without this, the fix could be "never rewrite", which would silently break every
110+
Cypher multi-alias RETURN — the thing the rewrite exists to serve.
111+
"""
112+
g = _graph(engine)
113+
out = g.gfql([
114+
ast.n({"id": 0}, name="n"),
115+
ast.e_undirected({"type": "KNOWS"}, name="r"),
116+
ast.n(name="f"),
117+
ast.rows(),
118+
], engine=engine)._nodes
119+
cols = _cols(out)
120+
assert any(c == "n" or c.startswith("n.") for c in cols), \
121+
f"[{engine}] expected a bindings table with alias columns, got {sorted(cols)}"
122+
123+
124+
@pytest.mark.parametrize("engine", ["pandas", "polars"])
125+
def test_explicit_table_nodes_still_rewrites_and_that_is_a_known_limitation(engine):
126+
"""`rows(table='nodes')` DOES still rewrite, and that is deliberate — pin the limitation.
127+
128+
`rows()` declares `table: str = "nodes"` and always emits it, so an explicit
129+
`rows(table='nodes')` and a bare `rows()` are byte-identical at the params level. The
130+
guard therefore cannot honour the explicit spelling without disabling the rewrite for
131+
every bare `rows()` — which is exactly what a first attempt at this fix did, breaking
132+
the IS6 bindings path (5 regressions, caught only by re-baselining against master).
133+
134+
So the contract is: a NON-DEFAULT table opts out of the rewrite; `"nodes"` cannot.
135+
Distinguishing them would need `rows()` to default `table=None` and resolve later — a
136+
wire-format change, out of scope here. This test exists so that limitation is pinned
137+
rather than discovered again.
138+
"""
139+
g = _graph(engine)
140+
out = g.gfql([
141+
ast.n({"id": 0}, name="n"),
142+
ast.e_undirected({"type": "KNOWS"}, name="r"),
143+
ast.n(name="f"),
144+
ast.rows(table="nodes"),
145+
], engine=engine)._nodes
146+
cols = _cols(out)
147+
assert any(c == "n" or c.startswith("n.") for c in cols), \
148+
f"[{engine}] expected the bindings table (documented limitation), got {sorted(cols)}"

0 commit comments

Comments
 (0)