Skip to content

perf(gfql): SNB point-query levers — index-hit verify, numpy hop tail, row-call trims, no edge-index scaffold for row calls - #2070

Open
lmeyerov wants to merge 9 commits into
masterfrom
perf/gfql-native-seed-skip-refilter
Open

perf(gfql): SNB point-query levers — index-hit verify, numpy hop tail, row-call trims, no edge-index scaffold for row calls#2070
lmeyerov wants to merge 9 commits into
masterfrom
perf/gfql-native-seed-skip-refilter

Conversation

@lmeyerov

@lmeyerov lmeyerov commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Objective

Indexed seed lookup, message creator, and message content queries repeatedly filter small results and rebuild intermediate frames. This PR removes that overhead while preserving validation, fallback behavior, and supported query results.

Changes

  1. An exact index hit answers a one-predicate seed without re-filtering.
  2. Alias tagging on the seeded hop builds its flag columns in place on pandas and cuDF.
  3. An index hit verifies its residual scalar predicates on the hit rows instead of running the canonical filter (~0.3 ms of fixed overhead on a one-row frame); the typed E302 errors are kept.
  4. The seeded typed hop validates endpoints on its gathered rows with native NumPy or CuPy arrays on pandas and cuDF numeric IDs. Null and incompatible ID cases retain the generic path.
  5. rows() / select on pandas skip the copies and null handling a one-row projection does not need (bool marker mask short-circuit; no copy before a mask filter; the projection frame built directly; zero-row edge slice without copy).
  6. A chain of plain row-table calls skips the edge-index scaffolding (an O(E) column write and drop per call). This also fixes a leaked __gfql_edge_index__ column on an edge row table.
  7. The alias lookup frame for binding rows is built in three frame ops rather than one per column (joined-rows route 15 → 13 ms locally); the no-scaffold result drops the row context.
  8. The polars single-node branch is a route (admission predicate, lane body, routes-off target, harness parity). No behavior change; the harness then showed the polars general path dropping null-id rows and collapsing duplicate ids on node-only op lists where the lane and the pandas oracle keep them: filed as GFQL polars general path drops null-id node rows and collapses duplicate ids on node-only op lists (pandas keeps them) #2071 (with gfql: rows(table=nodes, source=alias) multiplies rows for duplicate node ids and joins null ids to each other #2034), ledgered as known divergences of that route.

Review fixes at e5f67a7

  • Declared graph and AST state uses direct attributes. Optional endpoint annotations match the existing guards; backend capability checks remain where needed.
  • cuDF alias insertion and endpoint filtering use native masks and arrays. Nullable residual predicates reject nulls correctly. Alias collisions and duplicate requests retain generic handling.
  • GPU strictness and count corrections are included. Redundant comments were removed.
  • Added 81 review-driven tests covering backend agreement, nulls, aliases, input preservation, mixed ID widths, empty/repeated edges, dangling endpoints, and both directions.

The review response maps the changes to the inline and overall review comments.

Historical benchmark measurements

These measurements accompanied the earlier implementation, before the final review revision. They are retained as historical evidence and are not exact-head e5f67a7 measurements. DGX Spark, indexed pandas, median of three runs in milliseconds; competitor cells are from the published board.

query SF0.1 now (was) SF1 now (was) Kuzu SF0.1 / SF1 Memgraph Neo4j
seed_lookup 3.06 (3.86) 3.19 (4.14) 2.33 / 2.52 0.59 / 0.52 3.50 / 2.88
message_creator 1.77 (2.33) 1.71 (2.41) 1.43 / 1.51 0.60 / 0.31 2.63 / 1.67
message_content 1.12 (1.66) 1.08 (1.68) 0.72 / 0.67 0.49 / 0.29 2.26 / 2.44

W/T/L on the point queries, pandas-idx: vs Neo4j SF0.1 3/0/0, SF1 1/2/0 (was 1/1/2 and 1/0/2); vs Kuzu 0/0/3 at both scales (0.62–0.88×, up from 0.40–0.63×); vs Memgraph 0/0/3 (0.16–0.43×). These historical comparisons measure the pandas changes.

