perf(gfql): dispatch indexed fixed-hop bindings before the canonical traversal#1776
Merged
Conversation
…traversal The resident-index fixed-hop path (`rows(binding_ops=...)`, the Cypher multi-alias MATCH...RETURN shape) was only consulted inside row materialization — after the engine had already executed the full canonical traversal — so its compact path bag was built on top of the graph-sized work it exists to avoid. Both boundaries now ask the same shared structural gate first: - pandas/cuDF: `_handle_boundary_calls` in compute/chain.py - native Polars: `chain_polars` in gfql/lazy/engine/polars/chain.py Only when the helper can serve the WHOLE middle exactly do they skip the canonical traversal and hand the compact state to the unchanged row materializer through an internal graph copy; a decline is memoized so the rows path does not re-attempt (or re-record) the same decision. Engagement remains operator/index/dtype/cost based — no query, schema, or hop-count recognition — and seeded, prefiltered, policy-bearing, shortest-path, unsupported, and cost-gated shapes still fall back with identical results. Also in this change: - indexed seed lookup now covers a unique node id PLUS extra scalar constraints (gather the one indexed row, then filter it) instead of scanning the whole node table; - the seeded typed-hop property projection keeps its fast path through trailing DISTINCT / ORDER BY / SKIP / LIMIT by delegating those plain frame ops to the canonical chain; - fix a pre-existing cuDF dtype divergence in that projection: the pandas rows-pivot upcast (int -> float64, bool -> object) was applied on every engine, but cuDF's canonical pivot preserves source dtypes; - keep the empty edge frame's alias marker columns in each engine's own canonical column order (pandas puts them first, polars appends them). Tests are standard-derived (LDBC-shaped, names only in test ids): exact value/order/dtype/index/schema parity vs the same-engine canonical path, one trace decision per serve/decline, no-canonical-traversal proofs for both pandas and polars, an unnamed-middle regression that pins the bypass gate, and a mixed-dtype projection parity case across pandas/polars/cuDF. Validated on dgx-spark under the capped guard: full CPU `graphistry/tests/compute` 6232 passed with only master's pre-existing dask failure, cuDF lane 30 passed, ruff clean, mypy no new errors vs master. On a standard LDBC SNB SF1 interactive-short profile the redundant two-hop typed-mask and 16-merge buckets disappear and the profiled call drops ~11.9x (1263 ms -> 106 ms) with the exact 19-row oracle preserved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
…text Classifying WHY the indexed fixed-hop path declined re-validates index fingerprints and can filter the seed frame — diagnostic work whose only consumer is the trace step that `_record_indexed_traversal` drops when no `index_trace()`/`gfql_explain` context is active. It ran unconditionally on every decline, against the module's own stated contract that diagnostic enrichment costs the hot path nothing. Gate it on `_trace_active()`; untraced declines report `not_traced`. Every reason assertion in the suite runs inside `index_trace()`, so the exact classifications are unchanged where they are observed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
lmeyerov
commented
Jul 25, 2026
| return cast(DataFrameT, out) | ||
|
|
||
|
|
||
| def _concat(frames: Sequence[DataFrameT], engine: Engine) -> DataFrameT: |
Contributor
Author
There was a problem hiding this comment.
DRY or at least put in proprer files
lmeyerov
commented
Jul 25, 2026
| frame.with_columns(pl.Series(_EDGE_ORD, np.asarray(positions))), # type: ignore[operator] | ||
| ) | ||
| out = frame.copy() | ||
| out[_EDGE_ORD] = positions |
Contributor
Author
There was a problem hiding this comment.
shouldn't all these .copy() -> mutate be .assign() ?
lmeyerov
commented
Jul 25, 2026
| return one(src, dst, 0) | ||
|
|
||
|
|
||
| def _estimate_join_rows( |
Contributor
Author
There was a problem hiding this comment.
ins't there a file just for join/merges?
…doff Manual review flagged the dynamic typing this PR introduced: three separate `_gfql_indexed_bindings_*` attributes were attached with `setattr` and read back with `getattr` across four modules, with the plan-equality check re-derived at each site. They collapse into one `IndexedBindingsHandoff` dataclass (`index/handoff.py`), which owns the only attribute access for it and answers the two questions callers actually ask — `serves(binding_ops, engine)` and `declined(binding_ops)`. The chain boundary, the polars chain, both row pipelines, and `frame_ops` now make typed calls instead of smuggling attributes, and the row-pipeline adapter copies one field instead of two. Also from the same review: the review skill now documents the batched-`.assign()` rule (copy-then-mutate should be a single `assign`, and `rename`/boolean-masking already copy), which the shared-file follow-ups apply. Validated on dgx-spark against THIS branch alone: full CPU `graphistry/tests/compute` 6232 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 60 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
Follow-up review asked whether the `set_handoff` flow was error-prone. It was, in two ways that types did not catch: 1. `set_handoff(g_temp, ...)` MUTATED a graph in place on the decline path. That was safe only because of an invariant three branches away — `_chain_impl` had always rebuilt `g_temp` by then — so nothing but statement order stopped it writing onto the caller's own graph. 2. One decision was spread over three mutable locals (`indexed_middle_state`, `indexed_middle_attempted`, `g_temp`) whose illegal combinations were prevented only by ordering, with `serialize_binding_ops(middle)` recomputed three times. Now a single `_plan_indexed_middle(...)` returns `Optional[IndexedBindingsHandoff]` — `None` = the indexed path does not apply, a handoff WITH state = it serves, a handoff WITHOUT state = it was tried and declined — computed once, with the gate extracted into that named predicate so it reads as the twin of the polars chain's `_try_indexed_middle_polars` instead of a twelve-condition chain inline. Both boundaries now only ATTACH, never mutate, and the row-pipeline adapter uses the typed setter rather than `setattr`. `ASTCall` moves to the module-scope `.ast` import, since the extracted predicate needs it (a function-local import in the old inline site is what made this non-obvious). Validated on this branch: full CPU `graphistry/tests/compute` 6232 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 60 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
…g them `with_index_policy` did `setattr(out, POLICY_ATTR, policy)`, and the whole `_gfql_*` execution context was attached to instances by name and read back with `getattr(..., default)` — untyped, with every reader repeating the default. The family is now DECLARED on `Plottable`, with defaults on `PlotterBase` (`bind()` is `copy.copy`, so a per-graph value still travels with the instance): `_gfql_index_policy`, `_gfql_index_registry`, `_gfql_indexed_bindings_handoff`, `_gfql_rows_base_graph`, `_gfql_start_nodes`, `_gfql_rows_edge_aliases`, `_gfql_shortest_path_backend`. `RowPipelineMixin` mirrors them (its `_RowPipelineAdapter` is not a `PlotterBase`) and the `RowPipelineCtx` protocol declares the three it reads. Access in this stack's files is then ordinary typed attribute access: `index/handoff.py` drops to ZERO dynamic attribute calls, and the policy/registry accessors, both chain boundaries, both row pipelines, and `frame_ops` stop using `getattr`/`setattr` for this family. Two real bugs fell out, both invisible under `getattr(..., default)`: - `_SyntheticRowGraph` (a hand-rolled Plottable stand-in in the cypher lowering) set three of the fields and silently omitted `_gfql_rows_edge_aliases` and the handoff. It now constructs the full context. - Declaring a default DISABLES any `getattr(x, name, fallback)` whose fallback differs from it: `get_registry` fell back to `EMPTY_REGISTRY` while the declared default is `None`, so it started returning `None`. Converted here; audited every other reader of these fields — the only two left (`polars/pattern_apply.py`) use `None`, which matches. EXCLUDED deliberately: `_gfql_native_sp_cache` (a different subsystem's cache with no call site in this stack) and the repo-wide sweep of the same pattern outside these files. Validated on this branch alone: full CPU `graphistry/tests/compute` 6232 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 60 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
lmeyerov
commented
Jul 26, 2026
| g._gfql_indexed_bindings_handoff = handoff | ||
|
|
||
|
|
||
| def read_handoff(g: Any) -> Optional[IndexedBindingsHandoff]: |
lmeyerov
commented
Jul 26, 2026
| ops: Sequence[Any], | ||
| state: Any, | ||
| alias_frames: Dict[str, Any], | ||
| node_id: str, |
An audit of all seven declared fields found four non-test sites still using getattr/setattr (gfql_unified.py x2, polars/pattern_apply.py x2), now converted; repo-wide non-test dynamic access to this family is zero. The same audit found the _SyntheticRowGraph bug class was not fully closed: both hand-rolled Plottable stand-ins were missing _gfql_index_registry, which get_registry now reads directly, and the row pipeline's base_graph can be the adapter. Both carry it now. Also annotated the polars _try_indexed_middle_polars gate. Validated on this branch: CPU 6232 passed with only master's pre-existing test_chain_dask_edges; cuDF 60 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
lmeyerov
commented
Jul 26, 2026
| _edge: Any | ||
| _gfql_rows_base_graph: Optional["Plottable"] | ||
| _gfql_start_nodes: Any | ||
| _gfql_rows_edge_aliases: Any |
Contributor
Author
There was a problem hiding this comment.
More Any to refine
lmeyerov
commented
Jul 26, 2026
| base_graph = ctx._gfql_rows_base_graph | ||
| if base_graph is None: | ||
| base_graph = getattr(ctx, "_g", None) | ||
| base_graph = getattr(ctx, "_g", None) # adapter-only back-reference |
Contributor
Author
There was a problem hiding this comment.
Fix all these getattr
… decline **`row/frame_ops.py` declared its context protocol as `Any`.** The `RowPipelineCtx` fields are now typed — `Optional[DataFrameT]` for frames, `Optional[str]` for id bindings, `Optional[Plottable]` for the base graph and the adapter's `_g` back-reference, `Optional[Iterable[str]]` for edge aliases — via a `TYPE_CHECKING` import of `DataFrameT`. Three polars-on-`DataFrameT` calls the sharper types expose take localized `# type: ignore`s, the documented convention for engine-polymorphic frames. **`row/frame_ops.py` still used `getattr`.** All gone: `_g` is a declared field (defaulted on `PlotterBase`, set only by adapters), and the two pre-existing `getattr(base_graph, "_nodes"/"_edges", None)` are direct access now that the fields are declared. That file has ZERO `getattr`. **`polars/row_pipeline.py` swallowed unexpected exceptions.** Extracting `_finish_binding_rows_polars` made the INDEXED path share the generic path's `except pl.exceptions.SchemaError: return None`. That rationale belongs to the generic builder — polars will not unify int/float join keys the way pandas implicitly does, so declining is honest — whereas the indexed state comes from a helper that already verified those dtypes, so a `SchemaError` there is a BUG turned into a silent slow-path fallback. The finisher now takes `decline_on_schema_error`; the indexed caller passes `False` and lets it raise. `ops` there is now `Sequence[ASTObject]` with real `isinstance` narrowing, but `state`/`alias_frames` stay `Any` ON PURPOSE: the same parameter receives a polars eager frame, a polars LazyFrame, and the engine-polymorphic `DataFrameT` the indexed helper returns, so a precise union only buys `# type: ignore`s (an earlier attempt at one produced exactly 6). The comment records that. Validated on this branch: CPU 6232 passed with only master's pre-existing `test_chain_dask_edges`; cuDF 60 passed; ruff clean; mypy unchanged vs master, and `--warn-unused-ignores` finds none of the new suppressions unnecessary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
…cisely The declared fields and the polars finisher were still `Any`. `_gfql_start_nodes` is now `Optional[DataFrameT]` and `_gfql_rows_edge_aliases` `Optional[Iterable[str]]` on `Plottable`, `PlotterBase`, and `RowPipelineMixin` (`_g` / `_gfql_rows_base_graph` are `Optional[Plottable]`). `Plottable` types its own `_nodes`/`_edges` as `Any`; matching that would have been consistent, but new fields should not add to that debt. `_finish_binding_rows_polars`'s `state`/`alias_frames` are a CONSTRAINED TypeVar over polars eager/lazy frames rather than `Any`. A plain union does not work — `state.join(lookup)` needs both sides to be the same flavour, which only a TypeVar expresses. The engine-polymorphic `DataFrameT` the indexed helper returns is narrowed once at that call with a comment; the generic caller carries a single `[misc]` because its own pre-existing branches mix eager and lazy frames, so inference cannot pick one there. Two `[operator]` ignores cover polars calls on `DataFrameT`-typed seeds — the documented convention. The precision paid for itself immediately: mypy flagged that `_RowPipelineAdapter.bind()` dereferences `_g`, which the declaration made Optional. Narrowing the adapter's own attribute would break its structural match against `RowPipelineCtx` (a protocol's mutable attributes are invariant), so `bind()` asserts the constructor's invariant instead. Validated on this branch: CPU 6232 passed with only master's pre-existing `test_chain_dask_edges`; cuDF 60 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
The guard caps `lowering.py` line count (9249). This branch adds SIX lines to it: `_SyntheticRowGraph` — a hand-rolled Plottable stand-in — was constructing only part of the GFQL execution context, which the newly declared fields turned from a silent `getattr(..., None)` into an AttributeError. It now sets the full context. No cypher surface grew: field and property counts are unchanged (8/8, 7/7, 7/7 and 10/10, 0/0, 0/0). Only the line cap moves, 9249 -> 9255. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
…clared execution context The entry covered the three perf items and the cuDF dtype fix but not the correctness fix this branch also makes: the boundary accepted a rows() call with no binding ops over an UNNAMED middle, which reads the traversal-narrowed node table while the bypass hands it the full graph — it would have returned every node. Also notes the declared `_gfql_*` execution context, since hand-rolled Plottable stand-ins must now construct all of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The resident-index fixed-hop path (
rows(binding_ops=...)— the Cypher multi-aliasMATCH … RETURNshape) was only consulted inside row materialization, after the engine had already executed the full canonical traversal. It therefore built its compact path bag on top of the graph-sized work it exists to avoid.Both boundaries now ask the same shared, structural gate first:
_handle_boundary_calls(compute/chain.py)chain_polars(gfql/lazy/engine/polars/chain.py)Only when the helper can serve the whole middle exactly do they skip the canonical traversal and hand the compact state to the unchanged row materializer via an internal graph copy. A decline is memoized so the rows path does not re-attempt (or re-record) the same decision.
Engagement stays operator / index / dtype / cost based — no query, schema, or hop-count recognition. Seeded, prefiltered, policy-bearing, shortest-path, unsupported, and cost-gated shapes still fall back to canonical execution with identical results.
Also included:
{id, …extra scalar constraints}— gather the one indexed row, then filter it, instead of scanning the whole node table. Missing ids and non-matching constraints return the same empty result; duplicate-id graphs cannot build the index and are unaffected.DISTINCT/ORDER BY/SKIP/LIMITby delegating those plain frame ops to the canonical chain.float64, bool →object) on every engine, but cuDF's canonical pivot preserves source dtypes — so the fast path returnedfloat64/objectwhere cuDF's own canonical path returnsint64/bool. The cast rule is now engine-aware; the dtype-class decline guard is unchanged.Not in this PR
Engine.pyAUTO source-native routing is unchanged — the deferred#1743policy is deliberately out of scope, and the two tests that pinned it were dropped.Tests
Standard-derived (LDBC-shaped; benchmark names appear only in test ids/comments), all comparing against the same-engine canonical path:
offindex lifecycle, named-edge schema, policy hooks, shortest-path exclusion, exception propagation;ASTEdge.executeforbidden) and polars (_chain_traversal_polarsforbidden);test_out_of_shape_declines_with_parityreconciled:DISTINCTandORDER BY … LIMITmoved to a new engages-with-parity case, since they are now served with proven parity.Validation (dgx-spark, capped guard, read-only source)
graphistry/tests/compute: 6232 passed, 1 failed — master's pre-existingtest_chain_dask_edgesruff: clean ·mypy: 0 new errors vs master🤖 Generated with Claude Code
https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
Review follow-ups (2026-07-25)
Manual review flagged the dynamic typing this PR introduced. Addressed in
83212ab1:_gfql_indexed_bindings_*attributes collapse into one typedIndexedBindingsHandoffdataclass (gfql/index/handoff.py) that owns the only attribute access for them and answersserves(binding_ops, engine)/declined(binding_ops), instead of every call site re-deriving plan equality withgetattr/setattr.chain.py, the polars chain, both row pipelines, andframe_opsnow make typed calls.agents/skills/review/SKILL.md) now documents the batched-.assign()rule (copy-then-mutate → oneassign;renameand boolean-masking already copy).Verified on this branch alone: full CPU
graphistry/tests/compute6232 passed with only master's pre-existingtest_chain_dask_edges; cuDF lane 60 passed; ruff clean; mypy unchanged vs master.The remaining review items land in the stacked #1777, because they touch files that PR also modifies (
index/bindings.py,index/lookup.py,index/api.py): the_concat/join/degree-helper relocation and DRY, the.assign()conversions, the decline-reason fix, and the cross-engine test amplification. Reviewing the stack top-down shows the final state.Engine-shape divergences found along the way are filed separately as #1779.
Review follow-ups, round 2
7777e191declared fields. The_gfql_*execution context is now DECLARED onPlottablewith defaults onPlotterBase(bind()iscopy.copy, so per-graph values still travel), mirrored onRowPipelineMixin(its adapter is not aPlotterBase) and in theRowPipelineCtxprotocol.index/handoff.pydrops to zero dynamic attribute calls; the policy/registry accessors, both chain boundaries, both row pipelines, andframe_opsstop usinggetattr/setattrfor this family. Two real bugs fell out, both invisible undergetattr(..., default):_SyntheticRowGraphsilently omitted two fields, and declaring a default disables anygetattr(x, name, fallback)whose fallback differs —get_registryfell back toEMPTY_REGISTRYwhile the declared default isNone. Every other reader was audited; the two that remain useNone, which matches. Deliberately excluded:_gfql_native_sp_cache(different subsystem) and the repo-wide sweep.3015b736purely functional boundary.set_handoffmutated a graph in place, safe only via an invariant three branches away, and one decision was spread over three mutable locals. Now a single_plan_indexed_middle(...)returnsOptional[IndexedBindingsHandoff]—None= doesn't apply, with state = serves, without state = tried and declined — computed once, with the gate as a named predicate mirroring the polars twin. Both boundaries only attach, never mutate.Verified on this branch alone after each step: CPU 6232 passed (only master's pre-existing
test_chain_dask_edges), cuDF 60 passed, ruff clean, mypy unchanged vs master.Review follow-ups, round 3
54d6e77f—row/frame_ops.py's context protocol was declaredAny; it now types frames asOptional[DataFrameT], ids asOptional[str], the base graph and the adapter_gback-reference asOptional[Plottable], and edge aliases asOptional[Iterable[str]]. That file now has zerogetattr(including two pre-existing ones, pointless once the fields are declared). Three polars-on-DataFrameTcalls the sharper types expose take localized ignores — the documented convention for engine-polymorphic frames.54d6e77f—polars/row_pipeline.pyswallowed unexpected exceptions: extracting the finisher made the indexed path share the generic path'sexcept SchemaError: return None. That rationale is the generic builder's (polars won't unify int/float join keys the way pandas implicitly does), while the indexed state comes from a helper that already verified those dtypes — so aSchemaErrorthere is a bug being turned into a silent slow-path fallback. The finisher now takesdecline_on_schema_error; the indexed caller passesFalse.6c41ee8e— completed the field conversion: four remaining non-testgetattr/setattrsites converted, and both hand-rolled Plottable stand-ins were missing_gfql_index_registry(reachable, since the row pipeline'sbase_graphcan be the adapter).On
Anyinpolars/row_pipeline.py:opsis nowSequence[ASTObject]with realisinstancenarrowing, butstate/alias_framesstayAnydeliberately — the same parameter receives a polars eager frame, a polarsLazyFrame, and the engine-polymorphicDataFrameTthe indexed helper returns. A precise union there produced exactly 6 mypy errors, i.e. it buys only suppressions; the comment records that.Verified on this branch after each step: CPU 6232 passed (only master's pre-existing
test_chain_dask_edges), cuDF 60 passed, ruff clean, mypy unchanged vs master, no unnecessary suppressions.