Skip to content

Commit d08720e

Browse files
lmeyerovclaude
andauthored
fix(gfql): decline three silently-wrong bounded var-length polars shapes (#1787) (#1794)
* fix(gfql): decline three silently-wrong bounded var-length polars shapes (#1787) Pandas is the oracle and the contract is parity-or-NotImplementedError. Three BOUNDED var-length shapes in the native polars `rows(binding_ops=...)` path returned a DIFFERENT count with no error. Same root-cause family as the unbounded case #1781 declined: pandas' step_pairs come from the var-length `edge_op.execute` hop, whose hop-window pruning -- and, when seeded, its per-seed BFS -- changes the edge multiplicity in a way a rebuild from the raw matching edge table does not reproduce. Declined (diverging graphs out of 60 random ones per shape, differential fuzz against the pandas oracle): * directed `-[*k..m]->` with min_hops >= 3 (53/60; pandas 0 vs polars 35), and min_hops >= 2 off a FILTERED seed (27-30/60). Plain min_hops <= 2 is fuzz-clean, including as a non-first segment -- that is the graph-bench q3 `-[*1..k]->` shape, still served. * undirected `-[*1..1]-` / `-[*1]-`, the degenerate window: 60/60, halves the count (pandas 36 vs polars 18). `-[*1..2]-` and wider agree, which is why the existing tests missed it. * undirected `-[*1..k]-` that does not start from the full node set: filtered seed 51/60, non-first segment 36/60. Every directed equivalent agrees, so this is specific to the undirected doubled-pair expansion. The gate keys on an EXPLICIT var-length window rather than on `EdgeSemantics.is_multihop`, because `-[*1..1]-` resolves to min == max == 1 and so is NOT multihop -- yet pandas still routes it through the var-length hop (`-[]-` gives 18 on a graph where `-[*1..1]-` gives 36). Plain `-[]-` is untouched. Tests: explicit decline + still-served neighbours for each of the three, plus a seeded differential fuzz over random small graphs (cyclic, parallel edges, self-loops) that asserts parity-or-raise AND that enough shapes are still served for the check to mean anything. Bounded windows only: undirected unbounded shapes through the pandas oracle can exhaust the box. The new test file is REGISTERED in bin/test-polars.sh. That list is an explicit allowlist, and the polars lane is the only CI lane with polars installed -- so the only lane that can execute this file at all. Without the registration the three `return None` decline statements were the only changed lines no lane ever ran (10/13 = 76.92%, exactly the changed-line-coverage gate failure); with it the block is 13/13. Mutation-checked: dropping the min_hops clause fails 4 tests, the degenerate undirected window 3, the undirected-seed clause 3, the whole gate 8. Full graphistry/tests/compute is unchanged by the declines (9 pre-existing failures, same set). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB * test(gfql): engine-parametrize the #1787 varlen contract (pandas/polars/cuDF/polars-gpu) Review follow-ups on #1794. Comments -> tests: the gate in row_pipeline.py carried per-shape prose asserting which shapes diverge and by how much. A comment asserting behaviour is unverified and rots. Every numeric claim is now a named test; the comment keeps only the WHY of the deliberate divergence from master's serving decision plus a pointer to the tests. 29 -> 18 comment lines, executable logic byte-identical (the only non-comment hunks are two trailing comments on existing lines). Engine-parametrized: renamed to test_varlen_bounded_engine_parity_1787.py and parametrized over pandas / polars / cuDF / polars-gpu. It encodes the INTENDED per-engine behaviour rather than assuming identity -- the polars engines must decline exactly the shapes the pandas-API engines must answer -- with oracle counts pinned as literals so a regression that makes every engine equally wrong still fails. GPU params gate on a runtime probe and report a reasoned SKIP, never a silent pass; the coverage boundary (no CI lane runs cuDF or polars-gpu) is stated in the module docstring. Dropping the module-level polars importorskip also means the pandas params now execute in test-gfql-core, where the old file was skipped in its entirety (re: #1795). Found while doing so: cuDF silently returns 1 where pandas returns 9 on seeded undirected degenerate `(a {p:v})-[*1..1]-(b)` (23/40 random graphs; drop any one of seeded/undirected/max==1 and it agrees). Separate defect, filed as #1798 and pinned here with xfail(strict=True). Mutation-checked both directions: reverting the gate to 233b64c fails 13 tests; widening it to decline all bounded var-length fails 12. Verified: dgx-spark GB10 / cuDF 26.02.01 / polars 1.35.2 via graphistry/test-rapids-official:26.02-gfql-polars with --gpus all -- 80 passed, 1 xfailed, 0 skipped. CI-equivalent local polars lane (cuDF import-blocked) 2395 passed, 54 skipped, 0 failed. bin/lint.sh and bin/typecheck.sh clean. Note: the accompanying CI fix for the test-polars py3.12 timeout (#1797) touches .github/workflows/ci.yml and could not be pushed with the available token, which lacks the `workflow` OAuth scope. The patch is attached to the PR. 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>
1 parent 02041a4 commit d08720e

4 files changed

Lines changed: 466 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
2929
- **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.
3030

3131
### Fixed
32+
- **Three BOUNDED variable-length shapes returned a silently different count on `engine='polars'` (#1787)**: the native polars `rows(binding_ops=...)` builder rebuilds a variable-length segment from the raw matching edge table, and for three bounded shapes that rebuild produces a different edge multiplicity than the pandas oracle — with no error, just a different number. They now raise `NotImplementedError` like every other shape this lowering cannot reproduce, which is a deliberate behaviour change: these were *served* before, so a silent wrong answer becomes a loud, actionable error, and `engine='pandas'` still answers all of them. The three: (1) directed `-[*k..m]->` with `min_hops >= 3`, and `min_hops >= 2` when the segment starts from a filtered seed — `max_reached_hop` in `compute/hop.py` is a dedup-by-node BFS eccentricity rather than a longest-walk length, so the oracle prunes to empty where the rebuild expands a different edge multiset; (2) the DEGENERATE undirected window `-[*1..1]-` / `-[*1]-`, which halved the count — it resolves to `min == max == 1` and so is not "multihop", yet pandas still routes it through the variable-length hop, which is exactly why it slipped past the existing gates (the gate therefore keys on an explicit variable-length window, not on `is_multihop`); (3) undirected `-[*1..k]-` that does not start from the full node set (filtered seed, or a non-first segment), where the doubled-pair expansion over-counts. Every neighbouring shape stays native and is pinned as such — unseeded `min_hops <= 2`, the directed twin of each undirected decline, the directed degenerate window, and the plain `-[]-` edge — so the gate is a scalpel rather than a blanket refusal of variable-length segments. Same root-cause family as the unbounded shapes #1781 declined; the gate should shrink again once the multiplicity is reconstructible. Found by differential fuzzing against the pandas oracle, and pinned by an engine-parametrized suite (pandas / polars / cuDF / polars-gpu) that encodes the intended per-engine behaviour — the polars engines must decline exactly where the pandas-API engines must answer — plus a seeded fuzz that fails if the gate declines too much.
3233
- **`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.
3334
- **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".
3435
- **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).

bin/test-polars.sh

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ POLARS_TEST_FILES=(
2626
graphistry/tests/compute/gfql/test_engine_polars_chain.py
2727
graphistry/tests/compute/gfql/test_engine_polars_row_pipeline.py
2828
graphistry/tests/compute/gfql/test_engine_polars_binding_rows.py
29+
# engine-parametrized (pandas/polars/cudf/polars-gpu); the pandas params also run in
30+
# test-gfql-core, but only this lane has polars installed
31+
graphistry/tests/compute/gfql/test_varlen_bounded_engine_parity_1787.py
2932
graphistry/tests/compute/gfql/test_engine_polars_with_match_reentry.py
3033
graphistry/tests/compute/gfql/test_engine_polars_cypher_conformance.py
3134
graphistry/tests/compute/gfql/test_engine_polars_conformance_matrix.py

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1312,6 +1312,41 @@ def _names(lf: pl.LazyFrame) -> List[str]:
13121312
)
13131313
if _resolved_min != 1:
13141314
return None
1315+
# #1787, same root-cause family as the unbounded shapes #1781 declined: pandas'
1316+
# step_pairs come from the var-length `edge_op.execute` hop, whose hop-window
1317+
# pruning -- and, when seeded, its per-seed BFS -- changes an edge multiplicity
1318+
# this raw-edge rebuild cannot reproduce. Declining is a DELIBERATE divergence
1319+
# from master, which served these: parity-or-NIE means a loud error, never a
1320+
# different number. Shrink the gate again once the multiplicity is reconstructible.
1321+
# WHICH shapes, why each boundary sits where it does, and the counts that prove
1322+
# each one are executable rather than prose -- every claim that used to be written
1323+
# out here is now a named test in:
1324+
# graphistry/tests/compute/gfql/test_varlen_bounded_engine_parity_1787.py
1325+
#
1326+
# Keyed on an EXPLICIT window, NOT on `sem.is_multihop`: `-[*1..1]-` resolves to
1327+
# min == max == 1, is therefore not multihop, and pandas still routes it here.
1328+
if op.min_hops is not None or op.max_hops is not None:
1329+
_vl_max = op.max_hops if op.max_hops is not None else op.hops
1330+
_vl_min = op.min_hops if op.min_hops is not None else (
1331+
op.hops if op.hops is not None else 1
1332+
)
1333+
# a seed is anything that starts the segment from less than the whole node
1334+
# set: a filtered start alias, or a re-entry / `WITH` seed frame
1335+
_prev_op = ops[idx - 1] if idx >= 1 else None
1336+
_seeded_start = start_nodes is not None or (
1337+
isinstance(_prev_op, ASTNode) and bool(_prev_op.filter_dict)
1338+
)
1339+
if _vl_max is not None:
1340+
if op.direction == "undirected":
1341+
if _vl_max == 1: # the degenerate window `-[*1..1]-` / `-[*1]-`
1342+
return None
1343+
if _seeded_start or idx > 1: # doubled-pair expansion over-counts
1344+
return None
1345+
# `max_reached_hop` (compute/hop.py) is a dedup-by-node BFS eccentricity,
1346+
# not a longest-walk length, so pandas prunes where this rebuild expands
1347+
# a different edge multiset
1348+
elif _vl_min >= 3 or (_vl_min >= 2 and _seeded_start):
1349+
return None
13151350
if op.direction not in ("forward", "reverse", "undirected"):
13161351
return None
13171352
if any(

0 commit comments

Comments
 (0)