Skip to content

Commit 54fcba3

Browse files
authored
Merge pull request #1840 from graphistry/feat/gfql-range-chain-parity
test(gfql): lock range-chain index decline
2 parents 0f8d26d + aa5338a commit 54fcba3

6 files changed

Lines changed: 141 additions & 14 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
99
<!-- Do Not Erase This Section - Used for tracking unreleased changes -->
1010

1111
### Added
12+
- **Structured GFQL index-planner diagnostics**: `gfql_explain()` and the planner trace now expose a stable `decision_code` alongside the human-readable `decision_reason`. Codes distinguish policy-off scans, missing resident indexes, unsupported query shapes, missing graph bindings, cost-gate declines, selected indexes, engine mismatches, and unavailable index paths. `index_policy=force` bypasses only the cost gate for an index-coverable shape. Unsupported or unavailable shapes still scan and return the same result. Tests cover no resident index, missing graph columns, automatic cost decline, forced selection, policy-off diagnostics, and scan/index parity.
1213
- **Static enforcement for the type-hygiene defect classes that keep failing code review**: five recurring review rejections -- unannotated function contracts, dynamic attribute access, `Any` + call-site `cast()`, unparameterized `list`/`dict`, and a bare `str` where a `Literal` belongs -- are now checked by tooling instead of by a human. `bin/ci_type_hygiene_guard.py` is a stdlib-only AST pass over `graphistry/` (tests excluded, matching `mypy.ini`) wired into `bin/lint.sh`, so it runs in the existing `python-lint-types` matrix on py3.8-3.14 with no new workflow and no new job; it takes ~1.4s and returns byte-identical counts on all seven interpreters. Enforcement is a **per-file count ratchet** against `bin/ci_type_hygiene_baseline.json` -- a file may not gain findings and a file absent from the baseline must have zero -- because the honest measurement is that the codebase carries 1704 missing annotations, 1605 `Any`-in-annotation sites and 959 `cast()` calls, and a rule that flags those as errors today is a rule nobody can turn on. Two checks are effectively hard rules rather than ratchets because their real counts are tiny: `plottable-setattr` has **3** sites repo-wide (one of them is the open bug #1825, where caching by `setattr` onto the caller's `Plottable` keyed on `id()` returned a stale answer after an in-place frame mutation), and `plottable-attr-write` -- the `param.attr = ...` form the same hazard takes when nobody writes `setattr` -- has 39, all in five files. A blanket `getattr`/`setattr` ban was measured and rejected: 300 non-test `getattr` sites are overwhelmingly legitimate optional-attribute reads, and the leak requires a *write*, so only writes onto a parameter annotated as a `Plottable` are flagged. Class 5 gets the narrowest defensible rule rather than a plausible one: `vocab-str-param` knows exactly six parameter names (`table`, `kind`, `direction`, `how`, `mode`, `engine`) whose vocabularies this repo has already committed to as `Literal` aliases, and covers 42 sites; the general "this `str` should be a `Literal`" question was prototyped as a call-site/comparison heuristic, measured at roughly 80% precision over 44 hits, and deliberately **not** shipped -- a column name is legitimately `str`, and a rule that needs manual triage every PR is worse than no rule. Ruff gains `B009`/`B010` (constant-name `getattr`/`setattr`) as genuine errors, with today's 31 offenders across 14 files seeded into `per-file-ignores` and retired one file at a time by `ruff check --fix`. Findings that are genuinely correct are annotated in place with `# hygiene-ok: <check> -- <reason>` rather than by raising a baseline cap. Conventions and escape hatches documented in DEVELOP.md "Type Hygiene Guard". No production code changed.
1314
- **Native polars `rows(binding_ops=...)` for UNBOUNDED directed variable-length patterns (`-[*]->` / `-[*0..]->`) (#1709)**: the Cypher multi-alias bindings table already lowered natively for fixed-length and *bounded* variable-length segments, but an unbounded fixed-point segment declined with `NotImplementedError: polars engine does not yet natively support cypher row op 'rows'`. This was the last shape blocking `engine='polars'` on LDBC SNB interactive-short-6 (`MATCH (m:Message)-[:REPLY_OF*0..]->(p:Post)<-[:CONTAINER_OF]-(f:Forum)-[:HAS_MODERATOR]->(mod)`), the one interactive-short query polars could not answer. It now runs natively: a dedup-by-node frontier walk finds the exhaustion depth, then the SAME lazy bounded pair-join loop the `-[*1..k]->` arm uses materializes one row per distinct edge SEQUENCE (Cypher path multiplicity, parallel edges included) — no pandas bridge, no `to_pandas()` round trip. A cycle reachable from the seed means infinitely many paths; that raises the same E108 "require terminating variable-length segments" error pandas raises — same exception class and same `.code` on both engines — and is detected before the path expansion blows up rather than after, bounded by the REACHABLE node count so an unreachable remainder of the graph costs nothing. Still declining honestly (NIE, never a silent answer): UNDIRECTED unbounded (`-[*]-`, needs the min_hops == 1 multiplicity reconstruction plus backtrack-aware termination — pandas rejects it outright), aliased variable-length relationships (pandas rejects those too), unbounded segments WITHOUT `to_fixed_point` (pandas silently truncates at a bound this lowering cannot reconstruct), unbounded segments with `min_hops >= 2` (`-[*2..]->`: pandas' step pairs are pruned by min_hops against a dedup-by-node eccentricity, which this raw-edge reconstruction cannot reproduce — serving it would return a different count with no error), and `to_fixed_point` combined with an explicit bound (declined on master too; it is not Cypher-reachable, since the parser only sets the flag for `*` / `*k..` where there is no maximum, but through the AST surface it hits the same reconstruction gap for `min_hops >= 3`). Cross-engine parity is the gate: differential fuzz vs the pandas oracle over random DAGs and cyclic graphs with self-loops and parallel edges, plus pinned IS6/zero-hop/multiplicity/cycle tests.
1415

graphistry/compute/gfql/index/api.py

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
from .policy import IndexPolicy, validate_index_policy
2626
from .types import (
2727
AdjacencyIndexKind, EdgeIndexDirection, HopDirection, IndexKind,
28-
IndexTrace, IndexTraceStep,
28+
IndexDecisionCode, IndexTrace, IndexTraceStep,
2929
)
3030

3131
# Private Plottable attachment keys. Keep access behind helpers.
@@ -140,6 +140,7 @@ def _record_indexed_traversal(
140140
"hop_details": [] if hop_details is None else hop_details,
141141
"path": path,
142142
"decision_reason": reason,
143+
"decision_code": "index_selected" if served else "index_path_unavailable",
143144
}))
144145