Sentinel at 7eec9cc: gate ok against the master baseline; pandas points 8–28% faster (node-only-props 0.57 → 0.41 ms).

Remaining performance work

The remaining 1–3 ms per call is orchestration: two chain entries per op list, the call executor, and Plottable rebinds. Memgraph answers these in 0.3–0.6 ms. Reaching that needs a dedicated point-lookup route (index gather → row table → projection in one pass) and, for seed_lookup, a one-call arm; both are follow-ups under the same objective. The polars point shapes decline the seeded lane and take the full chain; they need their own pass.

Validation at e5f67a7

  • DGX RAPIDS 26.02: 13,947 passed, 55 skipped, 67 expected failures, and eight non-strict unexpected passes. A real Polars GPU collect preflight passed.
  • Local suites: 422 focused tests, 1,733 shared tests, 568 strictness/aggregate/row-pipeline tests, and 149 Polars admission/route tests passed. These suites overlap and are not summed.
  • TCK: 4,143 tests passed. Repository lint and ten-source-file type checks passed.
  • Hosted CI succeeded; all 84 reported checks passed on the reviewed head.

The reviewed revision is pushed. Owner review and merge remain pending.

lmeyerov and others added 7 commits September 7, 2026 12:02
…thout re-filtering

`_seed_node_rows` re-applied the canonical filter to the rows the node-id or
node-property index had already gathered. For a filter that is exactly one scalar
equality on the served column the gathered rows are that filter's rows (integer
keys, membership verified after the probe, mismatched value families declined to
the scan), so the re-filter is skipped on that shape only; every other shape keeps
the canonical filter and its typed errors. Pinned on pandas and cuDF with parity to
index_policy="off".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QztW7jYsDd66e8rb8pJNQA
…lumns in place

`_tag_fast_path_aliases` attached the alias flags with assign, a full column reorder and
reset_index on both frames (three copies each). On pandas, with integer ids and no
colliding alias names, the flags are now numpy membership tests inserted into one
reset copy; every other shape (binding column not first, colliding names, float or
object ids, cuDF) keeps the generic path. Pinned frame-identical to the generic path
for every alias shape and direction.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QztW7jYsDd66e8rb8pJNQA
…he hit rows instead of re-filtering

The canonical filter costs about 0.3 ms of fixed overhead per call on a one-row frame; after an index hit the remaining scalar equalities are checked directly on the hit rows, with the same typed errors.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdYcPgiafGGutW9KfG3gg1
…red rows with numpy array ops on pandas

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdYcPgiafGGutW9KfG3gg1
… a one-row projection does not need

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdYcPgiafGGutW9KfG3gg1
…ffolding

Adding and dropping the internal edge index costs an O(E) column write per call and leaked the index column into an edge row table; row-table calls never read edge identity.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdYcPgiafGGutW9KfG3gg1
… frame ops; no-scaffold call results drop the row context; lever pins join the polars lane

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdYcPgiafGGutW9KfG3gg1
@lmeyerov

lmeyerov commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Real-GPU lane (dgx-spark, RAPIDS 26.02 image, safe_run) at b73cab4: 13830 pass / 7 fail. The 7 failure ids are identical to master's known set in that image (#2064 strictness cases + the routing-contract decline pin); zero branch-specific failures. Sentinel and SNB arm receipts are in the body; broad CPU+cuDF suites, tck-gfql and CI follow.

…redicate, lane body, routes-off target, harness parity

