Skip to content

Commit 91e4a32

Browse files
lmeyerovclaude
andcommitted
fix(gfql): keep policy-bearing queries off the native AUTO route
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
1 parent 62239d2 commit 91e4a32

3 files changed

Lines changed: 93 additions & 3 deletions

File tree

CHANGELOG.md

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

2626
### Changed
27+
- **`engine='auto'` (the default) now runs the NATIVE polars engine when both bound frames are polars (#1743)**: `resolve_engine(AUTO)` maps polars frames to `Engine.PANDAS` — that branch predates `Engine.POLARS` — so `g.gfql(q)` on a polars-frame graph silently bridged every query onto the generic pandas path and handed pandas frames back. **Behaviour change, user-visible by default: frames in = frames out.** AUTO on a polars-frame graph now returns polars frames, and shapes the native engine declines (`NotImplementedError`, e.g. `shortestPath`) still answer via the legacy AUTO path — permitted here because the user pinned no engine; the no-silent-fallback contract still applies to an explicit `engine=`. Measured on dgx-spark under the exclusive perf lock, position-balanced 8-slot ABBA/BAAB, matched graph-benchmark q1-q9 on the 20k dataset through `g.gfql(<cypher>)` with no engine argument: **q1 41.3 → 14.5 ms, q2 70.3 → 17.8, q3 24.0 → 7.9, q4 22.1 → 6.8, q5 102.8 → 8.6, q6 106.5 → 10.3, q7 39.6 → 10.6, q8 20.3 → 2.3, q9 38.5 → 15.7** — nine of nine faster with non-overlapping per-slot ranges, canonical row values identical on every cell, and the returned frame type (`pandas` before, `polars` after) recorded per query as the engagement receipt. The declined-shape fallback costs a flat **0.23 ms** (the decline is raised at planning, before any data is touched), i.e. 0.6% of the cheapest declining query and independent of graph size. Two boundaries are deliberately NOT crossed: a graph whose node and edge frames are not BOTH polars is untouched, and a query carrying a `policy` is untouched — the native executor does not go through `chain_impl` and so never emits the `postload`/`postchain` hooks, and diverting a policy-bearing query there would silently stop enforcing a DENYING `postload` policy. Pinned by positive tests (auto == explicit polars, edges-only graphs, both spellings of AUTO) and by negative tests for each refusal (pandas frames, mixed frames, explicit `engine='pandas'`, policy present, and hooks firing exactly once on a declining shape).
2728
- **`is_lazy` is a type predicate, so the polars engine's eager/lazy split is checked instead of asserted**: `dtypes.is_lazy` decides WHICH member of the two-member `PolarsFrame` union a frame is, but it was declared `-> bool`, so that answer was discarded at the call boundary — an eager-only attribute (`.height`, `.columns`, `.schema`) reached after a lazy guard was unprovable, and the native polars chain closed the gap with `cast("pl.DataFrame", frame)`. A cast is not a type: it re-asserts, unchecked, the exact fact the predicate had just established, once per call site, and a wrong one surfaces as a production `AttributeError` rather than a CI error. `is_lazy` is now declared `-> TypeIs["pl.LazyFrame"]` (PEP 742), which narrows the *negative* branch as well as the positive — the eager side is the one that needed it, so `TypeGuard` would not have done — and the cast is gone. Four previously untyped chain-combine helpers (`_semi`, `_combine_edges`, `_apply_node_names`, and the `_known_empty` guard) now carry real signatures: `_semi`'s two frames share the `PolarsT` TypeVar because polars joins do not mix eagerness, and the two combine helpers take the `_LazyShim` collect-once duck-type they are actually called with. Typing `_apply_node_names` also retired a `getattr(next_step, "edges_empty", None)` — the shim declares that tri-state in `__slots__`, so a typo is now a checker error instead of a silent `None` that would have re-armed the very cardinality gate the guard disarms. Internal only: no runtime behaviour, no public API, and the `TypeIs` import is `TYPE_CHECKING`-only, so no new runtime dependency floor.
2829
- **GFQL execution context is declared rather than attached at runtime**: the private `_gfql_*` per-execution fields (index policy and registry, row-pipeline base graph, carried seed nodes, edge aliases, shortest-path backend, and the indexed-bindings handoff) are now declared on `Plottable` with defaults on `PlotterBase`, instead of being set with `setattr` and read back with `getattr(..., default)`. No public API or behaviour change; internal call sites are typed, and hand-rolled `Plottable` stand-ins must now construct the full context.
2930

graphistry/compute/gfql_unified.py

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1810,11 +1810,24 @@ def gfql(self: Plottable,
18101810
"""
18111811
# engine inference: resolve_engine(AUTO) maps polars frames to PANDAS (polars predates
18121812
# Engine.POLARS there), silently bridging polars-frame graphs onto the generic pandas path
1813-
# (~13x slower on cypher point queries, pandas frames out). Route AUTO to the native polars
1814-
# engine instead; an honest NIE (unsupported shape) falls back to the legacy AUTO path, which
1815-
# is allowed here because the user did not pin an engine.
1813+
# and handing pandas frames back. Measured on the matched graph-benchmark q1-q9 lane through
1814+
# this exact surface (dgx-spark, perf lock, position-balanced over 8 slots): 2.45-11.9x at 20k
1815+
# and 4.8-37.2x at 100k, values identical. Route AUTO to the native polars engine instead;
1816+
# an honest NIE (unsupported shape) falls back to the legacy AUTO path -- allowed here
1817+
# because the user pinned no engine -- at a flat ~0.23 ms, the decline being raised at
1818+
# planning before any data is touched.
1819+
#
1820+
# ``policy is None`` is REQUIRED, not conservatism: the native polars executor does not go
1821+
# through ``chain_impl``, so it never emits the ``postload``/``postchain`` hooks that path
1822+
# emits. Routing a policy-carrying query there would silently stop enforcing a DENYING
1823+
# ``postload`` policy (measured: deny-on-postload blocks under the generic path and does not
1824+
# under the native one) -- a governance hook that stops firing is worse than a slow query.
1825+
# The NIE fallback compounds it: re-running the query would fire ``preload``/``precompile``/
1826+
# ``postcompile`` twice for one user call. Explicit ``engine='polars'`` is unchanged and still
1827+
# carries the pre-existing hook gap; this guard only refuses to make that gap the default.
18161828
if (
18171829
(engine == EngineAbstract.AUTO or engine == EngineAbstract.AUTO.value)
1830+
and policy is None
18181831
and is_polars_df(self._edges) and (self._nodes is None or is_polars_df(self._nodes))
18191832
):
18201833
try:

graphistry/tests/compute/gfql/test_engine_polars_cypher_conformance.py

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,3 +526,79 @@ def test_auto_falls_back_on_polars_nie(self):
526526
out = g.gfql(q)._nodes # auto: answers via fallback
527527
rows = out.to_dict("records") if hasattr(out, "to_dict") else out.to_pandas().to_dict("records")
528528
assert rows == [{"l": 3}]
529+
530+
def test_auto_routes_edges_only_graph(self):
531+
"""``self._nodes is None`` is inside the guard's condition, so it must actually work."""
532+
edges = pl.DataFrame({"s": [0, 1, 2], "d": [1, 2, 3], "type": ["KNOWS"] * 3})
533+
g = graphistry.edges(edges, "s", "d")
534+
out = g.gfql("MATCH (a)-[:KNOWS]->(b) RETURN b.id AS bid ORDER BY bid")._nodes
535+
assert "polars" in type(out).__module__
536+
assert out["bid"].to_list() == [1, 2, 3]
537+
538+
def test_auto_enum_form_routes_too(self):
539+
"""AUTO arrives as either the enum or its string value; both must route."""
540+
from graphistry.Engine import EngineAbstract
541+
g = self._graph()
542+
q = "MATCH (a:Person {id: 0})-[:KNOWS]->(b) RETURN b.id AS bid"
543+
for eng in (EngineAbstract.AUTO, EngineAbstract.AUTO.value):
544+
assert "polars" in type(g.gfql(q, engine=eng)._nodes).__module__
545+
546+
547+
class TestAutoEngineDoesNotRoutePolarsNative:
548+
"""The negative half: what must NOT be diverted. A routing change is only as safe as the
549+
set of inputs it refuses, so each of these pins a case where the pre-change path must
550+
still be taken byte-for-byte."""
551+
552+
NODES = {"id": [0, 1, 2, 3], "label__Person": [True] * 4}
553+
EDGES = {"s": [0, 1, 2], "d": [1, 2, 3], "type": ["KNOWS"] * 3}
554+
Q = "MATCH (a:Person {id: 0})-[:KNOWS]->(b) RETURN b.id AS bid"
555+
556+
def test_pandas_frames_unaffected(self):
557+
g = graphistry.nodes(pd.DataFrame(self.NODES), "id").edges(pd.DataFrame(self.EDGES), "s", "d")
558+
assert "pandas" in type(g.gfql(self.Q)._nodes).__module__
559+
560+
def test_mixed_frames_not_routed(self):
561+
"""Polars edges + pandas nodes: the native engine has no bridge, so AUTO must not try."""
562+
g = (graphistry
563+
.nodes(pd.DataFrame(self.NODES), "id")
564+
.edges(pl.DataFrame(self.EDGES), "s", "d"))
565+
assert "pandas" in type(g.gfql(self.Q)._nodes).__module__
566+
g2 = (graphistry
567+
.nodes(pl.DataFrame(self.NODES), "id")
568+
.edges(pd.DataFrame(self.EDGES), "s", "d"))
569+
assert "pandas" in type(g2.gfql(self.Q)._nodes).__module__
570+
571+
def test_explicit_pandas_engine_still_pandas(self):
572+
g = graphistry.nodes(pl.DataFrame(self.NODES), "id").edges(pl.DataFrame(self.EDGES), "s", "d")
573+
assert "pandas" in type(g.gfql(self.Q, engine="pandas")._nodes).__module__
574+
575+
def test_policy_disables_the_route(self):
576+
"""The native executor does not go through ``chain_impl`` and so never emits
577+
``postload``/``postchain``. Diverting a policy-carrying query would silently stop
578+
enforcing a DENYING postload policy -- measured, not theorised."""
579+
from graphistry.compute.gfql.policy import PolicyException
580+
g = graphistry.nodes(pl.DataFrame(self.NODES), "id").edges(pl.DataFrame(self.EDGES), "s", "d")
581+
582+
def deny(ctx):
583+
raise PolicyException(phase="postload", reason="denied by test")
584+
585+
with pytest.raises(PolicyException):
586+
g.gfql(self.Q, policy={"postload": deny})
587+
# ...and structurally: a policy-bearing AUTO query stays on the generic path,
588+
# which is what makes the hook trace identical to the pre-change build.
589+
assert "pandas" in type(g.gfql(self.Q, policy={"preload": lambda ctx: None})._nodes).__module__
590+
seen = []
591+
pol = {h: (lambda ctx, h=h: seen.append(h)) for h in ("preload", "postload", "precompile")}
592+
g.gfql(self.Q, policy=pol)
593+
assert "postload" in seen, "postload must still fire under AUTO on a polars graph"
594+
595+
def test_policy_hooks_fire_once_on_nie_shape(self):
596+
"""A retry-on-NIE route would fire the compile/load hooks twice for one user call."""
597+
g = graphistry.nodes(pl.DataFrame(self.NODES), "id").edges(pl.DataFrame(self.EDGES), "s", "d")
598+
q = ("MATCH p = shortestPath((a:Person {id: 0})-[:KNOWS*]-(b:Person {id: 3})) "
599+
"RETURN length(p) AS l")
600+
seen = []
601+
pol = {h: (lambda ctx, h=h: seen.append(h)) for h in ("preload", "precompile", "postcompile")}
602+
g.gfql(q, policy=pol)
603+
assert seen.count("precompile") == 1, seen
604+
assert seen.count("preload") == 1, seen

0 commit comments

Comments
 (0)