145146

@@ -514,6 +515,8 @@ def maybe_index_hop(
514515
Cost gate: only route to the index when (a) a valid matching index is resident
515516
(or buildable under auto/force), (b) the query is covered, (c) the frontier is
516517
not so large that a full scan is cheaper. Correctness is identical either way.
518+
"force" bypasses only the cost gate for a covered query. It still falls back to
519+
scan when the index cannot serve the query.
517520
"""
518521
resolved_policy: IndexPolicy = validate_index_policy(policy) or "use"
519522

@@ -533,16 +536,18 @@ def maybe_index_hop(
533536
except (AttributeError, TypeError, ValueError):
534537
pass
535538

536-
def _bail(reason: str) -> Optional[Plottable]:
539+
def _bail(reason: str, decision_code: IndexDecisionCode) -> Optional[Plottable]:
537540
if trace:
538-
_record(cast(IndexTraceStep, {**diag, "path": "scan", "decision_reason": reason}))
541+
_record(cast(IndexTraceStep, {
542+
**diag, "path": "scan", "decision_reason": reason, "decision_code": decision_code,
543+
}))
539544
return None
540545

541546
if resolved_policy == "off":
542-
return _bail("policy=off")
547+
return _bail("policy=off", "policy_off")
543548
registry = get_registry(g)
544549
if registry.is_empty() and resolved_policy not in ("auto", "force"):
545-
return _bail("no resident index (policy=use)")
550+
return _bail("no resident index (policy=use)", "no_resident_index")
546551

547552
min_hops = cast(Optional[int], rest.get("min_hops"))
548553
max_hops = cast(Optional[int], rest.get("max_hops"))
@@ -575,18 +580,18 @@ def _bail(reason: str) -> Optional[Plottable]:
575580
target_wave_front=target_wave_front,
576581
return_as_wave_front=return_as_wave_front,
577582
):
578-
return _bail("query not index-coverable")
583+
return _bail("query not index-coverable", "not_index_coverable")
579584
assert nodes is not None
580585

581586
node_col = g._node
582587
src, dst = g._source, g._destination
583588
if node_col is None or src is None or dst is None or g._edges is None or g._nodes is None:
584-
return _bail("graph missing node/edge columns")
589+
return _bail("graph missing node/edge columns", "missing_graph_columns")
585590

586591
if resolved_policy in ("auto", "force"):
587592
registry = _ensure_indexes(g, registry, direction, engine, resolved_policy, nodes, src, dst, node_col)
588593
if registry.is_empty():
589-
return _bail("no index available (build declined)")
594+
return _bail("no index available (build declined)", "index_build_declined")
590595

591596
# Cost gate: if the frontier covers a large fraction of distinct sources, the
592597
# scan path is competitive — fall back (avoids index overhead on bulk-ish hops).
@@ -612,7 +617,7 @@ def _bail(reason: str) -> Optional[Plottable]:
612617
if idx0.n_keys > 0 and frontier_n >= frac * idx0.n_keys:
613618
return _bail(
614619
f"frontier {frontier_n} >= {frac}*n_keys "
615-
f"({frac * idx0.n_keys:.0f}) -> scan cheaper"
620+
f"({frac * idx0.n_keys:.0f}) -> scan cheaper", "scan_cost"
616621
)
617622
except (AttributeError, TypeError, ValueError):
618623
pass
@@ -629,12 +634,20 @@ def _bail(reason: str) -> Optional[Plottable]:
629634
edge_match=cast(Optional[dict], rest.get("edge_match")),
630635
)
631636
if trace:
637+
engine_mismatch_reason = (
638+
_engine_mismatch_reason(registry, direction, engine) if result is None else None
639+
)
632640
_record(cast(IndexTraceStep, {
633641
**diag, "hops": eff_hops,
634642
"path": "index" if result is not None else "scan",
635643
"decision_reason": (
636644
"frontier below cost gate -> index" if result is not None
637-
else _engine_mismatch_reason(registry, direction, engine) or "index path not applicable -> scan"
645+
else engine_mismatch_reason or "index path not applicable -> scan"
646+
),
647+
"decision_code": (
648+
"index_selected" if result is not None
649+
else "engine_mismatch" if engine_mismatch_reason is not None
650+
else "index_path_unavailable"
638651
),
639652
}))
640653
return result

graphistry/compute/gfql/index/explain.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
from graphistry.compute.gfql.query_types import GFQLQuery
1414
from .api import index_trace, show_indexes
1515
from .policy import IndexPolicy, validate_index_policy
16-
from .types import IndexTraceStep
16+
from .types import IndexDecisionCode, IndexTraceStep
1717

1818
if TYPE_CHECKING:
1919
from graphistry.compute.ComputeMixin import ComputeMixin
@@ -29,6 +29,7 @@ class GfqlExplainReport(TypedDict):
2929
est_result_rows: Optional[int]
3030
chosen_direction: Optional[str]
3131
decision_reason: Optional[str]
32+
decision_code: Optional[IndexDecisionCode]
3233
error: Optional[str]
3334

3435

@@ -54,6 +55,8 @@ def gfql_explain(
5455
# of seeds; `est_result_rows` = estimated fanout (Σ seed degree, free from CSR).
5556
ref = [s for s in steps if s.get("path") == "index"] or list(steps)
5657
last = ref[-1] if ref else {}
58+
if not last and resolved_policy == "off":
59+
last = {"decision_reason": "policy=off", "decision_code": "policy_off"}
5760
resident_names = cast(List[str], resident["name"].tolist() if len(resident) else [])
5861
return {
5962
"engine": eng.value,
@@ -65,5 +68,6 @@ def gfql_explain(
6568
"est_result_rows": cast(Optional[int], last.get("est_result_rows")),
6669
"chosen_direction": cast(Optional[str], last.get("direction")),
6770
"decision_reason": cast(Optional[str], last.get("decision_reason")),
71+
"decision_code": last.get("decision_code"),
6872
"error": error,
6973
}

graphistry/compute/gfql/index/policy.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@
1010
def validate_index_policy(policy: Optional[str]) -> Optional[IndexPolicy]:
1111
"""Validate a public ``index_policy`` value.
1212
13-
``None`` means the caller did not override the default planner behavior.
13+
``None`` means the caller did not override the default planner behavior. "force"
14+
is a performance and diagnostic opt-in: it builds missing indexes and bypasses the
15+
cost gate for an index-coverable query. It does not require index coverage and never
16+
changes query results. A query that the index cannot serve still uses the scan path.
1417
"""
1518
if policy is None:
1619
return None

graphistry/compute/gfql/index/types.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,17 @@
2121

2222

2323
IndexPath = Literal["scan", "index"]
24+
IndexDecisionCode = Literal[
25+
"policy_off",
26+
"no_resident_index",
27+
"not_index_coverable",
28+
"missing_graph_columns",
29+
"index_build_declined",
30+
"scan_cost",
31+
"index_selected",
32+
"engine_mismatch",
33+
"index_path_unavailable",
34+
]
2435

2536

2637
# One column's constraint in an ``edge_match``/filter dict — exactly the runtime shapes
@@ -54,6 +65,7 @@ class IndexTraceStep(TypedDict, total=False):
5465
frontier_n: int
5566
path: IndexPath
5667
decision_reason: str
68+
decision_code: IndexDecisionCode
5769
n_keys: int
5870
seed_deg_sum: Optional[int]
5971
est_result_rows: Optional[int]

graphistry/tests/compute/gfql/index/test_index.py

Lines changed: 96 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,15 @@
55
importorskip so GPU lanes run only where available.
66
"""
77
import importlib
8+
from copy import copy
89

910
import numpy as np
1011
import pandas as pd
1112
import pytest
1213

1314
import graphistry
1415
from graphistry.compute.ast import n, e_forward, e_reverse
15-
from graphistry.compute.gfql.index.api import _engine_mismatch_reason
16+
from graphistry.compute.gfql.index.api import _engine_mismatch_reason, maybe_index_hop
1617
from graphistry.compute.gfql.index import (
1718
CreateIndex, DropIndex, ShowIndexes, index_op_from_json, parse_index_ddl,
1819
get_registry,
@@ -230,12 +231,84 @@ def test_index_policy_force_and_explain(graph, engine):
230231
chain = [n({"id": 0}), e_forward(hops=1)]
231232
rep_off = graph.gfql_explain(chain, index_policy="off", engine=engine)
232233
assert rep_off["used_index"] is False
234+
assert rep_off["decision_code"] == "policy_off"
235+
assert rep_off["decision_reason"] == "policy=off"
233236
rep_force = graph.gfql_explain(chain, index_policy="force", engine=engine)
234237
assert rep_force["used_index"] is True
238+
assert rep_force["decision_code"] == "index_selected"
235239
# results identical regardless of policy
236240
r_scan = graph.gfql(chain, engine=engine)
237241
r_force = graph.gfql(chain, index_policy="force", engine=engine)
238242
assert _sig(r_scan) == _sig(r_force)
243+
assert rep_force["error"] is None
244+
245+
246+
def test_maybe_index_hop_reports_no_resident_index(graph):
247+
"""A direct planner caller gets a named scan decline when no index is resident."""
248+
from graphistry.Engine import Engine
249+
from graphistry.compute.gfql.index import index_trace
250+
251+
with index_trace() as steps:
252+
result = maybe_index_hop(
253+
graph,
254+
Engine.PANDAS,
255+
nodes=pd.DataFrame({"id": [0]}),
256+
hops=1,
257+
direction="forward",
258+
return_as_wave_front=False,
259+
policy="use",
260+
)
261+
262+
assert result is None
263+
assert steps[-1]["decision_code"] == "no_resident_index"
264+
265+
266+
def test_maybe_index_hop_reports_missing_graph_columns(graph):
267+
"""A resident index cannot make an unbound graph use a different query path."""
268+
from graphistry.Engine import Engine
269+
from graphistry.compute.gfql.index import index_trace
270+
271+
unbound = copy(graph.gfql_index_all(engine="pandas"))
272+
unbound._nodes = None
273+
with index_trace() as steps:
274+
result = maybe_index_hop(
275+
unbound,
276+
Engine.PANDAS,
277+
nodes=pd.DataFrame({"id": [0]}),
278+
hops=1,
279+
direction="forward",
280+
return_as_wave_front=False,
281+
policy="use",
282+
)
283+
284+
assert result is None
285+
assert steps[-1]["decision_code"] == "missing_graph_columns"
286+
287+
288+
def test_maybe_index_hop_reports_auto_build_decline_with_scan_parity(graph):
289+
"""An unselective auto request declines the build and preserves scan results."""
290+
from graphistry.Engine import Engine
291+
from graphistry.compute.gfql.index import index_trace
292+
293+
seeds = graph._nodes.copy()
294+
with index_trace() as steps:
295+
result = maybe_index_hop(
296+
graph,
297+
Engine.PANDAS,
298+
nodes=seeds,
299+
hops=1,
300+
direction="forward",
301+
return_as_wave_front=False,
302+
policy="auto",
303+
)
304+
305+
auto_graph = graph.bind()
306+
auto_graph._gfql_index_policy = "auto"
307+
auto_result = auto_graph.hop(nodes=seeds, engine="pandas", hops=1)
308+
scan_result = graph.hop(nodes=seeds, engine="pandas", hops=1)
309+
assert result is None
310+
assert steps[-1]["decision_code"] == "index_build_declined"
311+
assert _sig(auto_result) == _sig(scan_result)
239312

240313

241314
@pytest.mark.parametrize("engine", ENGINES)
@@ -261,6 +334,7 @@ def test_explain_exposes_planner_diagnostics(graph, engine):
261334
gi = graph.gfql_index_all(engine=engine)
262335
rep_off = gi.gfql_explain(chain, index_policy="off", engine=engine)
263336
assert rep_off["used_index"] is False
337+
assert rep_off["decision_code"] == "policy_off"
264338
assert rep_off["decision_reason"] == "policy=off", rep_off
265339

266340

@@ -506,7 +580,7 @@ def test_index_min_two_bounded_range_scans_pandas(graph):
506580
indexed = _force(graph, "pandas").hop(nodes=seeds, engine="pandas", **kwargs)
507581
assert _sig(base) == _sig(indexed)
508582
assert not any(step["path"] == "index" for step in steps), steps
509-
assert any(step["decision_reason"] == "query not index-coverable" for step in steps), steps
583+
assert any(step["decision_code"] == "not_index_coverable" for step in steps), steps
510584

511585
@pytest.mark.parametrize("engine", ENGINES)
512586
def test_index_duplicate_node_ids(engine):
@@ -791,6 +865,9 @@ def topd(df):
791865
e_forward({"etype": 2}, hops=1)]
792866
_UNTYPED_CHAIN = [n({"id": 100}), e_forward(hops=1)]
793867
_MEMBER_CHAIN = [n({"id": 100}), e_forward({"etype": [0, 1]}, hops=1)]
868+
_RANGE_CHAIN = [n({"id": 100}), e_forward(
869+
min_hops=1, max_hops=2, output_min_hops=2, output_max_hops=2,
870+
)]
794871

795872

796873
@pytest.mark.parametrize("engine", ENGINES)
@@ -832,6 +909,23 @@ def test_chain_membership_edge_match_stays_on_scan(typed_graph, engine):
832909
assert rep["used_index"] is False, (engine, rep)
833910

834911

912+
def test_chain_range_with_auto_labels_stays_on_scan(typed_graph):
913+
"""A Cypher range needs per-depth records, so it must decline until indexed."""
914+
engine = "pandas"
915+
from graphistry.compute.gfql.index import index_trace
916+
917+
gi = typed_graph.gfql_index_all(engine=engine)
918+
base = typed_graph.gfql(_RANGE_CHAIN, index_policy="off", engine=engine)
919+
with index_trace() as steps:
920+
indexed = gi.gfql(_RANGE_CHAIN, index_policy="force", engine=engine)
921+
rep = gi.gfql_explain(_RANGE_CHAIN, index_policy="force", engine=engine)
922+
assert _sig_typed(base) == _sig_typed(indexed)
923+
assert rep["used_index"] is False, (engine, rep)
924+
assert not any(step["path"] == "index" for step in steps), steps
925+
assert rep["decision_code"] == "not_index_coverable", rep
926+
assert any(step["decision_code"] == "not_index_coverable" for step in steps), steps
927+
928+
835929
def test_rebind_edges_revalidates_after_shallow_augmentation():
836930
"""rebind_edges re-points the edge adjacency index at a shallow-copied frame that
837931
merely ADDS a column (the chain's synthetic edge id), so get_valid recognizes it."""

0 commit comments

Comments
 (0)