From 50749cb54bb078bdb79517533bef47ebc935c83c Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Mon, 27 Jul 2026 04:01:44 -0700 Subject: [PATCH] fix(gfql): rows(table=...) must survive a named middle on both chain surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The named-middle rewrite turns `[...named ops..., rows()]` into `rows(binding_ops=)` 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 3f2128ea's 7020 (+10, the new file) and an IDENTICAL 9-failure set (pre-existing dask/cuDF coercion). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB --- CHANGELOG.md | 1 + graphistry/compute/chain.py | 11 ++ .../compute/gfql/lazy/engine/polars/chain.py | 6 + .../gfql/test_rows_table_named_middle.py | 148 ++++++++++++++++++ 4 files changed, 166 insertions(+) create mode 100644 graphistry/tests/compute/gfql/test_rows_table_named_middle.py diff --git a/CHANGELOG.md b/CHANGELOG.md index e540e01ea7..4733d43dc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - **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. ### Fixed +- **`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. - **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. - **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. diff --git a/graphistry/compute/chain.py b/graphistry/compute/chain.py index b62bd23918..7ad44e8aa8 100644 --- a/graphistry/compute/chain.py +++ b/graphistry/compute/chain.py @@ -734,6 +734,17 @@ def _handle_boundary_calls( and suffix[0].params.get("binding_ops") is None and suffix[0].params.get("source") is None and suffix[0].params.get("alias_endpoints") is None + # A NON-DEFAULT `table` names the table the caller wants. Rewriting that into a + # bindings table silently answers a different question — and on an even-length + # middle (a path ending on an EDGE, e.g. LDBC IS3's edge lookup) the rewritten + # op list is not an alternating node/edge path at all, so it hard-errors. Same + # reason `source` and `alias_endpoints` are excluded above. + # Tested against `"nodes"` rather than None because `rows()` DEFAULTS table to + # "nodes" and always emits it, so `is None` is never true and would disable the + # rewrite outright (measured: it breaks the IS6 bindings path). The cost is that + # an EXPLICIT `rows(table="nodes")` is indistinguishable from a bare `rows()` at + # the params level, so it still rewrites; only a non-default table opts out. + and suffix[0].params.get("table", "nodes") == "nodes" and all(isinstance(op, (ASTNode, ASTEdge)) for op in middle) ): suffix = [rows_fn(binding_ops=serialize_binding_ops(middle))] + list(suffix[1:]) diff --git a/graphistry/compute/gfql/lazy/engine/polars/chain.py b/graphistry/compute/gfql/lazy/engine/polars/chain.py index 568c44058a..528a909051 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/chain.py +++ b/graphistry/compute/gfql/lazy/engine/polars/chain.py @@ -494,6 +494,12 @@ def _run_calls_polars(g_cur, calls, start_nodes, base_graph, middle): and calls[0].params.get("binding_ops") is None and calls[0].params.get("source") is None and calls[0].params.get("alias_endpoints") is None + # See the twin guard in compute/chain.py: a NON-DEFAULT `table` names the table the + # caller wants, so the bindings rewrite must not override it. Both surfaces need + # the check — this one is the native polars chain, that one the generic chain. + # `== "nodes"`, not `is None`: `rows()` defaults table to "nodes" and always emits + # it, so an `is None` test disables the rewrite entirely. + and calls[0].params.get("table", "nodes") == "nodes" and all(isinstance(op, (_ASTNode, _ASTEdge)) for op in middle) ): calls = [rows_fn(binding_ops=serialize_binding_ops(middle))] + list(calls[1:]) diff --git a/graphistry/tests/compute/gfql/test_rows_table_named_middle.py b/graphistry/tests/compute/gfql/test_rows_table_named_middle.py new file mode 100644 index 0000000000..75fbcfefa6 --- /dev/null +++ b/graphistry/tests/compute/gfql/test_rows_table_named_middle.py @@ -0,0 +1,148 @@ +"""`rows(table=...)` must survive a NAMED middle — on both chain surfaces. + +The named-middle rewrite turns `[... named ops ..., rows()]` into +`rows(binding_ops=)` so a Cypher multi-alias RETURN lowers to a bindings table. +It skipped that rewrite when the call already carried `binding_ops`, `source` or +`alias_endpoints` — but NOT when it carried `table`, which names the output table just as +explicitly. Two distinct failures followed, and the quiet one is the worse: + + * ODD-length named middle -> the rewrite fired and the caller got the BINDINGS table + instead of the edges table. No error; `table=` was simply ignored. + * EVEN-length named middle (a path ending on an EDGE — LDBC IS3's edge lookup is exactly + `(person)-[r:KNOWS]-`) -> the rewritten op list is not an alternating node/edge path, so + validation hard-errored with "require ... a single connected alternating node/edge path". + +Pinned on both engines because the rewrite is duplicated in `compute/chain.py` (generic) and +`gfql/lazy/engine/polars/chain.py` (native polars); fixing one alone leaves the other wrong. +""" +import pandas as pd +import pytest + +import graphistry +from graphistry.compute import ast + +pl = pytest.importorskip("polars") + + +NODES = pd.DataFrame({"id": [0, 1, 2, 3], "firstName": ["a", "b", "c", "d"]}) +EDGES = pd.DataFrame({"s": [0, 0, 1], "d": [1, 2, 3], + "type": ["KNOWS"] * 3, "creationDate": [30, 10, 20]}) + +EDGE_COLS = {"s", "d", "type", "creationDate"} + + +def _graph(engine): + if engine == "polars": + return graphistry.nodes(pl.from_pandas(NODES), "id").edges( + pl.from_pandas(EDGES), "s", "d") + return graphistry.nodes(NODES, "id").edges(EDGES, "s", "d") + + +def _cols(frame): + return set(frame.columns) + + +@pytest.mark.parametrize("engine", ["pandas", "polars"]) +def test_rows_table_edges_after_named_middle_ending_on_an_edge(engine): + """LDBC IS3's edge lookup: `(n {id})-[r:KNOWS]-` then `rows(table='edges')`. + + The middle is [node, edge] — EVEN length, so the rewrite produced a non-alternating + binding_ops list and the query raised instead of returning the edges. + """ + g = _graph(engine) + out = g.gfql([ + ast.n({"id": 0}, name="n"), + ast.e_undirected({"type": "KNOWS"}, name="r"), + ast.rows(table="edges"), + ], engine=engine)._nodes + assert EDGE_COLS <= _cols(out), \ + f"[{engine}] expected the EDGES table, got columns {sorted(_cols(out))}" + assert len(out) == 2, f"[{engine}] expected node 0's two KNOWS edges, got {len(out)}" + + +@pytest.mark.parametrize("engine", ["pandas", "polars"]) +def test_rows_table_edges_after_named_middle_of_odd_length(engine): + """The SILENT half: an odd-length named middle returned the bindings table instead. + + This is the case that produced no error at all — `table='edges'` was ignored and the + caller got alias columns (`n`, `r.type`, `f`, ...). Asserting the edge columns are + present AND the alias columns are absent is what distinguishes the two tables. + """ + g = _graph(engine) + out = g.gfql([ + ast.n({"id": 0}, name="n"), + ast.e_undirected({"type": "KNOWS"}, name="r"), + ast.n(name="f"), + ast.rows(table="edges"), + ], engine=engine)._nodes + cols = _cols(out) + assert EDGE_COLS <= cols, \ + f"[{engine}] expected the EDGES table, got columns {sorted(cols)}" + assert not any(c.startswith("r.") for c in cols), \ + f"[{engine}] bindings columns leaked into a rows(table='edges') result: {sorted(cols)}" + + +@pytest.mark.parametrize("engine", ["pandas", "polars"]) +def test_rows_table_edges_is_unaffected_by_whether_the_middle_is_named(engine): + """Naming an op is a projection concern; it must not change WHICH table comes back.""" + g = _graph(engine) + named = g.gfql([ + ast.n({"id": 0}, name="n"), + ast.e_undirected({"type": "KNOWS"}, name="r"), + ast.rows(table="edges"), + ], engine=engine)._nodes + unnamed = g.gfql([ + ast.n({"id": 0}), + ast.e_undirected({"type": "KNOWS"}), + ast.rows(table="edges"), + ], engine=engine)._nodes + assert EDGE_COLS <= _cols(unnamed) + assert EDGE_COLS <= _cols(named) + assert len(named) == len(unnamed), \ + f"[{engine}] naming the middle changed the row count: {len(named)} vs {len(unnamed)}" + + +@pytest.mark.parametrize("engine", ["pandas", "polars"]) +def test_bare_rows_after_a_named_middle_still_gets_the_bindings_table(engine): + """The NEGATIVE side of the boundary: the rewrite must still fire without `table=`. + + Without this, the fix could be "never rewrite", which would silently break every + Cypher multi-alias RETURN — the thing the rewrite exists to serve. + """ + g = _graph(engine) + out = g.gfql([ + ast.n({"id": 0}, name="n"), + ast.e_undirected({"type": "KNOWS"}, name="r"), + ast.n(name="f"), + ast.rows(), + ], engine=engine)._nodes + cols = _cols(out) + assert any(c == "n" or c.startswith("n.") for c in cols), \ + f"[{engine}] expected a bindings table with alias columns, got {sorted(cols)}" + + +@pytest.mark.parametrize("engine", ["pandas", "polars"]) +def test_explicit_table_nodes_still_rewrites_and_that_is_a_known_limitation(engine): + """`rows(table='nodes')` DOES still rewrite, and that is deliberate — pin the limitation. + + `rows()` declares `table: str = "nodes"` and always emits it, so an explicit + `rows(table='nodes')` and a bare `rows()` are byte-identical at the params level. The + guard therefore cannot honour the explicit spelling without disabling the rewrite for + every bare `rows()` — which is exactly what a first attempt at this fix did, breaking + the IS6 bindings path (5 regressions, caught only by re-baselining against master). + + So the contract is: a NON-DEFAULT table opts out of the rewrite; `"nodes"` cannot. + Distinguishing them would need `rows()` to default `table=None` and resolve later — a + wire-format change, out of scope here. This test exists so that limitation is pinned + rather than discovered again. + """ + g = _graph(engine) + out = g.gfql([ + ast.n({"id": 0}, name="n"), + ast.e_undirected({"type": "KNOWS"}, name="r"), + ast.n(name="f"), + ast.rows(table="nodes"), + ], engine=engine)._nodes + cols = _cols(out) + assert any(c == "n" or c.startswith("n.") for c in cols), \ + f"[{engine}] expected the bindings table (documented limitation), got {sorted(cols)}"