Skip to content

Commit 3f2128e

Browse files
lmeyerovclaude
andauthored
feat(gfql): native polars unbounded directed var-length rows(binding_ops) — unblocks LDBC IS6 on polars (#1709) (#1781)
* feat(gfql): native polars unbounded directed var-length binding rows (LDBC IS6, #1709) The Cypher multi-alias bindings table (`rows(binding_ops=...)`) lowered natively for fixed-length and BOUNDED variable-length segments, but an unbounded fixed-point segment (`-[*]->` / `-[*0..]->`) declined with "polars engine does not yet natively support cypher row op 'rows'". That was the last shape blocking engine='polars' on LDBC SNB interactive-short-6, the one interactive-short query polars could not answer. Unbounded DIRECTED fixed point now lowers natively. Termination is data-dependent, so instead of pandas' expand-paths-until-empty (which materializes every partial path at every hop) the lowering runs a dedup-by-node frontier walk to find the exhaustion depth D, then reuses the SAME lazy bounded pair-join loop the `-[*1..k]->` arm uses with max_hops=D. Deduping by endpoint changes no walk's EXISTENCE, only its multiplicity, which the bounded loop then reproduces in full -- so the emitted rows are pandas-identical while the exponential path expansion happens once, lazily. A cycle reachable from the seed means infinitely many paths: that raises the same E108 "require terminating variable-length segments" error pandas raises, detected by a pigeonhole bound on the distinct nodes rather than after the blow-up. Also fixes a latent silent-wrong found by the differential fuzz: the polars builder rebuilt bindings from the CHAIN OUTPUT while pandas rebuilds from the pre-chain base graph. The traversal prunes to what IT considers matched, which is not the bindings builder's match set -- a zero-hop var-length segment binds a seed with no matching outgoing edge, and the traversal drops that node. So `-[*0..k]->` could silently return fewer rows than pandas. The builder now sources its node/edge tables from the same base graph pandas uses (and that the indexed builder was already handed). Honest declines unchanged in spirit (NIE, never a silent answer): undirected unbounded, aliased var-length relationships, and unbounded segments without to_fixed_point (pandas truncates at a bound this lowering cannot reconstruct). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 * review: share the bounded var-length expansion; type the polars path bag lazily - extract `_directed_varlen_reachable_polars` so the unbounded fixed-point arm and the bounded `-[*1..k]->` arm run literally the same loop instead of two copies kept in sync by a comment. - pin the generic bindings builder's path bag as a LazyFrame. It always was one; mypy inferred eager because `filter_by_dict_polars` declared the eager type. - make `filter_by_dict_polars` frame-polymorphic via a constrained TypeVar (DataFrame | LazyFrame) rather than declaring one flavour and being called with both -- the eager viz lane and the lazy chain lane take the same `.filter(expr)` path, and the TypeVar keeps the caller's flavour on the way out. Removes the mypy noise this file was carrying, so the new code lands at delta 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 * review: keep the localized ignores matching the new error code Widening filter_by_dict_polars to a constrained TypeVar changes the diagnostic at its two engine-neutral DataFrameT call sites from arg-type to type-var, so the existing localized ignore no longer applied. Retarget it and add the twin in gfql_fast_paths. Both sites are already gated on a polars engine; the cast is what the surrounding code has always asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 * refactor(gfql): move the var-length row specializations out of row_pipeline.py Pure code move, no behaviour change. row_pipeline.py 1638 -> 1528 lines; the unbounded/variable-length arms live in polars/varlen_rows.py with a docstring explaining the exhaustion-depth walk and why deduping by endpoint is sound. Motivation is rebase surface: #1757, #1714 and the seeded-chain work all touch row_pipeline.py, and ~270 lines of var-length specialization sitting in the core flow made every one of those a conflict. RowPipelineMixin stays imported FUNCTION-LOCALLY exactly as at the original site — hoisting it to module scope reintroduces an import cycle (row.pipeline -> lazy engine -> back), so the move preserves it and says why. 2244 polars tests pass, ruff clean. * review(#1781): decline `-[*k..]->` for k>=2, and fix the coverage baseline Adversarial review of this branch found a silent-wrong regression and a red CI. BLOCKER — `-[*k..]->` with k >= 2 returned WRONG COUNTS, no error. The unbounded-directed arm of the `rows` gate had no `min_hops` guard. Cypher `-[*k..]->` lowers to `{hops: null, min_hops: k, to_fixed_point: true, direction: forward}`, so every k >= 2 was SERVED. Pandas' `step_pairs` come from the var-length `edge_op.execute` hop, which empties when its `max_reached_hop < min_hops` and otherwise drops edges labelled below min_hops; `max_reached_hop` is a dedup-by-node BFS eccentricity, not a longest-walk length, so the raw-edge reconstruction here expands a different edge multiset. On a 7-node acyclic graph, both engines answering: MATCH (a)-[*3..]->(b) RETURN count(*) pandas 0 polars 30 MATCH (a {id: 0})-[*2..]->(b) RETURN count(*) pandas 24 polars 41 85 of 1200 acyclic fuzz cases diverged, with zero declines. Master declines all of them. The second gate this PR's description relies on (`_is_native_multihop` in `_chain_traversal_polars`) never runs here: `RETURN count(*)` lowers to a pure-CALL chain, so the `rows` gate is the only gate. The existing decline test missed it because it pins `to_fixed_point=False` — a shape Cypher never emits. Now the unbounded arm requires resolved `min_hops <= 1`, mirroring the undirected arm; 0 and 1 (`-[*]->`, `-[*0..]->`, the IS6 walk) stay served. BLOCKER — CI was red and the coverage gate never ran. `varlen_rows.py` was missing from `coverage_baselines/ci-polars-py3.12.json`, whose own note requires a complete list, so `test-polars (3.12)` exited 3 and `changed-line-coverage` (which needs it) was SKIPPED — this branch's changed lines were never gated. Also fixed: * the cycle path raised a different exception CLASS and `.code` per engine (pandas GFQLTypeError/E303 via execute_call's wrapper, polars a raw GFQLValidationError/E108 because the native kernel runs before that wrapper). Repo control flow keys on `.code`. Now wrapped identically. * cycle detection was bounded by the GLOBAL node count with an eager collect per hop, so a two-node cycle reachable from one seed cost O(graph) collects (measured linear in global N) before raising. Now bounded by the REACHABLE set: a walk of h edges visits h+1 nodes, all within `seen`, so h >= |seen| IS a repeat. Exact in both directions, and the O(E) `node_cap` scan it replaces is gone entirely (`pairs_df.height == 0` is the same test). * the "hoisting reintroduces an import cycle" note was FALSE — disproven by hoisting. The sibling import moves to the import block and the mid-file `# noqa: E402` goes; `RowPipelineMixin` stays function-local, matching the engine convention. Unused typing imports and a stale line count removed. * `to_fixed_point` WITH an explicit bound was newly served and undocumented. It is parity-correct (pandas ignores the flag once max_hops is set) but was pinned by nothing; now tested and in the changelog. Tests, all mutation-checked (reverting each fix fails the matching test): * gate-level, k in 0..3, either side of the boundary * end-to-end through Cypher on the repro graph — the one that actually catches the blocker, since both engines answer and only values differ * the cycle test now asserts class and `.code` are ENGINE-INDEPENDENT rather than pinning polars' own code (the old form passed while they disagreed) * a long acyclic walk must not be mistaken for a cycle, and a small reachable cycle behind a large unreachable remainder must still raise — the two failure modes of the new bound Verified in-container on dgx-spark: 86 passed in the binding-rows file, 276 with the row-pipeline file, and `graphistry/tests/compute` with `--gpus all` gives an IDENTICAL 9-failure set to this branch's base (pre-existing dask/cuDF coercion), 6821 passed vs the base's 6804. mypy differential: 166 errors on both trees, so these changes add none. ruff clean under the repo config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB * review(#1781): decline to_fixed_point with an explicit bound The re-review of the previous commit found that the gate rewrite which closed the unbounded `-[*k..]->` hole left a narrower one open, and that the test I added to pin it could not have caught it. Master declined on `bool(op.to_fixed_point)` ALONE. Restructuring the gate to key on the RESOLVED MAX incidentally let `to_fixed_point=True` combined with an explicit bound fall through to the bounded arm — where it hits the same reconstruction gap as the unbounded case, silently, for min_hops >= 3: fwd min=3 max=4 tfp=True pandas 0 polars 30 (master: NIE) fwd min=3 max=5 tfp=True pandas 0 polars 30 fwd min=4 max=5 tfp=True pandas 0 polars 6 fwd hops=3 tfp=True pandas 0 polars 24 reverse spellings: identical Not reachable from Cypher — `cypher/parser.py` sets to_fixed_point False for `*k` and `*i..k`, and only leaves it True for `*` and `*k..`, which always have max_hops None. So this is the AST / `rows(binding_ops=...)` wire surface only. Hard to reach is not correct, and the previous commit additionally claimed in the CHANGELOG that the shape "matches pandas", which is false. Declining it restores master's behaviour exactly, so it cannot regress anything that previously worked. WHY THE OLD TEST WAS BLIND, since the same mistake is easy to repeat: it compared flagged-vs-unflagged WITHIN each engine and its parametrization stopped at min_hops=2. A within-engine comparison never consults the pandas oracle, so it cannot see a divergence where BOTH engines answer; and min_hops <= 2 happens to agree on that fixture. It now asserts the DECLINE, runs to min_hops=4, and is joined by a value test on the divergence graph that compares ROW COUNTS against the oracle (engine-neutral: a bare `rows()` call emits engine-specific scaffolding columns on every shape, pre-existing and unrelated). Mutation-checked: removing the guard fails 9 tests, including the value test — the property the previous version lacked. Verified in-container on dgx-spark: binding-rows file 90 passed; `graphistry/tests/compute` with `--gpus all` gives 6825 passed and a failure set byte-identical to this branch's base (the 9 pre-existing dask/cuDF coercion failures). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 674443e commit 3f2128e

10 files changed

Lines changed: 697 additions & 85 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
99
<!-- Do Not Erase This Section - Used for tracking unreleased changes -->
1010

1111
### Added
12+
- **Native polars `rows(binding_ops=...)` for UNBOUNDED directed variable-length patterns (`-[*]->` / `-[*0..]->`) (#1709)**: the Cypher multi-alias bindings table already lowered natively for fixed-length and *bounded* variable-length segments, but an unbounded fixed-point segment declined with `NotImplementedError: polars engine does not yet natively support cypher row op 'rows'`. This was the last shape blocking `engine='polars'` on LDBC SNB interactive-short-6 (`MATCH (m:Message)-[:REPLY_OF*0..]->(p:Post)<-[:CONTAINER_OF]-(f:Forum)-[:HAS_MODERATOR]->(mod)`), the one interactive-short query polars could not answer. It now runs natively: a dedup-by-node frontier walk finds the exhaustion depth, then the SAME lazy bounded pair-join loop the `-[*1..k]->` arm uses materializes one row per distinct edge SEQUENCE (Cypher path multiplicity, parallel edges included) — no pandas bridge, no `to_pandas()` round trip. A cycle reachable from the seed means infinitely many paths; that raises the same E108 "require terminating variable-length segments" error pandas raises — same exception class and same `.code` on both engines — and is detected before the path expansion blows up rather than after, bounded by the REACHABLE node count so an unreachable remainder of the graph costs nothing. Still declining honestly (NIE, never a silent answer): UNDIRECTED unbounded (`-[*]-`, needs the min_hops == 1 multiplicity reconstruction plus backtrack-aware termination — pandas rejects it outright), aliased variable-length relationships (pandas rejects those too), unbounded segments WITHOUT `to_fixed_point` (pandas silently truncates at a bound this lowering cannot reconstruct), unbounded segments with `min_hops >= 2` (`-[*2..]->`: pandas' step pairs are pruned by min_hops against a dedup-by-node eccentricity, which this raw-edge reconstruction cannot reproduce — serving it would return a different count with no error), and `to_fixed_point` combined with an explicit bound (declined on master too; it is not Cypher-reachable, since the parser only sets the flag for `*` / `*k..` where there is no maximum, but through the AST surface it hits the same reconstruction gap for `min_hops >= 3`). Cross-engine parity is the gate: differential fuzz vs the pandas oracle over random DAGs and cyclic graphs with self-loops and parallel edges, plus pinned IS6/zero-hop/multiplicity/cycle tests.
13+
1214
- **GFQL secondary node property indexes (`create_index('node_prop', column=...)` / `g.gfql_index_node_props([...])`)**: a seed predicate on a NON-key column — `MATCH (m {id: 42})` where the graph's node id binding is some other column — previously cost a full node scan, because the registry only indexed the node-id binding and the CSR adjacencies. A property index is the same pay-as-you-go sidecar as the existing kinds: sorted distinct values over node **row positions** (CSR, so duplicate values are indexable), never reorders `.nodes`, fingerprint-validated so a `.nodes()` rebind is treated as absent (safe miss, never a wrong answer), engine-polymorphic (numpy host / cupy on-device), and policy-gated (`off`/`use`/`auto`/`force`). The seeded fixed-hop planner picks the **most selective** indexed scalar predicate in the seed filter using a free CSR-offset estimate, gathers those candidates, and applies every remaining predicate to them — so results are identical whether the index is present, absent, stale, or cost-gated out. `show_indexes()` lists property indexes; `drop_index('node_prop', column=...)` drops one. Only integer columns are indexable today (float NaN ordering, strings on cupy, and nulls all decline to the scan); widening that is additive. **Perf (dgx-spark, official LDBC SNB SF1, 3.18M nodes, warm median, value-identical 19-row result):** interactive-short IS7 `71.6 ms -> 19.5 ms` (**3.7x**), with a one-time `112 ms` build — the seed lookup itself goes from a `51.2 ms` scan to `0.096 ms`.
1315

1416
### Performance
@@ -25,6 +27,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
2527
- **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.
2628

2729
### Fixed
30+
- **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.
31+
2832
- **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.
2933
- **Seeded property-RETURN dtype divergence on cuDF**: the lean projection applied the pandas rows-pivot artifact (int → float64, bool → object) on every engine, but cuDF's canonical pivot preserves the source dtypes — so the fast path returned `float64`/`object` where cuDF's own canonical path returns `int64`/`bool`. The cast rule is now engine-aware; the dtype-class decline guard is unchanged.
3034

graphistry/compute/gfql/index/bindings.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,9 @@ def _filter_frame(
149149
filter_by_dict_polars,
150150
)
151151

152-
return cast(DataFrameT, filter_by_dict_polars(frame, filter_dict)) # type: ignore[arg-type]
152+
# frame is DataFrameT (engine-neutral); on this branch it IS a polars frame,
153+
# which the helper's constrained TypeVar cannot see through the alias
154+
return cast(DataFrameT, filter_by_dict_polars(frame, filter_dict)) # type: ignore[type-var]
153155
from graphistry.compute.filter_by_dict import filter_by_dict
154156

155157
return filter_by_dict(frame, filter_dict, engine) # type: ignore[arg-type]

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -508,8 +508,27 @@ def _run_calls_polars(g_cur, calls, start_nodes, base_graph, middle):
508508
# is MECHANICAL (is_row_pipeline_call), not curated. (umap/hypergraph never reach here —
509509
# the generic chain routes schema-changers straight to execute_call.)
510510
from graphistry.compute.gfql.row.pipeline import is_row_pipeline_call
511+
from graphistry.compute.exceptions import ErrorCode, GFQLTypeError, GFQLValidationError
511512
for op in calls:
512-
native = _try_native_row_op(g_cur, op)
513+
try:
514+
native = _try_native_row_op(g_cur, op)
515+
except GFQLTypeError:
516+
raise
517+
except GFQLValidationError as validation_error:
518+
# Same wrapping `execute_call` applies (gfql/call/executor.py): a kernel that
519+
# raises a validation error surfaces as GFQLTypeError(E303) with this message
520+
# shape. The native attempt runs BEFORE execute_call, so without this the SAME
521+
# query carries a different class AND a different `.code` per engine — and this
522+
# repo's control flow keys on `.code`. Scoped to GFQLValidationError, the one
523+
# divergence actually observed (an E108 from the var-length cycle guard);
524+
# other exception classes are left alone rather than blanket-normalized.
525+
fn_name = getattr(op, "function", None)
526+
raise GFQLTypeError(
527+
ErrorCode.E303,
528+
f"Error executing '{fn_name}': {validation_error}",
529+
field="function",
530+
value=fn_name,
531+
) from validation_error
513532
if native is not None:
514533
g_cur = native
515534
continue

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
import operator
1212
import re
13-
from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Union
13+
from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, TypeVar, Union
1414

1515
from graphistry.compute.predicates.ASTPredicate import ASTPredicate
1616
from graphistry.compute.predicates.str import Contains, Endswith, Fullmatch, Match, Startswith
@@ -21,6 +21,12 @@
2121
import datetime
2222
import polars as pl
2323

24+
# These helpers are frame-polymorphic: the eager (viz/crossfilter) lane hands them
25+
# DataFrames, the lazy chain/bindings lane hands them LazyFrames, and both take the
26+
# SAME `.filter(expr)` path. A constrained TypeVar says exactly that and keeps the
27+
# caller's flavour on the way out (a plain union would not).
28+
PolarsFrameT = TypeVar("PolarsFrameT", "pl.DataFrame", "pl.LazyFrame")
29+
2430
# Comparison-predicate RHS: genuinely dynamic (Cypher properties are dynamically typed) — a
2531
# python scalar or a GFQL/py temporal matched structurally by type(val).__name__
2632
# (DateValue/TemporalValue/…, never imported here).
@@ -282,7 +288,7 @@ def _is_membership(value: Any) -> bool:
282288
return isinstance(value, (list, tuple, set, frozenset))
283289

284290

285-
def _is_cross_type_predicate(df: "pl.DataFrame", col: str, pred: ASTPredicate) -> bool:
291+
def _is_cross_type_predicate(df: "Union[pl.DataFrame, pl.LazyFrame]", col: str, pred: ASTPredicate) -> bool:
286292
"""True iff the predicate compares a numeric column to a string value (or vice versa):
287293
polars raises `cannot compare string with numeric type` (an uncatchable Rust panic when
288294
nested); pandas/cypher return a value/null. Recurses into AllOf (fold of x>a AND x<b) and
@@ -307,15 +313,15 @@ def _mismatch(v: Any) -> bool:
307313
return _mismatch(val)
308314

309315

310-
def filter_by_dict_polars(df: "pl.DataFrame", filter_dict: "Optional[Dict[str, Any]]") -> "pl.DataFrame":
316+
def filter_by_dict_polars(df: "PolarsFrameT", filter_dict: "Optional[Dict[str, Any]]") -> "PolarsFrameT":
311317
"""Return rows of polars ``df`` matching all entries in ``filter_dict`` via one filter."""
312318
combined = filter_expr_by_dict_polars(df, filter_dict)
313319
if combined is None:
314320
return df
315321
return df.filter(combined)
316322

317323

318-
def filter_expr_by_dict_polars(df: "pl.DataFrame", filter_dict: "Optional[Dict[str, Any]]") -> "Optional[pl.Expr]":
324+
def filter_expr_by_dict_polars(df: "Union[pl.DataFrame, pl.LazyFrame]", filter_dict: "Optional[Dict[str, Any]]") -> "Optional[pl.Expr]":
319325
"""Build the combined boolean ``pl.Expr`` filter_by_dict_polars would apply, or None
320326
for an empty/absent filter dict. ``df`` supplies the schema for column/dtype
321327
resolution only — callers may apply the expr to a LazyFrame over the same schema

0 commit comments

Comments
 (0)