Skip to content

Commit 4061e7c

Browse files
lmeyerovclaude
andauthored
perf(gfql): evaluate indexed edge_match on candidate rows, not the whole edge frame (#1782)
* perf(gfql): evaluate indexed edge_match on candidate rows, not the whole 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 * docs(changelog): indexed edge_match candidate-row evaluation; tighten 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 * refactor(gfql): static typing for the indexed edge_match row filter 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 * perf(gfql): bound candidate-row edge_match with a cumulative-cost guard 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. * feat(gfql): make the edge_match cost boundary externally switchable 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. * test(gfql/index): GPU coverage for the indexed edge_match path, honestly 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. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 84be35f commit 4061e7c

5 files changed

Lines changed: 618 additions & 40 deletions

File tree

CHANGELOG.md

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

1414
### Performance
15+
- **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.
1516
- **GFQL indexed fixed-hop bindings now dispatch BEFORE the canonical traversal (pandas / cuDF / native Polars)**: the resident-index fixed-hop path (`rows(binding_ops=...)`, the Cypher multi-alias `MATCH … RETURN` shape) was consulted only *inside* row materialization — after the engine had already run the full canonical traversal — so its compact path bag was computed on top of the graph-sized work it exists to avoid. Both the pandas/cuDF chain boundary and the native Polars chain now ask the same shared, structural gate first and, only when it can serve the WHOLE middle exactly, skip the canonical traversal and hand the compact state to the unchanged row materializer. Engagement stays operator/index/dtype/cost based — no query, schema, or hop-count recognition — and every unsupported, seeded, prefiltered, policy-bearing, shortest-path, or cost-gated shape still declines to canonical execution with identical results. On a standard LDBC SNB SF1 interactive-short profile this removed the redundant two-hop typed-mask and 16-merge buckets and cut the profiled call ~11.9× (1263 ms → 106 ms), exact 19-row oracle preserved.
1617
- **Indexed seed lookup now covers a unique node id PLUS extra scalar constraints**: `{node_id: v, other: w}` previously fell back to a full node-table scan even though the unique node-id index can gather at most one row; it now gathers that one row and applies the remaining constraints to it. Missing ids and non-matching extra constraints return the same empty result as before, and duplicate-id graphs (which cannot build the index) are unaffected.
1718
- **Seeded typed-hop property projection keeps its fast path through DISTINCT / ORDER BY / SKIP / LIMIT**: those trailing row ops are plain frame operations, so the fast path now delegates them to the canonical chain instead of declining the whole query.

0 commit comments

Comments
 (0)