perf(gfql): route engine=auto to native polars for polars-frame graphs - #1743
Draft
lmeyerov wants to merge 4 commits into
Draft
perf(gfql): route engine=auto to native polars for polars-frame graphs#1743lmeyerov wants to merge 4 commits into
lmeyerov wants to merge 4 commits into
Conversation
lmeyerov
marked this pull request as draft
July 21, 2026 08:51
Contributor
Author
|
Too early to enable by default |
lmeyerov
added a commit
that referenced
this pull request
Jul 22, 2026
…rialize-first Review findings (plans/review-pr-1767/review.md): B1 edges-only polars graph crashed in the NODE_ID build (materialize under AUTO synthesizes pandas; polars gathers ran on a pandas frame) — gfql_index_all now materializes + engine-aligns BEFORE any build so all indexes land valid, and create_index re-aligns for direct calls. M1 LazyFrame frames under AUTO crashed — the gate now requires EAGER polars DataFrames on ALL present frames (LazyFrame/mixed keep the legacy pandas path). Spy test asserts the index SERVED (non-None), not merely was called. AUTO-query trade-off documented in CHANGELOG (deferred to #1743 by policy hold). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
lmeyerov
force-pushed
the
fix/gfql-auto-engine-polars-native
branch
3 times, most recently
from
July 28, 2026 01:37
1727a49 to
91e4a32
Compare
resolve_engine(AUTO) maps polars frames to PANDAS (it predates Engine.POLARS), so g.gfql(query) on a polars-frame graph silently bridged to the generic pandas path: ~3-13x slower on cypher point queries and pandas frames out. Route AUTO to the native polars engine; an honest NotImplementedError (unsupported shape) falls back to the legacy AUTO path — allowed because the user did not pin an engine. Frames in = frames out: AUTO results on polars graphs are now polars. Repro: 2k-node polars graph, seeded 1-hop cypher — AUTO 25.0ms/pandas out before, 9.3ms/polars out after (engine='polars' = 8.8ms); polars-NIE shapes (shortestPath) still answer via the pandas fallback. Fixes the q5 finding in plans/gfql-benchmark-numbers (inferred-engine 13x penalty). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
…c records The dual-path guards checked hasattr(out, 'to_dict') first, but polars DataFrame HAS to_dict (without orient) — so the polars branch was unreachable and these tests only ever ran because auto silently returned pandas. With auto routing polars-frame graphs natively they now receive polars frames; probe to_dicts first. Assertions (record values) unchanged and still pass on both engines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
The routing change is only as safe as the set of inputs it refuses, and one refusal is load-bearing rather than conservative. The native polars executor does not go through `chain_impl`, so it never emits the `postload` / `postchain` policy hooks that path emits. Measured on a polars-frame graph: a policy that DENIES on `postload` blocks the query under the generic path and does NOT block it under the native one. Routing a policy-carrying query there would therefore have silently stopped enforcing a denying postload policy for every user who never pinned an engine -- a governance hook that stops firing is worse than a slow query. The NIE fallback compounded it: re-running the query fired `preload` / `precompile` / `postcompile` TWICE for one user call, so a policy that counts or rate-limits would have double-counted. `policy is None` in the guard restores both, exactly: the full hook trace and every deny/allow outcome across all six hooks is now byte-identical to the pre-change build on polars-frame graphs, pandas-frame graphs, and the declining shape. Explicit `engine='polars'` is unchanged and still carries the pre-existing hook gap; this only refuses to make that gap the default. Tests: the positive half gains the edges-only graph (`self._nodes is None` is inside the guard's condition, so it must actually work) and both spellings of AUTO. A new negative class pins each refusal -- pandas frames, mixed polars/pandas frames, explicit `engine='pandas'`, a denying postload policy still blocking, and the compile/load hooks firing exactly once on a shape the native engine declines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
lmeyerov
force-pushed
the
fix/gfql-auto-engine-polars-native
branch
from
July 28, 2026 03:01
91e4a32 to
3722129
Compare
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.
Summary
resolve_engine(AUTO)maps polars frames toEngine.PANDAS— that branch predatesEngine.POLARS— sog.gfql(query)on a polars-frame graph silently bridges every query onto the generic pandas path and hands pandas frames back.This routes
engine='auto'(the default) to the native polars engine when both bound frames are polars, with an honest-NotImplementedErrorfallback to the legacy AUTO path. The fallback is permitted here because the user pinned no engine; the no-silent-fallback contract still applies to an explicitengine=.This is a user-visible default-behaviour change, not a bench tweak. Scope is deliberately gfql-only (one guard in
gfql()): changingresolve_engineitself would ripple into plotting/util paths that only handle pandas/cudf.What actually changes, at the surface
float64(30→30.0)Int64with nullsobjectBooleanpolicyengine='pandas'/'polars'/ cudfPerf: both directions, per-slot, with an engagement receipt
Matched graph-benchmark q1-q9, run through
g.gfql(<cypher>)with no engine argument on polars-materialized frames — the surface this PR moves, and the one the committed lanes cannot see because they all pinengine=. Arms: A =233b64c8, B = this branch. One session, slot orderA B B A B A A B,--gpus all, perf lock held. 20k: 7 runs / 3 warmup; 100k: 5 / 2.framesis the engagement receipt: the module of the returned_nodesframe, recorded per query.pandas → polarsmeans the routing engaged on that cell. A cell that reportedpandason arm B would mean the routing did not engage there and its number would mean nothing.20k (20,030 nodes / 260,107 edges)
100k (107,434 nodes / 2,775,195 edges)
Overlapping per-slot ranges are scored TIE, never a win. Every scored cell's per-slot ranges are disjoint by a wide margin, and canonical row values are identical on every cell of both scales.
What the untouched control arms say about the noise floor
Both scales also ran
engine='pandas'andengine='polars'— paths this PR does not touch, so their true delta is zero and they measure the harness.pandas: a uniform +1.1% to +6.5% tilt toward B on all nine, i.e. residual drift that favours A. Thepolars-autowins are therefore conservative.polars: nine TIEs and one spurious FASTER (q1, −11.9%).pandas: flat (−1.8% to +4.2%), all TIE.polars: eight TIEs and one spurious SLOWER (q5, +3.5%) with disjoint slot ranges.Read that as the design's resolution floor: this 8-slot layout produces spurious non-overlapping verdicts up to about ±12% at 20k and ±5% at 100k. A claim in that band would not be safe from this harness. −59% to −97% is an order of magnitude outside it.
Explicitly hunting for what gets SLOWER
Nothing measurably regresses on the served shapes, at any of the four scales probed. The two structural ways this change can cost time were measured directly rather than assumed:
1. The declined-shape fallback tax. A shape the native engine declines pays the declined attempt and then the whole generic path. Isolated (
engine='polars'timed until it raises):A flat ~0.23 ms, independent of graph size — the decline is raised at planning, before any data is touched. On the lane's
niecell it is invisible (TIE at both scales).2. Small-graph fixed cost. The native path could carry a larger constant than the bridge. It does not: at 50 / 1k / 20k nodes across four cheap shapes, arm B is faster or equal on every served cell (e.g. 50-node
cheap_scan12.02 → 1.16 ms; 50-nodecheap_seed21.66 → 1.17 ms).VALUES, not just latency
A 45-shape matrix (node scans, typed/untyped filters, nulls,
toLower, arithmetic, string concat, seeded 1-hop and 2-hop, undirected, five aggregate families, group-by,DISTINCT,ORDER BY/SKIP/LIMIT, multi-alias joins,OPTIONAL MATCH, variable-length,shortestPath,WITH, edge-property filters, whole-entity returns, empty results, and three native chain-API shapes) was run under AUTO on both builds, comparing row counts, canonical values, column dtypes, column order and row order.Row values: 0 mismatches across all 45 shapes once integer/float representation is normalised, and 0 status changes. 44/45 routed natively;
shortestPathtook the fallback.The differences that are real, and which the reviewer should rule on:
30.0→30. The old default upcast tofloat64because pandas had no nullable int; polars keepsInt64. Values are equal numerically; the printed representation and the dtype both change. Same for nullable booleans (object→Boolean)._edges— an aliased edge column moves from first to last (['r','s','d',…]→['s','d',…,'r']) in 5 of 45 shapes. Column sets are identical.-[*1..2]->result carried__gfql_output_edge_hop__in_edgesunder the old default and does not under the new one.Two divergences that are NOT introduced here but that this PR makes the DEFAULT
These already exist between
engine='polars'andengine='pandas'today. They are surfaced because a default-routing change is what promotes them from opt-in to universal. I am not proposing to decide these unilaterally.RETURN sum(a.<string col>)'abac'(pandas concatenates strings)polars.exceptions.InvalidOperationErrorRETURN avg(a.<string col>)GFQLTypeError [type-mismatch]nullmin/maxover strings agree on both engines.Neither old behaviour is openCypher-correct (both should be type errors), but the directions differ in how much they matter:
sum()— a working query becomes a hard failure, and the exception is a raw polars type leaking through the GFQL surface rather than aGFQLTypeError. That is an error-hygiene defect independent of the routing question.avg()— a query that used to raise a typed error now silently returnsnull. This is the direction that worries me: it is the silent-wrong-answer class, arriving by default.My recommendation is that the
avg()-on-string case be fixed in the polars engine (decline or raiseGFQLTypeError) before this lands, and thesum()case at least be wrapped so aGFQLTypeErroris raised instead of a leaked polars exception. Both are small and separable from this PR. Found by a 107-query differential sweep of AUTO-vs-AUTO; the other 99 were identical.gfqlname, which is the@otel_traced("gfql.run")-decorated wrapper, so an AUTO-routed polars query now enters it twice (measured: 2 entries per user call on the routed path, 1 on the pandas path and 1 on explicitengine='polars'). Each user call therefore emits a nested duplicategfql.runspan and evaluates_gfql_otel_attrstwice. Results are unaffected. I have not changed this: the clean fix is to split the body into an undecorated_gfql_impland have the guard call that, which is a mechanical but sizeable move on a file several other in-flight PRs touch, and a nested span arguably documents the routing decision. Flagging it as a reviewer call rather than deciding it here. (gfql.__wrapped__would be a one-token fix but is untypeable without a bannedcast.)The policy hooks: a defect found, and fixed here
The original guard routed policy-carrying queries too. Measured, that was not safe:
chain_impl, so it never emitspostload/postchain. A policy that DENIES onpostloadblocked the query before and did not block it after. A governance hook that silently stops firing is worse than a slow query, and it would have applied to every user who never pinned an engine.preload/precompile/postcompiletwice for one user call — a policy that counts or rate-limits would have double-counted.policy is Noneis now part of the guard. The full hook trace and every deny/allow outcome across all six hooks is byte-identical to233b64c8on polars-frame graphs, pandas-frame graphs, and the declining shape. Explicitengine='polars'is unchanged and still carries the underlying gap — that gap is a separate, pre-existing bug worth its own issue; this PR only refuses to make it the default.Regression lock-in
graphistry/pyg-bench#110addsauto-engine-default-routeto thegfql-perf-lockinlane.It is a structural receipt, not a ladder, and that is the point: a growth ratio measures the fraction of a query proportional to the scaled dimension, and routing moves a constant, so a ladder would police the wrong direction and go green on a build that had silently reverted. The probe runs the named seeded pattern through the bare
g.gfql(query)spelling on polars frames and asserts2= native polars frames out and zeropl.DataFrame.to_pandascalls;1= pandas frames out of a polars-frame graph (regression);0= invalid. Its positive and negative controls (engine='polars'must report2,engine='pandas'must report1) are part of the observation, so a probe that cannot discriminate cannot go green.Validated both ways through the committed runner:
233b64c8→1on every rung, verdictregression, exit 1; the same tree with this PR →2on every rung, exit 0.rows_returned{nodes: 33, edges: 32}on every rung of both.Merge order: pyg-bench#110 is deliberately held until this lands. Against current pygraphistry
masterthe probe correctly reports a regression — which is the proof that the gate works, but merging it first would leavegfql-perf-lockinred for a change that has not shipped, and a lane that is red for a legitimate reason is a lane people learn to ignore.Tests
Positive: AUTO result frame-equal to explicit polars; edges-only graph (
self._nodes is Noneis inside the guard's condition); both spellings of AUTO (enum and string);shortestPathNIEs when pinned and answers via the fallback.Negative — for a routing change, what must not be routed matters as much: pandas frames untouched; mixed polars/pandas frames untouched; explicit
engine='pandas'untouched; a denyingpostloadpolicy still blocks; the compile/load hooks fire exactly once on a declining shape.Verification
bin/lint.shclean ·bin/typecheck.shclean (325 files) ·graphistry/tests/compute/— 123 failed on this branch and 123 failed on233b64c8in the same env, identical failure set (local umap/dask/env failures, unrelated and pre-existing); passes 6806 → 6815, the +9 being the tests added here.🤖 Generated with Claude Code
https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB