perf(gfql): secondary node property indexes for seeded traversal#1777
Merged
Conversation
A seed predicate on a NON-key column — `MATCH (m {id: 42})` where the graph's
node id binding is some other column — cost a full node scan: the registry only
indexed the node-id binding and the CSR adjacencies, so a property predicate had
no index to consult. On official LDBC SNB SF1 that single scan was 51.8 ms of
IS7's 72 ms wall (3,181,724 x 32 rows examined to find one row), while every
other step of the traversal totalled under 2 ms.
Add the property index as the same pay-as-you-go sidecar as the existing kinds:
- `NodePropIndex`: sorted distinct values over node ROW POSITIONS in CSR form, so
duplicate values are indexable (unlike the unique-only node-id index). Never
reorders `.nodes`; fingerprint- and identity-validated, so a `.nodes()` rebind
is a safe miss rather than a wrong answer; engine-polymorphic (numpy host,
cupy on-device). Stored in a column-keyed map beside the kind-keyed one.
- `build_node_prop_index` declines (-> scan) for anything whose vectorized
ordering/equality is not unambiguous on both backends: today integer dtypes
only, since float NaN ordering, cupy strings, and nulls each diverge. Widening
it later is purely additive.
- `lookup_prop_rows` (searchsorted + CSR range expansion) and `prop_match_count`,
a free selectivity estimate read straight off the CSR offsets.
- The seeded fixed-hop planner picks the MOST SELECTIVE indexed scalar predicate
in the seed filter, gathers those candidates, and hands them to the UNCHANGED
filter, which applies every remaining predicate. Results are therefore
identical whether the index is present, absent, stale, or cost-gated out.
- Surface: `create_index('node_prop', column=...)` (column required),
`drop_index('node_prop', column=...)`, `show_indexes()` rows, and
`g.gfql_index_node_props([...])`, which skips unindexable columns.
Opt-in, like the secondary indexes of other engines: with no property index
resident, behaviour and cost are exactly as before.
Perf (dgx-spark, official LDBC SNB SF1, warm medians, value-identical 19 rows):
interactive-short IS7 71.6 ms -> 19.5 ms (3.7x), one-time build 112 ms; the seed
lookup alone goes 51.2 ms -> 0.096 ms. A position-balanced A/B of IS1-IS7 with
NO property index resident shows no detectable change (per-query deltas inside
each build's own 6-10% run-to-run spread).
Tests are standard-derived: seed-without-scanning parity plus candidate width on
pandas/polars/cuDF, indexed-vs-absent equality, duplicate-value keys gathering
every matching row, stale-rebind and policy-off fallback, unindexable dtype /
missing column / missing argument errors alongside the skipping convenience
wrapper, most-selective-column choice, and the show/drop lifecycle.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
Manual review raised DRY/placement, functional-style, dynamic-typing, and test amplification concerns. Changes, by concern: DRY / placement - `bindings._concat` duplicated `graphistry.Engine.df_concat` (whose polars twin already accepts `ignore_index` for exactly this) — deleted, call site rewired. - `_estimate_join_rows`, `_join_state`, `_filter_oriented_endpoints` moved to `compute/dataframe/join.py`, where the other join/merge helpers live, and generalized to explicit column parameters instead of module-private constants: `estimate_inner_join_rows`, `path_ordered_expand_join`, `semijoin_by_column`. - `_lookup_degree` moved to `index/lookup.py` and now shares one CSR probe with the property lookup (`_csr_hit_positions` / `csr_match_count` / `csr_gather_rows`), which were three copies of searchsorted + membership. - The helper count in `bindings.py` drops from 17 to 12, all index-specific. Functional style - Copy-then-mutate becomes a single batched `.assign(...)`: `_frame_with_positions`, `_with_marker`, `_orient_edges`, the seed state, and the frame_ops edge-marker synthesis. Also dropped a redundant `.copy()` before `.rename()` and after a boolean mask (both already copy). The rule is now written down in `agents/skills/review/SKILL.md` so review catches it next time. Dynamic typing - The three smuggled `_gfql_indexed_bindings_*` attributes collapse into ONE typed `IndexedBindingsHandoff` dataclass in `index/handoff.py`, which owns the only attribute access for it (`attach_handoff` / `set_handoff` / `read_handoff` / `clear_handoff`) and answers `serves()` / `declined()` instead of callers re-deriving plan equality. Call sites in `chain.py`, the polars chain, both row pipelines, and `frame_ops` are now typed calls. - `_integer_index` takes the index union and narrows with `isinstance` rather than probing `getattr(..., "keys_sorted", None)`; alias/filter access narrows on `ASTNode`/`ASTEdge` instead of `getattr(op, "_name", None)`. - Added `getattr`/`setattr` across the stack: 31 -> 19, of which 5 are inside the one module that owns the attribute; the rest are `threading.local` probes (the correct idiom) and pre-existing `_gfql_rows_*` plumbing this stack did not invent. Added `# type: ignore`: 18 -> 9, and `mypy --warn-unused-ignores` reports none of the remaining ones is unnecessary. Correctness fix found by the new tests - `_connected_decline_reason` re-derived the cost gate locally, so on engines with a tighter crossover (0.02 vs pandas' 0.5) a structurally-unsupported shape was reported as `cost_frontier`. It now asks the real gate with cost disabled: a serve there means cost was the only obstacle, anything else is structural. One source of truth, accurate on every engine. Tests - The unsupported-shape decline matrix now runs on pandas/polars/cuDF (it was pandas-only) — that is what caught the mislabelling above. - New negative matrix for the polars early gate: rows-with-source, alias-endpoints, alias-prefilters, and seeded re-entry must not engage, plus a plan-mismatch and unnamed-middle case, and a positive assertion so the suite cannot go vacuous. - Property-index cost gate is now exercised under `use` (every other case used `force`, which skips the gate) with the crossover pinned via the public knob. - Property index reachable from all three surfaces: Cypher DDL grammar accepts `node_prop`, and the JSON wire protocol handles column-keyed create/drop/name resolution (it previously raised "no resident index" for a resident property index and dropped every column on a targeted drop). - `docs/source/gfql/indexing.rst` documents the new kind and `gfql_index_node_props`. Validated on dgx-spark: full CPU `graphistry/tests/compute` 6260 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 70 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 25, 2026
| def with_index_policy(g: Plottable, policy: IndexPolicy) -> Plottable: | ||
| """A copy of ``g`` carrying ``policy`` (the attribute lives only in this module).""" | ||
| out = g.bind() | ||
| setattr(out, POLICY_ATTR, policy) |
Contributor
Author
There was a problem hiding this comment.
revisit for static typing..
lmeyerov
commented
Jul 25, 2026
| try: | ||
| g = create_index(g, NODE_PROP, column=column, engine=engine) | ||
| except ValueError: | ||
| pass |
Contributor
Author
There was a problem hiding this comment.
avoid or make more managed
… suppression Two follow-up review nits. **Declared fields instead of smuggled attributes.** `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, and every reader had to remember the default. The family is now DECLARED on `Plottable` with its 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 because `_RowPipelineAdapter` is not a `PlotterBase`, and the `RowPipelineCtx` protocol declares the three it reads. Call sites in this stack's files then become ordinary attribute access: `index/handoff.py` drops to ZERO dynamic attribute calls, and `api.py`'s policy and registry accessors, both chain boundaries, both row pipelines, and `frame_ops` stop using `getattr`/`setattr` for this family. That immediately surfaced a latent bug: `_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). Under `getattr(..., None)` it read as "absent"; under typed access it raised, which is the point. It now constructs the full context. EXCLUDED deliberately: `_gfql_native_sp_cache` (a different subsystem's cache, no call site in this stack) and the repo-wide sweep of the same pattern outside these files — both are separate changes, not this PR's scope. **Narrowed suppression.** `gfql_index_node_props` wrapped `create_index` in `except ValueError`, which swallowed caller mistakes too — a typo'd column name silently left the query unindexed. `create_index` now raises the purpose-built `GfqlIndexUnsupportedError` (a `ValueError` subclass, so existing handlers keep working) for exactly the two conditions the DATA cannot support: duplicate node ids and an unindexable property dtype. The convenience builders catch only that; everything else propagates. `gfql_index_all` gets the same treatment for `node_id`. Tests pin both directions: unindexable dtypes are skipped, a missing column raises. Validated: full CPU `graphistry/tests/compute` 6260 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 70 passed; ruff clean; mypy unchanged vs master. An intermediate run caught 63 failures from the `_SyntheticRowGraph` gap and 3 mypy `no-redef`s from duplicated mixin annotations — both fixed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1
…-property-index # Conflicts: # graphistry/compute/gfql/index/api.py
Follow-up review asked whether every field-access site was handled. It was not — 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. More important, the same audit found the `_SyntheticRowGraph` bug class was NOT fully closed: both hand-rolled Plottable stand-ins — that class and `_RowPipelineAdapter` (via `RowPipelineMixin`) — were missing `_gfql_index_registry`, which `get_registry` now reads directly. Reachable: `bindings.py` calls `get_registry(base_graph)` and the row pipeline's `base_graph` can be the adapter. Both now carry it, so typed access is total for every object that flows through these paths. Also annotated the defs this stack added that were bare: the polars `_try_indexed_middle_polars` gate (now `-> Tuple[Optional[IndexedBindingsState], bool]`) and the two `ComputeMixin` index wrappers. ComputeMixin is only ~30% annotated, so matching the local style would have been defensible, but new public surface should not add to that debt. Validated: full CPU `graphistry/tests/compute` 6260 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 70 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
… decline Three more review points on the files this stack touches. **`row/frame_ops.py` declared its context protocol as `Any`.** The `RowPipelineCtx` fields are now typed — `Optional[DataFrameT]` for the frames, `Optional[str]` for the id bindings, `Optional[Plottable]` for the base graph and the adapter's `_g` back-reference, `Optional[Iterable[str]]` for the edge aliases — via a `TYPE_CHECKING` import of `DataFrameT`. Three polars-on-`DataFrameT` calls that the sharper types expose take localized `# type: ignore`s, which is the documented convention for engine-polymorphic frames here. **`row/frame_ops.py` still used `getattr`.** All of them are gone: `_g` is now a declared field (defaulted on `PlotterBase`, set only by adapters) and the two `getattr(base_graph, "_nodes"/"_edges", None)` — pre-existing, but pointless once the fields are declared — are direct access. That file now 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 is specific 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 being converted into a silent slow-path fallback. The finisher now takes `decline_on_schema_error`; the indexed caller passes `False` and lets it raise. On the `Any`s in that same file: `ops` is now `Sequence[ASTObject]` and the alias narrowing is real `isinstance` rather than `getattr`, 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. The comment says so. 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 Follow-up review: 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]`). Note `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 now a CONSTRAINED TypeVar over polars eager/lazy frames rather than `Any`. A plain union does not work here — `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 for engine-polymorphic frames. 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 have broken 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
lmeyerov
changed the base branch from
perf/gfql-general-indexed-bindings
to
master
July 26, 2026 02:04
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.
Stacked on #1776 (base branch
perf/gfql-general-indexed-bindings) — review that one first.Why
With #1776 landed, official LDBC SNB SF1 interactive-short IS7 was 72 ms against Kuzu 36.7 / Ladybug 36.0. Phase timing (plain
perf_counter, not cProfile) located the whole gap precisely:_filter_frameand per-call shapes showed a single offender:
{id, label__Message}{type: REPLY_OF}{label__Comment}{type: HAS_CREATOR}{label__Person}The traversal is already optimal. We were scanning 3.2M rows to find one seed node, because the graph's node id is bound to a synthetic key while the query seeds on the
idproperty — and the registry had no index for properties. That's the secondary-index capability other engines have; this PR adds it.What
NodePropIndex— sorted distinct values over node row positions, CSR form so duplicate values are indexable (unlike the unique-only node-id index). Never reorders.nodes; identity + fingerprint validated, so a.nodes()rebind is a safe miss; engine-polymorphic (numpy host / cupy on-device). Kept in a column-keyed map beside the kind-keyed one.build_node_prop_indexdeclines (→ scan) for anything whose vectorized ordering/equality isn't unambiguous on both backends — today integer dtypes only (float NaN ordering, cupy strings, nulls). Widening is additive.lookup_prop_rows(searchsorted + CSR range expansion) andprop_match_count, a free selectivity estimate read straight off the CSR offsets.create_index('node_prop', column=...)(column required),drop_index('node_prop', column=...),show_indexes()rows,g.gfql_index_node_props([...])(skips unindexable columns).Opt-in, exactly like other engines' secondary indexes: with no property index resident, behaviour and cost are unchanged.
Perf (dgx-spark, official LDBC SNB SF1, warm medians, value-identical 19 rows)
That puts IS7 ~1.9× ahead of the frozen Kuzu (36.65 ms) and Ladybug (36.0 ms) reference points, from ~2× behind.
A position-balanced A/B of IS1–IS7 with no property index resident shows no detectable change — per-query deltas sit inside each build's own 6–10% run-to-run spread, and with nothing indexed the added path is an empty-tuple check.
Tests
Standard-derived, all against the same-engine canonical path: seed-without-scanning parity plus candidate width on pandas/polars/cuDF; indexed-vs-absent equality; duplicate-value keys gathering every matching row; stale-rebind and policy-off fallback; unindexable dtype / missing column / missing argument errors alongside the skipping convenience wrapper; most-selective-column choice; show/drop lifecycle.
Validation (dgx-spark, capped guard, read-only source)
graphistry/tests/compute: 6242 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 of the stack raised DRY/placement, functional-style, dynamic-typing, and test-amplification concerns. Addressed in
189dcc73(this PR) and83212ab1(on #1776):DRY / placement —
bindings._concatduplicatedEngine.df_concat(whose polars twin already acceptsignore_indexfor exactly this) and is deleted;_estimate_join_rows/_join_state/_filter_oriented_endpointsmove tocompute/dataframe/join.pyasestimate_inner_join_rows/path_ordered_expand_join/semijoin_by_column, generalized to explicit column parameters;_lookup_degreemoves toindex/lookup.pyand now shares one CSR probe with the property lookup (three copies of searchsorted+membership became_csr_hit_positions/csr_match_count/csr_gather_rows).bindings.pydrops from 17 local helpers to 12, all index-specific.Functional style — copy-then-mutate became a single batched
.assign(...)throughout, plus a redundant.copy()dropped before.rename()and after a boolean mask. Rule written down in the review skill.Dynamic typing — typed handoff (see #1776),
_integer_indexnarrows withisinstanceon the index union instead of probinggetattr(..., "keys_sorted", None), alias/filter access narrows onASTNode/ASTEdge. Across the stack:getattr/setattr31 → 19 (5 of those inside the one module that owns the attribute; the rest arethreading.localprobes and pre-existing_gfql_rows_*plumbing),# type: ignore18 → 9, andmypy --warn-unused-ignoresreports none of the remaining ones unnecessary.A correctness fix the new tests caught —
_connected_decline_reasonre-derived the cost gate locally, so on engines with a tighter crossover (0.02 vs pandas' 0.5) a structurally unsupported shape was reported ascost_frontier. It now asks the real gate with cost disabled: a serve there means cost was the only obstacle, anything else is structural. One source of truth, accurate on every engine.Tests — the unsupported-shape decline matrix now runs pandas/polars/cuDF (was pandas-only; that is what caught the mislabelling); a negative matrix for the polars early gate (rows-with-source, alias-endpoints, alias-prefilters, seeded re-entry, plan mismatch, unnamed middle) plus a positive assertion so it cannot go vacuous; the property-index cost gate is now exercised under
usewith the crossover pinned via the public knob (every other case usedforce, which skips the gate); and the property index is now reachable from all three surfaces — the Cypher DDL grammar acceptsnode_prop, and the JSON wire protocol handles column-keyed create/drop/name resolution (it previously raised "no resident index" for a resident property index and dropped every column on a targeted drop).docs/source/gfql/indexing.rstdocuments the new kind.Verified: full CPU
graphistry/tests/compute6260 passed with only master's pre-existingtest_chain_dask_edges; cuDF lane 70 passed; ruff clean; mypy unchanged vs master.Not in this PR: benchmark scaffolding stays out of this repo — the reusable IS1–IS7 A/B suite config and the phase-timing probe went to pyg-bench (
bench/gfql-is1-is7-ab-harness). Engine-shape divergences found along the way are filed as #1779.Review follow-ups, round 2
Two further nits, both about hiding things the type system should carry:
7777e191, on perf(gfql): dispatch indexed fixed-hop bindings before the canonical traversal #1776):with_index_policyno longer doessetattr(out, POLICY_ATTR, policy)— the whole_gfql_*execution context is declared onPlottable/PlotterBase, soindex/handoff.pyhas zero dynamic attribute calls and the policy/registry accessors are plain typed access. Two latent bugs surfaced doing it (a Plottable stand-in missing fields; agetattrfallback silently disabled by the new default) — details in perf(gfql): dispatch indexed fixed-hop bindings before the canonical traversal #1776.d85b421f, here):gfql_index_node_propswrappedcreate_indexinexcept ValueError, swallowing caller mistakes — a typo'd column name silently left the query unindexed.create_indexnow raises the purpose-builtGfqlIndexUnsupportedError(aValueErrorsubclass, so existing handlers keep working) for exactly the two conditions the DATA cannot support: duplicate node ids and an unindexable property dtype. The convenience builders catch only that; everything else propagates.gfql_index_allgets the same treatment fornode_id. Tests pin both directions — unindexable dtypes are skipped, a missing column raises, and the raised type is asserted to not be the skippable one.Full-stack verification from a pristine master clone: CPU 6260 passed (only master's pre-existing
test_chain_dask_edges), cuDF 70 passed, ruff clean, mypy unchanged vs master, andmypy --warn-unused-ignoresreports no unnecessary suppression from this stack (the 7 it does flag exist verbatim on master).Review follow-ups, round 3
Three more nits, all on files owned by #1776 and fixed there (
54d6e77f,6c41ee8e), then merged forward:row/frame_ops.py'sRowPipelineCtxprotocol declared its fieldsAny→ nowDataFrameT/str/Plottable/Iterable[str]via aTYPE_CHECKINGimport, and the file has zerogetattr.polars/row_pipeline.pyswallowed unexpected exceptions — the extracted finisher made the indexed path share the generic path'sexcept SchemaError: return None. It now takesdecline_on_schema_error, so only the generic caller may decline; on indexed state (whose dtypes our helper already verified) aSchemaErrorraises._gfql_index_registry, whichget_registryreads directly.Full-stack verification from a pristine master clone: CPU 6260 passed (only master's pre-existing
test_chain_dask_edges), cuDF 70 passed, ruff clean, mypy unchanged vs master.