The harness then shows the polars GENERAL path dropping null-id rows and collapsing duplicate ids on node-only op lists where the lane and the pandas oracle keep them (#2071, #2034); ledgered as known divergences of that route.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdYcPgiafGGutW9KfG3gg1
@lmeyerov

lmeyerov commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Receipts at 40edaf6

  • Broad CPU+cuDF suites (chain, hop, gfql, gfql/, specializations, collision, internal columns, hypergraph): 13253 pass / 0 fail.
  • tck-gfql: 4143 pass / 689 xfail (unchanged).
  • Real-GPU lane (dgx-spark, RAPIDS 26.02): 13854 pass / 7 fail, failure ids identical to master's known set.
  • CI: running; posted when it lands.

Polars point latency on dgx is thread fan-out (sentinel, same head, same window, ms)

point default POLARS_MAX_THREADS=4 =1 pandas
is1-official 6.16 3.69 2.46 1.79
is5-official 4.98 2.07 1.21 0.89
seeded-hop-props 4.21 5.66 1.22 0.85
seeded-hop-entity 5.37 4.87 2.61 2.86
node-only-entity 2.48 3.73 2.07 2.35
native-hop-alias-collision (full chain, bulk) 27.35 41.19 61.81 1.84

On the 20-core ARM box the per-call cost of the polars point shapes is the thread pool, not the query: at one thread they land within 1.4× of pandas, and the bulk shape gets slower as expected. Polars has no per-collect thread control, so this PR does not change polars behavior; it records the finding. Consequence for the board: pandas is the point-query engine and polars the bulk engine, which is what the engines page already recommends.

@lmeyerov
lmeyerov marked this pull request as ready for review September 7, 2026 20:49
@lmeyerov

lmeyerov commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

CI on 40edaf6: 84/84 green. Ready for review: all receipts on this PR (broad CPU+cuDF 13253/0, tck 4143/689 xfail, real-GPU lane = master's known set, sentinel gate ok with pandas points 8–28% faster, SNB arm table, polars thread experiment).

polars equivalent expr is ``pl.col(source).fill_null(False).cast(pl.Boolean)``).
Shared by ``rows``/``count_table`` so the null handling can't diverge."""
mask = table_df[source]
if str(getattr(mask, "dtype", "")) == "bool":

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move away from getattr in all these? audit, fix..

pandas frames with numeric id columns; None keeps the engine-generic Series path."""
import numpy as np
import pandas as pd
if not isinstance(cand, pd.DataFrame) or not isinstance(edges, pd.DataFrame):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does cudf need this?

Comment thread graphistry/compute/chain.py Outdated
g_out = self.nodes(g_out._nodes).edges(final_edges_df, edge=original_edge)
else:
from .gfql.exec_context import clear_row_exec_context
g_out = clear_row_exec_context(g_out) # the row context must not escape on a result

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of the comments in this pr seems unnecessary, better handled by better variable naming, adding tests for the intended behavior, or when straight up redundant, just delete

Comment thread graphistry/compute/chain_fast_paths.py Outdated
return res.nodes(nodes).edges(edges)


def _tag_fast_path_aliases_pandas(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-platform for cudf?

@lmeyerov lmeyerov left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels brittle

  • sufficient positive/negative tests on either side of boundaries?
  • saw some dynamic typing
  • looked like missing cross-platform support & testing, eg, cudf
  • I have to wonder about test amplification in general, and true generlaization...

@lmeyerov

lmeyerov commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the inline comments and overall review in e5f67a7.

  • Added 81 review-driven cases for null handling, alias combinations, input preservation, mixed ID widths, empty/repeated edges, dangling endpoints, and both traversal directions.
  • Generalized alias insertion and numeric endpoint filtering to cuDF. Endpoint arrays remain on the GPU; unsupported nullable/mixed-ID cases retain the generic path.
  • Fixed indexed all-null residual masks, nullable string comparisons in endpoint filtering, and nullable cuDF alias-mask normalization. Repeated requested aliases now retain generic handling.
  • Replaced dynamic access to declared graph/AST attributes, corrected optional endpoint annotations, and removed redundant comments. Also included the missing-property strictness/count fixes needed for the real Polars GPU gate.

Validation at e5f67a7:

  • DGX RAPIDS 26.02 (cuDF 26.02.01, Polars 1.35.2), with successful real Polars GPU collect preflight: 13,947 passed, 55 skipped, 67 expected failures, 8 non-strict unexpected passes; zero failures, process exit 0.
  • Local final focused suite: 422 passed; shared suite: 1,733 passed; strictness/aggregate/row pipeline: 568 passed; Polars admission/routes: 149 passed. These suites overlap.
  • TCK: 4,143 passed, 6 skipped, 689 expected failures. Full lint and changed-source type checks passed.

The new head is pushed; hosted CI is being checked separately. No merge performed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant