perf(gfql): evaluate indexed edge_match on candidate rows, not the whole edge frame - #1782
Conversation
…ole edge frame The index path built the simple-equality `edge_match` mask over ALL E edges and then read it only at `rows[edge_keep[rows]]` -- the handful of positions the CSR adjacency lookup returned. That put an O(E) predicate scan inside an O(degree) traversal, so the indexed path scaled with the graph instead of with the answer. Measured on a 3.18M-node / 14M-edge polars graph (LDBC SNB SF1-shaped), the single `(series == val)` compare was 248ms of a 308ms seeded typed-edge query -- 81% of it, and the reason the indexed surface scaled while the native hop stayed flat. `_build_edge_row_filter` now does the schema-level validation up front (the dtype-mismatch decline that keeps error parity with the scan is O(1) and unchanged), and `_EdgeMatchRowFilter.mask_for(rows)` evaluates `col == val` on the gathered candidate rows. It never examines more elements in total than the eager mask did, so this is strictly less work rather than a tradeoff. A failure mid-traversal abandons the indexed path entirely, so the scan fallback stays parity-safe. Surface query: 309.92 -> 57.44 ms (5.40x), value-identical. Parity: 650 differential comparisons (pandas + polars x 13 shapes x 25 random seeds, dense graph, null-carrying string and numeric columns) -- 0 divergences, comparing columns, dtypes, row counts and full content, including the decline paths. Two new regression tests pin the shape rather than a wall-clock number: the predicate must see only candidate rows, and growing the graph 8x at fixed degree must not grow the number of rows it examines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VARwnAqmvczhz5EmP7TYyZ
… typing CHANGELOG entry for the O(E)->O(degree) edge_match fix, plus review follow-ups on the implementation itself: - type the filter's state properly (Dict[str, SeriesT] / List[Tuple[str, Any]] and SeriesT on the gather) instead of bare `dict`/`list`/`Any`, per the repo's engine-agnostic typing convention. - correct an overclaim in the docstring: "never examines more elements than the eager mask" is false for a fixed-point UNDIRECTED walk, where the out- and in-indices are filtered separately and the total approaches 2E against the eager form's E. Stated accurately now, with the reason it is still a constant factor on an already-O(E) traversal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VARwnAqmvczhz5EmP7TYyZ
Correction to the mechanism claim in the descriptionSelf-review found a method error in my own profiling. Flagging it rather than leaving the description overstated. What still holds — the load-bearing evidence is unaffected. The SF1 numbers in the description came from the real What does not hold — the stated mechanism. The profile I cited ( So: the change is a genuine O(E)→O(candidates) improvement, and it demonstrably helps the polars lane by ~29% on IS1. But I have not demonstrated that the object-array compare is the mechanism on the polars path, and the description reads as though I had. Treat the 5.40× as pandas-path only. Unrelated finding worth its own issue (#1767 territory): on the same graph, I'd rather this PR be judged on the lane A/B than on a mechanism story I got to via a misconfigured probe. |
Typing-only; no behaviour change. Addresses the review note that
index/traverse.py leaned on dynamic typing where static types were
available.
- _EdgeMatchRowFilter: annotate the __slots__ members (_series, _items,
_engine) instead of leaving them implicitly typed.
- Replace the bare `Any` value type in the match items with the real
domain type: List[Tuple[str, ScalarMatchValue]]. This is now genuinely
checked -- the is_simple_equality_edge_match TypeGuard narrows
EdgeMatch -> SimpleEqualityEdgeMatch, so a non-scalar leaking into the
items list is a type error rather than an Any hole.
- Drop the cast(ArrayLike, ...) / cast(Any, ...) call-site casts in
mask_for(); declare col_mask: ArrayLike (and col_series: SeriesT in
_build_edge_row_filter) and let the engine-agnostic aliases carry the
type, matching the convention used elsewhere under index/.
- The mask combine was previously cast(ArrayLike, cast(Any, mask) &
cast(Any, col_mask)) purely because the ArrayLike protocol had no
bitwise surface. Declare __and__/__rand__ on the protocol (alongside
the existing __invert__/__add__/__radd__ pair convention) so
`mask & col_mask` type-checks as itself. Verified load-bearing: without
it mypy reports `Unsupported left operand type for & ("ArrayLike")`.
ArrayLike is not runtime_checkable and is never isinstance-tested, so
this is a checker-only change.
`Any` is no longer imported in traverse.py.
Checks: ./bin/typecheck.sh (mypy 2.3.0, 323 files, clean) and
./bin/lint.sh (ruff, clean). graphistry/tests/compute -k "not cudf and
not gpu" gives an identical 36 failed / 6192 passed before and after,
with a byte-identical FAILED set (pre-existing optional-dep failures);
the gfql/index suite is 176/176 green. cudf lanes are unrunnable here
(no libnvrtc on this host).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VARwnAqmvczhz5EmP7TYyZ
Review of this PR measured a real REGRESSION that the CHANGELOG had claimed was
impossible ("strictly less work, not a tradeoff"). Candidate-row evaluation beats
one whole-column compare only while candidates stay a small fraction of the frame.
A fixed-point walk that reaches most of the graph inverts that: out- and in-indices
are filtered separately, so it gathers up to 2E elements by RANDOM ACCESS against
the eager form's single sequential pass over E.
Measured before this commit, pandas, to_fixed_point + undirected + edge_match:
int etype, E=100k: 1.94xE gathered, +20% vs master
str etype, E=400k: 1.56x SLOWER than master
Reachable from Cypher `-[:KNOWS*]->` — `_hop_is_index_coverable` allows
to_fixed_point with edge_match — so this was not a synthetic-only shape.
Fix: the hop keeps a cumulative gathered-row count and, once the NEXT batch would
push it past E/8, builds the whole-column mask once (`full_mask()`) and reuses it
for the rest of the traversal. The check is made before gathering, not after, so a
single large batch cannot overshoot; total predicate work is bounded at ~1.125xE.
A seeded hop gathers ~degree and never approaches the threshold, so it never pays
for the mask. `full_mask()` reuses mask_for's null-fill rule verbatim, which is
what makes the two forms agree cell-for-cell across the switch.
After the guard, same shapes vs master 84be35f (edge counts identical):
int etype, E=100k: 13.12 -> 14.11 ms (+7.5%, was +20%)
str etype, E=400k: 77.47 -> 80.97 ms (+4.5%, was 1.56x)
Also corrects two false statements this PR shipped:
- CHANGELOG "strictly less work, not a tradeoff" -> states the tradeoff, the
measured regression, and the bound.
- the class docstring claimed each row is returned at most once because frontiers
are set-differenced against `visited`. edge_match is only reachable with
return_as_wave_front=True, which SKIPS the first-hop `visited` seeding, so seed
ids can re-enter a later frontier and be gathered twice.
- CHANGELOG now records the honest limit on error parity: only the O(1) dtype
gates are parity-exact; a data-dependent comparison failure is seen only if it
lands in the gathered candidates, so the indexed path can succeed where the
scan raises.
Tests: the mid-traversal abandonment branch (previously untested and
`# pragma: no cover`) now has a negative test asserting the fallback reproduces the
scan result exactly; and a fixed-point test pins that total gathered stays within
the guard's bound while the answer is unchanged. Both engines. 180 CPU index tests
pass, ruff and mypy clean.
Review response — one confirmed regression fixed, two false claims correctedRan The defectI wrote: "This never examines more elements in total than the eager mask did — it is strictly Measured, pandas,
Instrumented: 1.94×E elements gathered. And it is publicly reachable, not a synthetic The fix (
|
| shape | master | guarded | was |
|---|---|---|---|
| fixedpoint int, E=100k | 13.12 ms | 14.11 ms (+7.5%) | +20% |
| fixedpoint str, E=400k | 77.47 ms | 80.97 ms (+4.5%) | 1.56× |
The residual few percent is the designed cost of gathering up to E/8 before switching.
Also corrected
- The docstring claimed each row is returned at most once "because frontiers are
set-differenced againstvisited".edge_matchis only reachable with
return_as_wave_front=True, which skips the first-hopvisitedseeding — so seed ids
can re-enter a later frontier and be gathered twice. - Error parity is narrower than I claimed. Only the O(1) dtype gates are parity-exact. A
data-dependent comparison failure (e.g. a list-valued cell in an object column outside the
seed's neighborhood) is now observed only if it lands in the gathered candidates, so the
indexed path can succeed where the scan raises — reproduced. Now stated in the CHANGELOG.
The converse is safe: forcing a mid-traversal failure abandons the indexed path and returns
byte-identical results to the scan.
Tests added
- The mid-traversal abandonment branch — the one new branch, which carried all the parity risk
and was previously untested and# pragma: no cover— now has a negative test asserting the
fallback reproduces the scan result exactly. - A fixed-point test pinning that total gathered stays within the guard's bound while the
answer is unchanged.
Typing (51219e28)
Typed slots; Any → ScalarMatchValue (the existing index/types.py alias, genuinely checked
via the is_simple_equality_edge_match TypeGuard); every cast(ArrayLike/Any, …) call-site
cast removed. That required one real fix: the ArrayLike protocol had no bitwise surface, so
mask & col_mask needed the cast sandwich — __and__/__rand__ are now declared next to the
existing __invert__/__add__ pairs. Verified load-bearing (stash it and mypy errors).
mypy: 323 files clean. Full compute suite A/B: identical failure sets.
Still open
- DRY:
index/bindings.pyalready has a canonical candidate-row filter that delegates to
filter_by_dict(parity-exact by construction);traverse.pystill hand-rolls a second one.
Not addressed here. - cuDF remains unverified — no GPU on this box, and the GB10 25.02 image faults. The tests
that would close it already exist and just need a GPU:
test_chain_index_parity_vs_scan[chain0..3-cudf]/[…-polars-gpu]. A third, GPU-specific
concern is unmeasured: per-hop kernel-launch cost points the opposite way from the CPU win. - pyg-bench regression lane for this fix — in progress, not yet run.
| # keeps small frames on the candidate-row path, where the whole-column compare is cheap | ||
| # anyway and the switch would only add a branch. | ||
| _EAGER_MASK_SWITCH_DIVISOR = 8 | ||
| _EAGER_MASK_SWITCH_FLOOR = 1024 |
There was a problem hiding this comment.
should these be extern'd?
| - **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`. | ||
|
|
||
| ### Performance | ||
| - **GFQL indexed typed-edge traversals stop scanning the whole edge frame (#1658)**: the index path built the simple-equality `edge_match` mask over ALL `E` edges — one `(series == val)` over the full column — and then read that mask only at `rows[edge_keep[rows]]`, the handful of positions the CSR adjacency lookup had already returned. That put an **O(E) predicate scan inside an O(degree) traversal**, which is why an indexed seeded typed hop scaled with the graph while the underlying `g.hop()` stayed flat. The predicate is now evaluated on the gathered candidate rows instead: `_build_edge_row_filter` does the schema-level validation up front (the dtype-mismatch decline that keeps error parity with the scan is O(1) and unchanged) and `_EdgeMatchRowFilter.mask_for(rows)` applies `col == val` to just those rows, using each frame's native `==` exactly as before (so cuDF string columns stay on the cuDF layer). This is a cost tradeoff, not a free win, and it is bounded rather than assumed: candidate-row evaluation beats one whole-column compare only while the candidates stay a small fraction of the frame, and a fixed-point walk that reaches most of the graph inverts it — the out- and in-indices are filtered separately, so it can gather up to 2E elements by random access against the eager form's single sequential pass over E (measured before the guard: an undirected `to_fixed_point` typed walk gathered 1.94×E and ran 1.2–1.6× SLOWER than master). A cumulative-cost guard now switches to the whole-column mask once gathered rows reach E/8, so total predicate work is bounded at ~1.125×E in that regime while a seeded hop — which gathers ~degree — never approaches the threshold and never builds it. An evaluation failure mid-traversal abandons the indexed path entirely so the scan fallback stays parity-safe. Measured on a 3.18M-node / 14M-edge polars graph (LDBC SNB SF1-shaped), a seeded typed-edge query with alias markers goes **309.9 → 57.4 ms (5.4×)**, value-identical, with the full-column compare (previously 248 ms, 81% of the query) gone from the profile. Parity held across 650 differential comparisons — pandas + polars × 13 traversal shapes × 25 random seeds on a dense graph with null-carrying string and numeric columns — comparing columns, dtypes, row counts and full content, including the decline paths (membership `edge_match` still routes to the scan; a dtype mismatch still raises the same `GFQLSchemaError`). One honest limit on error parity: only the O(1) dtype gates are parity-exact. A *data-dependent* comparison failure (e.g. a list-valued cell in an object column) is now observed only if it lands in the gathered candidates, so the indexed path can succeed where the scan raises. That needs pathological cell values to reach, but it is a real narrowing of the previous guarantee. Pinned by two structural regression tests that assert the *shape* rather than a wall-clock number: the predicate must see only candidate rows, and growing the graph 8× at fixed degree must not grow the number of rows it examines. |
There was a problem hiding this comment.
once benefit proven & locked in pyg-bench, and happy with postive/negative tests on either side of opt boundary, and ci still green, can land both sides
Answers the review question on the guard constants ("should these be extern'd?")
by repo precedent rather than taste:
- `GFQL_LEAN_COMBINE=0` exists so the differential parity harness can force the
legacy path -> a BEHAVIOUR boundary gets an env switch.
- `_LEAN_SHRINK_RATIO` is a private module constant -> a numeric TUNING threshold
does not.
So `GFQL_INDEX_CANDIDATE_EDGE_MASK=0` now forces the whole-column mask on every hop,
while _EAGER_MASK_SWITCH_DIVISOR/_FLOOR stay private. They are a cost heuristic, not
an interface, and the guard already bounds the bad case.
This also supplies what the landing bar asks for — positive AND negative tests on
either side of the opt boundary:
- test_both_sides_of_the_edge_mask_cost_boundary_agree: forces each side and
compares, with the UNINDEXED scan as an independent third opinion so a shared
bug in both forms cannot hide. 4 shapes x 2 CPU engines.
- test_forcing_the_whole_column_mask_actually_changes_the_path: asserts the OFF
side really stops gathering candidate rows — without this the test above could
be comparing the candidate-row form against itself and proving nothing.
Records a PRE-EXISTING divergence rather than encoding it as expected: for an
undirected to_fixed_point wavefront hop the indexed path keeps the SEED in `_nodes`
while the scan drops it when the walk never returns to it (edges are identical).
Reproduced on master 84be35f, so it is NOT from this PR, but it does violate the
"identical whether the index is present or absent" contract. The test asserts the
indexed result is a superset of the scan's and that any excess is exactly the seed.
|
| probe | pins | master | #1782 | #1783 | #1784 | #1785 |
|---|---|---|---|---|---|---|
indexed-typed-edge-match-hop[polars] |
#1782 | regression | pass | pass | pass | pass |
empty-left-typed-edge-miss[pandas] |
#1783 | regression | regression | pass | pass | pass |
polars-edge-order-materializations |
#1783 | regression | regression | pass | pass | pass |
semi-join-key-dedup-node-ladder[polars] |
#1784 | regression | regression | regression | pass | pass |
indexed-typed-edge-match-hop[pandas] |
#1782 | pass | pass | pass | pass | pass |
A monotone staircase: each PR flips exactly its own probes, nothing regresses, and #1785
holds all five. Growth over an 8× ladder:
indexed-typed-edge-match-hop[polars]: master 2.97× (2.55 → 7.59 ms) → perf(gfql): evaluate indexed edge_match on candidate rows, not the whole edge frame #1782 0.93×
(0.28 → 0.26 ms), i.e. flat.empty-left-typed-edge-miss: master 2.55× (9.20 → 23.45 ms) → perf(gfql): empty-left merge shrink + polars EORD/EID row-index dedup #1783 0.998×
(6.67 → 6.66 ms), i.e. flat.
Note the config-level exit code is cumulative — it is the stack gate, so on its own it
reads as "master/#1782/#1783 fail". The per-probe verdicts above are what [1] actually asks
for.
One probe does not discriminate — flagging rather than burying
indexed-typed-edge-match-hop[pandas] reports pass on master. It is not blind — it
measures 5.294× growth there. It passes because the verdict ANDs the ratio gate with a
5.0 ms min_absolute_delta_ms floor, and that variant's absolute delta at this ladder size is
only 2.11 ms. So the floor, which exists to stop flaky sub-noise failures, is suppressing a
real 5.3× growth.
The [polars] variant is what actually pins #1782; the pandas one currently rides along.
It should be fixed by raising base_edges for that probe until the delta clears the floor, or
by giving it a smaller per-probe floor — not by deleting it. It is one config change away from
being load-bearing, and it is a live instance of exactly the failure mode the lane's own README
warns about.
Two run traps worth recording
uv is not on the non-interactive ssh PATH (exit 127 — use $HOME/.local/bin/uv), and the
pyg-bench uv env has no polars (--with polars). Both produced silent-looking failures first.
Ready to merge — all three bars met
Beyond the bar
Competitive resultSecond flip, and this one needs no asterisk: IS5 Four weak tests of my own that mutation-checking caught, now fixed
Known limits, stated rather than buried
Not merging per your instruction — this is yours to land. #1785 stays draft pending its |
…tly scoped
The stack's own regression tests are all `parametrize("engine", _cpu_engines())`
— pandas + polars — so the Engine.CUDF and POLARS_GPU branches had NO coverage
from them. Switching those to `_engines()` is not the fix: that helper includes
cudf whenever cudf is IMPORTABLE, so on any box with cudf installed but no CUDA
runtime every such test fails (verified: 12 failures locally).
So this is a separate, properly gated file. The gate is the operation itself —
a tiny indexed typed hop on cudf — because every cheaper probe fails to
discriminate on a half-installed box: `cudf.DataFrame(...)`, `.to_pandas()`,
`.values` (a small cupy alloc), `==`, and even `groupby().sum()` all SUCCEED
there and the suite then dies with `OSError: libnvrtc.so.12` in the first real
kernel.
Verified BOTH ways, which is the point of a gated file:
local (no CUDA runtime): 12 skipped
dgx-spark, graphistry/test-rapids-official:26.02-gfql-polars --gpus all: 12 passed
(`--gpus all` is required; without it the 26.02 runtime reports
cudaErrorInsufficientDriver and the file would skip everywhere — a silently
always-skipping test file being strictly worse than none.)
Covers, against the PANDAS oracle: null-bearing edge predicate columns across
int64/Int64/float/string/boolean on cudf and polars-gpu, and the empty candidate
batch on device.
Discloses what it does NOT pin, mutation-checked: deleting the `fillna(False)`
before `.values` in the cudf branch leaves all 12 GREEN on cudf 26.02, because
`(sub == val)` there already yields a non-null boolean column. The fillna is
defensive on this version, not load-bearing; the docstring says so rather than
letting a green run imply otherwise.
1 participant
perf(gfql): evaluate indexed
edge_matchon candidate rows, not the whole edge frameThe bug
index_seeded_hopbuilt the simple-equalityedge_matchmask over all E edges:The mask is only ever read at
rows— the handful of positions the CSR adjacencylookup already returned. So a full-column predicate scan sat inside an O(degree)
traversal, and the indexed path scaled with the graph instead of with the answer.
This is the mechanism behind a long-standing puzzle in our LDBC work: the native
g.hop()is flat in graph size while the indexed surface query is not. It is thesame code either way — the difference is that the surface query carries a typed edge.
Evidence it is the dominant cost
Profiling a seeded typed-edge query on a 3.18M-node / 14M-edge polars graph
(LDBC SNB SF1-shaped), master
84be35fb:Caller chain (spy on the pandas comparison op):
The fix
_build_edge_row_filterdoes the schema-level validation up front — including thedtype-mismatch decline that keeps error parity with the scan, which is O(1) and
unchanged — and
_EdgeMatchRowFilter.mask_for(rows)appliescol == valto thegathered candidate rows only, using each frame's native
==exactly as before (socuDF string columns stay on the cuDF layer).
Each edge row is returned by a given index at most once (frontiers are
set-differenced against
visited), so a seeded hop examines O(edges traversed)elements. The worst case is a fixed-point undirected walk reaching the whole graph,
where the out- and in-indices are filtered separately and the total approaches 2E
against the eager form's E — a constant factor on a query already O(E) in its
traversal alone.
A failure mid-traversal abandons the indexed path entirely (
return None), so thescan fallback stays parity-safe; nothing observable has been mutated at that point.
Correctness
shapes × 25 random seeds, on a dense graph with null-carrying string and
numeric columns. Compares columns, dtypes, row count and full content against the
same query with the index disabled.
two-column
edge_match, numeric-only, untyped,+rows(source=), and both DECLINEpaths (membership
edge_matchstays on the scan; dtype mismatch still raises thesame
GFQLSchemaError).graphistry/tests/compute/gfql/index: identical failure sets before and after(38 failed / 169 passed on both, diff-clean — all pre-existing GPU-image failures
on this host).
Flagged, not claimed.
Tests
Two new regression tests (× 2 CPU engines) pin the shape, not a wall-clock number,
so they cannot go flaky on a loaded host:
test_typed_edge_predicate_only_reads_candidate_rows— the predicate must see farfewer rows than the edge count.
test_typed_edge_predicate_cost_flat_in_graph_size— growing the graph 8× at fixeddegree must not grow the rows the predicate examines.
Not a benchmark hack
Engagement keys on a structural precondition — an index-seeded hop carrying a
simple-equality edge predicate — and helps any typed traversal on any schema, engine,
or label. No query, constant, or dataset is recognized anywhere in the change.
Measured on the real LDBC SNB SF1 lane
Position-balanced ABBA/BAAB (4 samples per build, each build in each slot position
exactly once — dgx-spark has a ~5% slot effect that makes naive A,B,A,B
uninterpretable), quiet host, host-wide perf lock held for the whole experiment,
rows_returnedvalidated identical on every query. Pooled mean of per-slot medians:84be35fb4 faster, 0 slower, every result value-identical.
(IS6 fails and IS3 is a primitive gap on this lane on both builds — pre-existing,
not evidence either way.)
What this does NOT do
It does not flip any competitor result. IS1 was ~77× behind Kuzu and is now ~55×.
The remaining dominant cost on IS1/IS5 is a different one, and the run artifacts
name it:
index_engagementreportsnode_prop: built_seam_never_reached, i.e.MATCH (n:Person {id: $personId})still full-scans 3.18M nodes because the propertyindex only reaches the connected-bindings seam (#1780). That is the next fix, not
this one.