From 513bbf5b2d691368c167385f28e3b04e09572df0 Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 25 Jul 2026 09:25:25 -0700 Subject: [PATCH 1/6] perf(gfql): secondary node property indexes for seeded traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A seed predicate on a NON-key column — `MATCH (m {id: 42})` where the graph's node id binding is some other column — cost a full node scan: the registry only indexed the node-id binding and the CSR adjacencies, so a property predicate had no index to consult. On official LDBC SNB SF1 that single scan was 51.8 ms of IS7's 72 ms wall (3,181,724 x 32 rows examined to find one row), while every other step of the traversal totalled under 2 ms. Add the property index as the same pay-as-you-go sidecar as the existing kinds: - `NodePropIndex`: sorted distinct values over node ROW POSITIONS in CSR form, so duplicate values are indexable (unlike the unique-only node-id index). Never reorders `.nodes`; fingerprint- and identity-validated, so a `.nodes()` rebind is a safe miss rather than a wrong answer; engine-polymorphic (numpy host, cupy on-device). Stored in a column-keyed map beside the kind-keyed one. - `build_node_prop_index` declines (-> scan) for anything whose vectorized ordering/equality is not unambiguous on both backends: today integer dtypes only, since float NaN ordering, cupy strings, and nulls each diverge. Widening it later is purely additive. - `lookup_prop_rows` (searchsorted + CSR range expansion) and `prop_match_count`, a free selectivity estimate read straight off the CSR offsets. - The seeded fixed-hop planner picks the MOST SELECTIVE indexed scalar predicate in the seed filter, gathers those candidates, and hands them to the UNCHANGED filter, which applies every remaining predicate. Results are therefore identical whether the index is present, absent, stale, or cost-gated out. - Surface: `create_index('node_prop', column=...)` (column required), `drop_index('node_prop', column=...)`, `show_indexes()` rows, and `g.gfql_index_node_props([...])`, which skips unindexable columns. Opt-in, like the secondary indexes of other engines: with no property index resident, behaviour and cost are exactly as before. Perf (dgx-spark, official LDBC SNB SF1, warm medians, value-identical 19 rows): interactive-short IS7 71.6 ms -> 19.5 ms (3.7x), one-time build 112 ms; the seed lookup alone goes 51.2 ms -> 0.096 ms. A position-balanced A/B of IS1-IS7 with NO property index resident shows no detectable change (per-query deltas inside each build's own 6-10% run-to-run spread). Tests are standard-derived: seed-without-scanning parity plus candidate width on pandas/polars/cuDF, indexed-vs-absent equality, duplicate-value keys gathering every matching row, stale-rebind and policy-off fallback, unindexable dtype / missing column / missing argument errors alongside the skipping convenience wrapper, most-selective-column choice, and the show/drop lifecycle. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 --- CHANGELOG.md | 3 + graphistry/compute/ComputeMixin.py | 15 +- graphistry/compute/gfql/index/__init__.py | 14 +- graphistry/compute/gfql/index/api.py | 67 +++++++- graphistry/compute/gfql/index/bindings.py | 83 +++++++++- graphistry/compute/gfql/index/build.py | 39 ++++- graphistry/compute/gfql/index/lookup.py | 57 ++++++- graphistry/compute/gfql/index/registry.py | 67 +++++++- graphistry/compute/gfql/index/types.py | 2 +- .../gfql/index/test_indexed_bindings.py | 154 ++++++++++++++++++ 10 files changed, 472 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 640bc867f3..0e8eeaebbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Development] +### Added +- **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`. + ### Performance - **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. - **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. diff --git a/graphistry/compute/ComputeMixin.py b/graphistry/compute/ComputeMixin.py index f8956663e9..966cc969d1 100644 --- a/graphistry/compute/ComputeMixin.py +++ b/graphistry/compute/ComputeMixin.py @@ -638,10 +638,10 @@ def create_index(self, kind, *, column=None, name=None, engine='auto'): from graphistry.compute.gfql.index import create_index as _ci return _ci(self, kind, column=column, name=name, engine=engine) - def drop_index(self, kind=None): - """Drop one resident GFQL index (by kind) or all (kind=None). Idempotent; returns a new Plottable.""" + def drop_index(self, kind=None, *, column=None): + """Drop one resident GFQL index (by kind, or one property index by column) or all (kind=None). Idempotent; returns a new Plottable.""" from graphistry.compute.gfql.index import drop_index as _di - return _di(self, kind) + return _di(self, kind, column=column) def show_indexes(self): """Return a pandas DataFrame describing resident GFQL indexes (name, kind, column, valid). Empty if none; ``valid=False`` marks a stale index after a frame rebind.""" @@ -658,6 +658,15 @@ def gfql_index_all(self, engine='auto'): from graphistry.compute.gfql.index import gfql_index_all as _gia return _gia(self, engine=engine) + def gfql_index_node_props(self, columns, engine='auto'): + """Convenience: build node PROPERTY indexes for ``columns`` (secondary indexes). + + A seed predicate on a non-key column (``{id: 42}`` when the graph's node id + is some other column) otherwise costs a full node scan. Unindexable columns + are skipped, keeping the correct scan path. Returns a new Plottable.""" + from graphistry.compute.gfql.index import gfql_index_node_props as _ginp + return _ginp(self, columns, engine=engine) + def filter_nodes_by_dict(self, *args, **kwargs): return filter_nodes_by_dict_base(self, *args, **kwargs) filter_nodes_by_dict.__doc__ = filter_nodes_by_dict_base.__doc__ diff --git a/graphistry/compute/gfql/index/__init__.py b/graphistry/compute/gfql/index/__init__.py index 1d2386923d..2482d9de04 100644 --- a/graphistry/compute/gfql/index/__init__.py +++ b/graphistry/compute/gfql/index/__init__.py @@ -2,7 +2,8 @@ seeded traversal. Public surface (see api.py): ``create_index``, ``drop_index``, ``show_indexes``, -``gfql_index_edges``, ``gfql_index_all``, and the planner entry ``maybe_index_hop``. +``gfql_index_edges``, ``gfql_index_all``, ``gfql_index_node_props``, and the +planner entry ``maybe_index_hop``. These are wired onto Plottable via ComputeMixin. """ from .types import ( @@ -11,11 +12,12 @@ ) from .registry import ( GfqlIndexRegistry, EMPTY_REGISTRY, - EDGE_OUT_ADJ, EDGE_IN_ADJ, NODE_ID, ADJ_KINDS, ALL_KINDS, - AdjacencyIndex, NodeIdIndex, + EDGE_OUT_ADJ, EDGE_IN_ADJ, NODE_ID, NODE_PROP, ADJ_KINDS, ALL_KINDS, + AdjacencyIndex, NodeIdIndex, NodePropIndex, ) from .api import ( create_index, drop_index, show_indexes, gfql_index_edges, gfql_index_all, + gfql_index_node_props, get_registry, set_registry, get_index_policy, maybe_index_hop, index_name, index_trace, ) from .wire import ( @@ -30,10 +32,10 @@ "AdjacencyIndexKind", "ArrayLike", "ArrayNamespace", "EdgeIndexDirection", "HopDirection", "IndexBackend", "IndexKind", "IndexTraceStep", "GfqlIndexRegistry", "EMPTY_REGISTRY", - "EDGE_OUT_ADJ", "EDGE_IN_ADJ", "NODE_ID", "ADJ_KINDS", "ALL_KINDS", - "AdjacencyIndex", "NodeIdIndex", + "EDGE_OUT_ADJ", "EDGE_IN_ADJ", "NODE_ID", "NODE_PROP", "ADJ_KINDS", "ALL_KINDS", + "AdjacencyIndex", "NodeIdIndex", "NodePropIndex", "create_index", "drop_index", "show_indexes", "gfql_index_edges", - "gfql_index_all", "get_registry", "set_registry", "get_index_policy", "maybe_index_hop", "index_name", + "gfql_index_all", "gfql_index_node_props", "get_registry", "set_registry", "get_index_policy", "maybe_index_hop", "index_name", "index_trace", "CreateIndex", "DropIndex", "ShowIndexes", "IndexOp", "apply_index_op", "index_op_from_json", "is_index_op", "is_index_op_json", diff --git a/graphistry/compute/gfql/index/api.py b/graphistry/compute/gfql/index/api.py index 80d4b19092..7251d4cf36 100644 --- a/graphistry/compute/gfql/index/api.py +++ b/graphistry/compute/gfql/index/api.py @@ -8,7 +8,7 @@ from __future__ import annotations import copy -from typing import Dict, List, Literal, Optional, cast +from typing import Dict, List, Literal, Optional, Sequence, cast import pandas as pd @@ -17,9 +17,9 @@ from graphistry.Plottable import Plottable from .registry import ( AdjacencyIndex, GfqlIndexRegistry, EMPTY_REGISTRY, NodeIdIndex, - EDGE_OUT_ADJ, EDGE_IN_ADJ, NODE_ID, ADJ_KINDS, ALL_KINDS, + EDGE_OUT_ADJ, EDGE_IN_ADJ, NODE_ID, NODE_PROP, ADJ_KINDS, ALL_KINDS, ) -from .build import build_adjacency_index, build_node_id_index +from .build import build_adjacency_index, build_node_id_index, build_node_prop_index from .traverse import index_seeded_hop from .cost import cost_gate_frac, seed_deg_sum, seed_id_array from .policy import IndexPolicy, validate_index_policy @@ -216,14 +216,43 @@ def create_index( registry = registry.with_index(NODE_ID, node_idx) return _attach(g2, registry) + if kind == NODE_PROP: + if not column: + raise ValueError( + f"A {NODE_PROP!r} index indexes one node PROPERTY column; pass " + f"column=''." + ) + g2 = g.materialize_nodes() if g._nodes is None else g + assert g2._nodes is not None + if column not in g2._nodes.columns: + raise ValueError( + f"Cannot build a {NODE_PROP!r} index: node column {column!r} not found." + ) + prop_idx = build_node_prop_index(g2._nodes, column, eng) + if prop_idx is None: + raise ValueError( + f"Cannot build a {NODE_PROP!r} index on {column!r}: only integer " + f"columns without nulls are indexable today. Seeded queries still " + f"work via the un-indexed scan path." + ) + prop_idx = replace(prop_idx, name=name or index_name(kind, column)) + registry = registry.with_node_prop(column, prop_idx) + return _attach(g2, registry) + raise ValueError(f"Unknown GFQL index kind: {kind!r}. Expected one of {ALL_KINDS}.") -def drop_index(g: Plottable, kind: Optional[IndexKind] = None) -> Plottable: - """Drop one index (by kind) or all indexes (kind=None). Idempotent.""" +def drop_index( + g: Plottable, kind: Optional[IndexKind] = None, *, column: Optional[str] = None +) -> Plottable: + """Drop one index (by kind, or one property index by column) or all (kind=None). + + Idempotent.""" registry = get_registry(g) if kind is None: return _attach(g, EMPTY_REGISTRY) + if kind == NODE_PROP and column is not None: + return _attach(g, registry.without_node_prop(column)) return _attach(g, registry.without(kind)) @@ -263,6 +292,19 @@ def show_indexes(g: Plottable) -> pd.DataFrame: "nbytes": index_nbytes(idx), "valid": valid, }) + for column in registry.node_prop_cols(): + prop = registry.node_props[column] + rows.append({ + "name": prop.name or index_name(NODE_PROP, column), + "kind": NODE_PROP, + "key_col": column, + "engine": prop.engine.value, + "backend": prop.backend, + "n_keys": prop.n_keys, + "n_rows": prop.n_nodes, + "nbytes": index_nbytes(prop), + "valid": registry.get_node_prop_valid(column, g._nodes, prop.engine) is not None, + }) cols = ["name", "kind", "key_col", "engine", "backend", "n_keys", "n_rows", "nbytes", "valid"] return pd.DataFrame(rows, columns=cols) @@ -277,6 +319,21 @@ def gfql_index_edges(g: Plottable, direction: EdgeIndexDirection = "both", return g +def gfql_index_node_props(g: Plottable, columns: Sequence[str], + engine: EngineAbstractType = EngineAbstract.AUTO) -> Plottable: + """Convenience: build node property indexes for ``columns`` (skips unindexable). + + Skipping mirrors ``gfql_index_all``'s node_id behaviour — a column that cannot + be indexed keeps the correct scan path. ``create_index(NODE_PROP, column=...)`` + still raises, since the caller asked for that column specifically.""" + for column in columns: + try: + g = create_index(g, NODE_PROP, column=column, engine=engine) + except ValueError: + pass + return g + + def gfql_index_all(g: Plottable, engine: EngineAbstractType = EngineAbstract.AUTO) -> Plottable: """Convenience: build out+in adjacency + (when ids are unique) node_id indexes. diff --git a/graphistry/compute/gfql/index/bindings.py b/graphistry/compute/gfql/index/bindings.py index 2dfbd72f90..5dbeee597b 100644 --- a/graphistry/compute/gfql/index/bindings.py +++ b/graphistry/compute/gfql/index/bindings.py @@ -24,8 +24,20 @@ ) from .cost import cost_gate_frac from .engine_arrays import array_namespace, col_to_array, take_rows -from .lookup import lookup_edge_rows, lookup_node_rows -from .registry import EDGE_IN_ADJ, EDGE_OUT_ADJ, NODE_ID, AdjacencyIndex, NodeIdIndex +from .lookup import ( + lookup_edge_rows, + lookup_node_rows, + lookup_prop_rows, + prop_match_count, +) +from .registry import ( + EDGE_IN_ADJ, + EDGE_OUT_ADJ, + NODE_ID, + AdjacencyIndex, + GfqlIndexRegistry, + NodeIdIndex, +) from .traverse import _indices_for_direction @@ -363,6 +375,55 @@ def _lookup_degree(index: AdjacencyIndex, frontier: Any, xp: Any) -> int: counts = index.group_offsets[hits + 1] - index.group_offsets[hits] return int(counts.sum()) + +def _seed_rows_via_property_index( + registry: GfqlIndexRegistry, + nodes: DataFrameT, + first_filter: Mapping[str, Any], + engine: Engine, + xp: Any, + *, + policy: str, +) -> Optional[Any]: + """Node row positions for the most selective indexed scalar seed predicate. + + The seed of a fixed-hop pattern is usually a high-selectivity equality on a + business key (``{id: 42}``) that is NOT the graph's node-id binding, which + otherwise costs a full node scan. When a resident, still-valid property index + covers such a column, gather its candidates instead; the caller re-applies the + WHOLE filter to them, so the result is identical to the scan either way. + + Returns None (keep scanning) when nothing is indexed, no predicate is a plain + integer scalar, or the estimated candidate count is not selective enough to + beat the scan (``force`` skips the cost gate). + """ + if not first_filter: + return None + best_rows = None + best_count: Optional[int] = None + for column in registry.node_prop_cols(): + value = first_filter.get(column) + if value is None or isinstance(value, bool) or not isinstance(value, Integral): + continue + index = registry.get_node_prop_valid(column, nodes, engine) + if index is None: + continue + values = xp.asarray([value]) + count = prop_match_count(index, values, xp) + if best_count is not None and count >= best_count: + continue + best_count = count + best_rows = (index, values) + if best_rows is None or best_count is None: + return None + if policy != "force": + n_nodes = int(nodes.shape[0]) + if best_count >= cost_gate_frac(engine) * n_nodes: + return None # not selective enough to beat one vectorized scan + index, values = best_rows + return xp.sort(lookup_prop_rows(index, values, xp)) + + def _try_indexed_connected_bindings_state( base_graph: Plottable, ops: Sequence[Any], @@ -501,10 +562,20 @@ def _try_indexed_connected_bindings_state( first_nodes = take_rows(nodes, seed_rows, engine) first_nodes = _filter_frame(first_nodes, first_filter, engine) else: - hop_count = (len(ops) - 1) // 2 - if int(nodes.shape[0]) >= hop_count * int(edges.shape[0]): - return None - first_nodes = _filter_frame(nodes, first_filter, engine) + prop_rows = _seed_rows_via_property_index( + registry, nodes, first_filter, engine, xp, policy=get_index_policy(base_graph), + ) + if prop_rows is not None: + # Secondary index hit: gather the candidates, then let the UNCHANGED + # filter apply every remaining predicate to that small frame. + first_nodes = _filter_frame( + take_rows(nodes, prop_rows, engine), first_filter, engine, + ) + else: + hop_count = (len(ops) - 1) // 2 + if int(nodes.shape[0]) >= hop_count * int(edges.shape[0]): + return None + first_nodes = _filter_frame(nodes, first_filter, engine) first_alias = first_op._name alias_frames: Dict[str, DataFrameT] = {} diff --git a/graphistry/compute/gfql/index/build.py b/graphistry/compute/gfql/index/build.py index 344c4c9df4..9657463342 100644 --- a/graphistry/compute/gfql/index/build.py +++ b/graphistry/compute/gfql/index/build.py @@ -11,7 +11,7 @@ from graphistry.Engine import Engine from graphistry.compute.typing import DataFrameT from .engine_arrays import array_namespace, col_to_array -from .registry import AdjacencyIndex, NodeIdIndex, frame_fingerprint +from .registry import AdjacencyIndex, NodeIdIndex, NodePropIndex, frame_fingerprint from .types import AdjacencyIndexKind, ArrayLike, ArrayNamespace @@ -99,3 +99,40 @@ def build_node_id_index( source_ref=cast(DataFrameT, nodes), n_nodes=n_keys, ) + + +def build_node_prop_index( + nodes: DataFrameT, + column: str, + engine: Engine, +) -> Optional[NodePropIndex]: + """Sorted property value -> node row positions (CSR), or None when unindexable. + + Duplicates are fine (CSR keeps every row per value) — this is the secondary + index, so the caller still applies the remaining predicates to the gathered + candidates. Declines (None) for anything whose ordering/equality is not + unambiguous under a vectorized ``searchsorted`` on BOTH backends: non-integer + dtypes (float NaN ordering, object/string on cupy) and null-bearing columns. + Widening that gate later is additive — a decline only means "scan", never a + wrong answer. + """ + xp, backend = array_namespace(engine) + try: + keys = col_to_array(nodes, column, engine) + except (AttributeError, KeyError, TypeError, ValueError): + return None + if getattr(getattr(keys, "dtype", None), "kind", None) not in ("i", "u"): + return None + unique_keys, group_offsets, row_positions = _csr_from_keys(keys, xp) + return NodePropIndex( + key_col=column, + keys_sorted=unique_keys, + group_offsets=group_offsets, + row_positions=row_positions, + backend=backend, + engine=engine, + fingerprint=frame_fingerprint(nodes, (column,), engine), + source_ref=cast(DataFrameT, nodes), + n_nodes=int(keys.shape[0]), + n_keys=int(unique_keys.shape[0]), + ) diff --git a/graphistry/compute/gfql/index/lookup.py b/graphistry/compute/gfql/index/lookup.py index 14fa469021..7406fbdc40 100644 --- a/graphistry/compute/gfql/index/lookup.py +++ b/graphistry/compute/gfql/index/lookup.py @@ -8,7 +8,7 @@ from typing import Tuple -from .registry import AdjacencyIndex, NodeIdIndex +from .registry import AdjacencyIndex, NodeIdIndex, NodePropIndex from .types import ArrayLike, ArrayNamespace @@ -93,3 +93,58 @@ def lookup_node_rows(index: NodeIdIndex, ids: ArrayLike, xp: ArrayNamespace) -> pos_clipped = xp.where(pos < U, pos, U - 1) hit = keys[pos_clipped] == f return index.row_positions[pos_clipped[hit]] + + +def lookup_prop_rows(index: NodePropIndex, values: ArrayLike, xp: ArrayNamespace) -> ArrayLike: + """values (backend array) -> node row positions of every row holding one of them. + + Same searchsorted + CSR range-expansion as the adjacency lookup, so a seed + predicate on an indexed property costs O(V_q log U + matches) instead of the + O(N) scan. Order is unspecified here; callers that need frame order sort. + """ + keys = index.keys_sorted + empty = index.row_positions[:0] + U = int(keys.shape[0]) + if U == 0 or int(values.shape[0]) == 0: + return empty + + v = values + if v.dtype != keys.dtype: + common = xp.promote_types(v.dtype, keys.dtype) + v = v.astype(common) + keys = keys.astype(common) + + pos = xp.searchsorted(keys, v) + pos_clipped = xp.where(pos < U, pos, U - 1) + hit = keys[pos_clipped] == v + pos_hit = pos_clipped[hit] + if int(pos_hit.shape[0]) == 0: + return empty + + start = index.group_offsets[pos_hit] + counts = index.group_offsets[pos_hit + 1] - start + total = int(counts.sum()) + if total == 0: + return empty + return index.row_positions[_expand_ranges(start, counts, total, xp)] + + +def prop_match_count(index: NodePropIndex, values: ArrayLike, xp: ArrayNamespace) -> int: + """How many rows ``lookup_prop_rows`` would return — the free selectivity + estimate the planner cost-gates on (CSR offsets only, no gather).""" + keys = index.keys_sorted + U = int(keys.shape[0]) + if U == 0 or int(values.shape[0]) == 0: + return 0 + v = values + if v.dtype != keys.dtype: + common = xp.promote_types(v.dtype, keys.dtype) + v = v.astype(common) + keys = keys.astype(common) + pos = xp.searchsorted(keys, v) + pos_clipped = xp.where(pos < U, pos, U - 1) + hit = keys[pos_clipped] == v + pos_hit = pos_clipped[hit] + if int(pos_hit.shape[0]) == 0: + return 0 + return int((index.group_offsets[pos_hit + 1] - index.group_offsets[pos_hit]).sum()) diff --git a/graphistry/compute/gfql/index/registry.py b/graphistry/compute/gfql/index/registry.py index b5c2754e24..799f97ca1c 100644 --- a/graphistry/compute/gfql/index/registry.py +++ b/graphistry/compute/gfql/index/registry.py @@ -21,9 +21,10 @@ EDGE_OUT_ADJ: AdjacencyIndexKind = "edge_out_adj" EDGE_IN_ADJ: AdjacencyIndexKind = "edge_in_adj" NODE_ID: IndexKind = "node_id" +NODE_PROP: IndexKind = "node_prop" ADJ_KINDS: Tuple[AdjacencyIndexKind, ...] = (EDGE_OUT_ADJ, EDGE_IN_ADJ) -ALL_KINDS: Tuple[IndexKind, ...] = (EDGE_OUT_ADJ, EDGE_IN_ADJ, NODE_ID) +ALL_KINDS: Tuple[IndexKind, ...] = (EDGE_OUT_ADJ, EDGE_IN_ADJ, NODE_ID, NODE_PROP) FrameFingerprint = Tuple[int, Tuple[str, ...], str] @@ -81,20 +82,74 @@ class NodeIdIndex: name: Optional[str] = None +@dataclass(frozen=True) +class NodePropIndex: + """Sorted node PROPERTY value -> node row positions (CSR, duplicates allowed). + + The secondary index: a seed predicate on a non-key column (``{id: 42}`` where + the graph's node id is some other column) otherwise costs a full node scan. + Unlike :class:`NodeIdIndex` this keeps ALL rows per key in CSR form, so + non-unique properties are indexable — the caller applies any residual + predicates to the gathered candidates, so results are identical either way. + """ + key_col: str + keys_sorted: ArrayLike # distinct values, ascending (len U) + group_offsets: ArrayLike # CSR offsets into row_positions (len U+1) + row_positions: ArrayLike # node row indices grouped by value (len N) + backend: IndexBackend + engine: Engine + fingerprint: FrameFingerprint = field(compare=False, default=(-1, (), "")) + source_ref: Optional[DataFrameT] = field(compare=False, default=None) + n_nodes: int = 0 + n_keys: int = 0 + name: Optional[str] = None + + @dataclass(frozen=True) class GfqlIndexRegistry: """Immutable kind -> index map. ``with_index`` / ``without`` return copies.""" indexes: Dict[IndexKind, Union[AdjacencyIndex, NodeIdIndex]] = field(default_factory=dict) + # Property indexes are keyed by COLUMN, not kind: a graph may carry several. + node_props: Dict[str, NodePropIndex] = field(default_factory=dict) def with_index(self, kind: IndexKind, index: Union[AdjacencyIndex, NodeIdIndex]) -> "GfqlIndexRegistry": new = dict(self.indexes) new[kind] = index - return GfqlIndexRegistry(new) + return GfqlIndexRegistry(new, dict(self.node_props)) + + def with_node_prop(self, column: str, index: "NodePropIndex") -> "GfqlIndexRegistry": + props = dict(self.node_props) + props[column] = index + return GfqlIndexRegistry(dict(self.indexes), props) + + def node_prop_cols(self) -> Tuple[str, ...]: + return tuple(sorted(self.node_props.keys())) + + def get_node_prop_valid( + self, column: str, df: Optional[DataFrameT], engine: Engine + ) -> Optional["NodePropIndex"]: + """The property index for ``column``, only while it still matches the live + frame + engine (same identity/fingerprint contract as ``get_valid``).""" + idx = self.node_props.get(column) + if idx is None or df is None or idx.engine != engine: + return None + if idx.source_ref is not None and idx.source_ref is not df: + return None + if idx.fingerprint != frame_fingerprint(df, (column,), engine): + return None + return idx def without(self, kind: IndexKind) -> "GfqlIndexRegistry": + if kind == NODE_PROP: + return GfqlIndexRegistry(dict(self.indexes), {}) new = dict(self.indexes) new.pop(kind, None) - return GfqlIndexRegistry(new) + return GfqlIndexRegistry(new, dict(self.node_props)) + + def without_node_prop(self, column: str) -> "GfqlIndexRegistry": + props = dict(self.node_props) + props.pop(column, None) + return GfqlIndexRegistry(dict(self.indexes), props) def rebind_edges(self, new_edges: DataFrameT) -> "GfqlIndexRegistry": """Re-point the EDGE adjacency indexes' identity guard at ``new_edges``. @@ -134,7 +189,7 @@ def rebind_edges(self, new_edges: DataFrameT) -> "GfqlIndexRegistry": new[kind] = replace(idx, source_ref=new_edges) else: new.pop(kind, None) - return GfqlIndexRegistry(new) + return GfqlIndexRegistry(new, dict(self.node_props)) def get(self, kind: IndexKind) -> Optional[Union[AdjacencyIndex, NodeIdIndex]]: return self.indexes.get(kind) @@ -146,7 +201,7 @@ def kinds(self) -> Tuple[IndexKind, ...]: return cast(Tuple[IndexKind, ...], tuple(sorted(self.indexes.keys()))) def is_empty(self) -> bool: - return not self.indexes + return not self.indexes and not self.node_props def get_valid(self, kind: IndexKind, df: DataFrameT, cols: Tuple[str, ...], engine: Engine) -> Optional[Union[AdjacencyIndex, NodeIdIndex]]: """Return the index for ``kind`` only if its fingerprint still matches the @@ -166,7 +221,7 @@ def get_valid(self, kind: IndexKind, df: DataFrameT, cols: Tuple[str, ...], engi return idx -def index_nbytes(idx: Union[AdjacencyIndex, NodeIdIndex]) -> int: +def index_nbytes(idx: Union[AdjacencyIndex, NodeIdIndex, "NodePropIndex"]) -> int: """Approximate resident memory of an index's sidecar arrays (bytes).""" total = 0 for attr in ("keys_sorted", "group_offsets", "row_positions", "other_values"): diff --git a/graphistry/compute/gfql/index/types.py b/graphistry/compute/gfql/index/types.py index 43a7fe722f..3d6faa2547 100644 --- a/graphistry/compute/gfql/index/types.py +++ b/graphistry/compute/gfql/index/types.py @@ -11,7 +11,7 @@ if TYPE_CHECKING: from graphistry.compute.predicates.ASTPredicate import ASTPredicate -IndexKind = Literal["edge_out_adj", "edge_in_adj", "node_id"] +IndexKind = Literal["edge_out_adj", "edge_in_adj", "node_id", "node_prop"] AdjacencyIndexKind = Literal["edge_out_adj", "edge_in_adj"] IndexBackend = Literal["numpy", "cupy"] HopDirection = Literal["forward", "reverse", "undirected"] diff --git a/graphistry/tests/compute/gfql/index/test_indexed_bindings.py b/graphistry/tests/compute/gfql/index/test_indexed_bindings.py index acfbd28166..b92e22492d 100644 --- a/graphistry/tests/compute/gfql/index/test_indexed_bindings.py +++ b/graphistry/tests/compute/gfql/index/test_indexed_bindings.py @@ -526,6 +526,160 @@ def record_filter(frame: Any, *args: Any, **kwargs: Any) -> Any: assert filtered_lengths[0] == 1 +# --- secondary (node property) index ----------------------------------------- +# CONNECTED_QUERY seeds on ``public``, which is NOT the graph's node-id binding +# (``id``), so without a property index the seed costs a full node scan. + + +def _prop_frames() -> Tuple[pd.DataFrame, pd.DataFrame]: + nodes, edges = _base_frames() + # ``grp`` repeats (4 rows per value): the duplicate-key case a node-id index + # cannot express but a CSR property index can. + nodes = nodes.assign(grp=(nodes["rank"] % 3).astype("int64")) + return nodes, edges + + +def _prop_graph(engine: str, columns: Sequence[str] = ("public",)) -> Any: + nodes, edges = _prop_frames() + return _graph_from_frames(engine, nodes, edges).gfql_index_node_props( + list(columns), engine=engine + ) + + +def _seed_filter_widths( + g: Any, query: Any, engine: str, monkeypatch: pytest.MonkeyPatch +) -> Tuple[Any, List[Dict[str, Any]], List[int]]: + """Run traced, recording how many rows each helper filter had to look at.""" + import graphistry.compute.gfql.index.bindings as indexed_bindings + + widths: List[int] = [] + original = indexed_bindings._filter_frame + + def record(frame: Any, *args: Any, **kwargs: Any) -> Any: + widths.append(int(frame.shape[0])) + return original(frame, *args, **kwargs) + + monkeypatch.setattr(indexed_bindings, "_filter_frame", record) + actual, steps = _trace_run(g, query, engine) + return actual, steps, widths + + +@pytest.mark.parametrize("engine", ENGINES) +def test_node_property_index_seeds_without_scanning( + engine: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + g = _prop_graph(engine) + with monkeypatch.context() as m: + expected = _run(g, CONNECTED_QUERY, engine, m, generic=True) + actual, steps, widths = _seed_filter_widths( + g, CONNECTED_QUERY, engine, monkeypatch + ) + _assert_result_exact(actual, expected, engine) + decisions = [s for s in steps if s.get("seam") == "connected_bindings"] + assert len(decisions) == 1 + _assert_decision(decisions[0], seam="connected_bindings", served=True) + assert widths and widths[0] == 1 # one indexed candidate, not the node table + + +@pytest.mark.parametrize("engine", ENGINES) +def test_node_property_index_absent_matches_indexed( + engine: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Same query, same answer, with and without the secondary index.""" + nodes, edges = _prop_frames() + plain = _graph_from_frames(engine, nodes, edges) + indexed = plain.gfql_index_node_props(["public"], engine=engine) + with monkeypatch.context() as m: + expected = _run(plain, CONNECTED_QUERY, engine, m, generic=True) + for g in (plain, indexed): + actual, _ = _trace_run(g, CONNECTED_QUERY, engine) + _assert_result_exact(actual, expected, engine) + + +def test_node_property_index_duplicate_values_match_scan( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A non-unique property still gathers EVERY matching row (CSR, not first-hit).""" + from graphistry.compute.ast import rows as rows_call + + g = _prop_graph("pandas", columns=("grp",)) + query = [n({"grp": 0}, name="a"), e_forward({"type": "A"}, name="r"), n(name="b"), rows_call()] + with monkeypatch.context() as m: + expected = _run(g, query, "pandas", m, generic=True) + actual, steps, widths = _seed_filter_widths(g, query, "pandas", monkeypatch) + _assert_result_exact(actual, expected, "pandas") + assert widths and widths[0] == 4 # every row with grp == 0, none of the others + assert [s for s in steps if s.get("seam") == "connected_bindings"] + + +@pytest.mark.parametrize("case", ["stale", "policy_off"]) +def test_node_property_index_lifecycle_falls_back( + case: str, + monkeypatch: pytest.MonkeyPatch, +) -> None: + g = _prop_graph("pandas") + index_policy = "off" if case == "policy_off" else "force" + if case == "stale": + g = g.nodes(_native_copy(g._nodes), "id") # rebind -> index treated as absent + with monkeypatch.context() as m: + expected = _run( + g, CONNECTED_QUERY, "pandas", m, generic=True, index_policy=index_policy + ) + actual, _ = _trace_run(g, CONNECTED_QUERY, "pandas", index_policy=index_policy) + _assert_result_exact(actual, expected, "pandas") + + +def test_node_property_index_declines_unindexable_columns() -> None: + from graphistry.compute.gfql.index import NODE_PROP, create_index, get_registry + + g = _prop_graph("pandas", columns=()) + for column in ("kind", "maybe"): # object dtype, float-with-null + with pytest.raises(ValueError): + create_index(g, NODE_PROP, column=column) + with pytest.raises(ValueError): + create_index(g, NODE_PROP, column="nosuch") + with pytest.raises(ValueError): + create_index(g, NODE_PROP) # column is required for a property index + # the convenience wrapper skips instead of raising, and indexes what it can + g2 = g.gfql_index_node_props(["kind", "maybe", "public"]) + assert get_registry(g2).node_prop_cols() == ("public",) + + +def test_node_property_index_prefers_the_most_selective_column( + monkeypatch: pytest.MonkeyPatch, +) -> None: + from graphistry.compute.ast import rows as rows_call + + g = _prop_graph("pandas", columns=("public", "grp")) + query = [ + n({"public": 100, "grp": 0}, name="a"), + e_forward({"type": "A"}, name="r"), + n(name="b"), + rows_call(), + ] + with monkeypatch.context() as m: + expected = _run(g, query, "pandas", m, generic=True) + actual, _, widths = _seed_filter_widths(g, query, "pandas", monkeypatch) + _assert_result_exact(actual, expected, "pandas") + assert widths and widths[0] == 1 # 'public' (1 match) beats 'grp' (4 matches) + + +def test_node_property_index_shows_and_drops() -> None: + from graphistry.compute.gfql.index import NODE_PROP, get_registry + + g = _prop_graph("pandas", columns=("public", "grp")) + shown = g.show_indexes() + props = shown[shown["kind"] == NODE_PROP] + assert sorted(props["key_col"]) == ["grp", "public"] + assert bool(props["valid"].all()) + assert sorted(props["n_keys"]) == [3, 12] + assert get_registry(g.drop_index(NODE_PROP, column="grp")).node_prop_cols() == ("public",) + assert get_registry(g.drop_index(NODE_PROP)).node_prop_cols() == () + assert get_registry(g.drop_index()).is_empty() + + @pytest.mark.parametrize("engine", ENGINES) def test_indexed_execution_is_pure( engine: str, From 189dcc738becf40beff47cab586dfa38ac4302f2 Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 25 Jul 2026 15:40:59 -0700 Subject: [PATCH 2/6] review: address manual review of the indexed-bindings stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manual review raised DRY/placement, functional-style, dynamic-typing, and test amplification concerns. Changes, by concern: DRY / placement - `bindings._concat` duplicated `graphistry.Engine.df_concat` (whose polars twin already accepts `ignore_index` for exactly this) — deleted, call site rewired. - `_estimate_join_rows`, `_join_state`, `_filter_oriented_endpoints` moved to `compute/dataframe/join.py`, where the other join/merge helpers live, and generalized to explicit column parameters instead of module-private constants: `estimate_inner_join_rows`, `path_ordered_expand_join`, `semijoin_by_column`. - `_lookup_degree` moved to `index/lookup.py` and now shares one CSR probe with the property lookup (`_csr_hit_positions` / `csr_match_count` / `csr_gather_rows`), which were three copies of searchsorted + membership. - The helper count in `bindings.py` drops from 17 to 12, all index-specific. Functional style - Copy-then-mutate becomes a single batched `.assign(...)`: `_frame_with_positions`, `_with_marker`, `_orient_edges`, the seed state, and the frame_ops edge-marker synthesis. Also dropped a redundant `.copy()` before `.rename()` and after a boolean mask (both already copy). The rule is now written down in `agents/skills/review/SKILL.md` so review catches it next time. Dynamic typing - The three smuggled `_gfql_indexed_bindings_*` attributes collapse into ONE typed `IndexedBindingsHandoff` dataclass in `index/handoff.py`, which owns the only attribute access for it (`attach_handoff` / `set_handoff` / `read_handoff` / `clear_handoff`) and answers `serves()` / `declined()` instead of callers re-deriving plan equality. Call sites in `chain.py`, the polars chain, both row pipelines, and `frame_ops` are now typed calls. - `_integer_index` takes the index union and narrows with `isinstance` rather than probing `getattr(..., "keys_sorted", None)`; alias/filter access narrows on `ASTNode`/`ASTEdge` instead of `getattr(op, "_name", None)`. - Added `getattr`/`setattr` across the stack: 31 -> 19, of which 5 are inside the one module that owns the attribute; the rest are `threading.local` probes (the correct idiom) and pre-existing `_gfql_rows_*` plumbing this stack did not invent. Added `# type: ignore`: 18 -> 9, and `mypy --warn-unused-ignores` reports none of the remaining ones is unnecessary. Correctness fix found by the new tests - `_connected_decline_reason` re-derived the cost gate locally, so on engines with a tighter crossover (0.02 vs pandas' 0.5) a structurally-unsupported shape was reported as `cost_frontier`. It now asks the real gate with cost disabled: a serve there means cost was the only obstacle, anything else is structural. One source of truth, accurate on every engine. Tests - The unsupported-shape decline matrix now runs on pandas/polars/cuDF (it was pandas-only) — that is what caught the mislabelling above. - New negative matrix for the polars early gate: rows-with-source, alias-endpoints, alias-prefilters, and seeded re-entry must not engage, plus a plan-mismatch and unnamed-middle case, and a positive assertion so the suite cannot go vacuous. - Property-index cost gate is now exercised under `use` (every other case used `force`, which skips the gate) with the crossover pinned via the public knob. - Property index reachable from all three surfaces: Cypher DDL grammar accepts `node_prop`, and the JSON wire protocol handles column-keyed create/drop/name resolution (it previously raised "no resident index" for a resident property index and dropped every column on a targeted drop). - `docs/source/gfql/indexing.rst` documents the new kind and `gfql_index_node_props`. Validated on dgx-spark: full CPU `graphistry/tests/compute` 6260 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 70 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 --- agents/skills/review/SKILL.md | 14 + docs/source/gfql/indexing.rst | 30 +++ graphistry/compute/chain.py | 31 +-- graphistry/compute/dataframe/join.py | 121 +++++++++ graphistry/compute/gfql/index/__init__.py | 5 +- graphistry/compute/gfql/index/api.py | 7 + graphistry/compute/gfql/index/bindings.py | 251 +++++------------- graphistry/compute/gfql/index/cypher_ddl.py | 2 +- graphistry/compute/gfql/index/handoff.py | 76 ++++++ graphistry/compute/gfql/index/lookup.py | 91 ++++--- graphistry/compute/gfql/index/wire.py | 40 ++- .../compute/gfql/lazy/engine/polars/chain.py | 28 +- .../gfql/lazy/engine/polars/row_pipeline.py | 21 +- graphistry/compute/gfql/row/frame_ops.py | 25 +- graphistry/compute/gfql/row/pipeline.py | 32 +-- .../tests/compute/gfql/index/test_index.py | 40 +++ .../gfql/index/test_indexed_bindings.py | 134 +++++++++- 17 files changed, 635 insertions(+), 313 deletions(-) create mode 100644 graphistry/compute/gfql/index/handoff.py diff --git a/agents/skills/review/SKILL.md b/agents/skills/review/SKILL.md index 64b3c1f184..376f95e591 100644 --- a/agents/skills/review/SKILL.md +++ b/agents/skills/review/SKILL.md @@ -211,6 +211,20 @@ GFQL is vectorization-first and pure-functional. The row pipeline runs on pandas Vectorized mutation (`df.loc[mask, col] = v`) is still mutation — prefer `df.assign(...)`. +**Batch the assign** (SUGGESTION; IMPORTANT on a hot row path): copy-then-mutate +(`out = df.copy()` followed by one or more `out[col] = ...`) should be a single +`df.assign(col_a=..., col_b=...)`. It is the functional phrasing the rest of the +codebase uses, it drops the explicit copy (assign already returns a new frame), and +one assign builds the result once instead of copying and then writing column by +column. + +| Pattern | Fix | +|---|---| +| `out = df.copy()` + `out[c] = v` | `df.assign(c=v)` | +| `out = df.copy()` + several `out[c] = v` | one `df.assign(c1=v1, c2=v2)` | +| `df.copy().rename(columns=...)` | `df.rename(columns=...)` — rename already copies | +| `out = df[mask].copy()` with no later mutation | `df[mask]` — masking already copies | + **cuDF compatibility** — flag IMPORTANT unless noted: | Pattern | Fix | diff --git a/docs/source/gfql/indexing.rst b/docs/source/gfql/indexing.rst index 457dc28064..d63dc535db 100644 --- a/docs/source/gfql/indexing.rst +++ b/docs/source/gfql/indexing.rst @@ -39,6 +39,12 @@ carrying them: - Sorted node-id lookup: seed-row and endpoint materialization become positional gathers instead of ``O(N)`` scans. Requires unique node ids — ``gfql_index_all()`` silently skips it otherwise (adjacency is still built). + * - ``node_prop`` + - Sorted lookup on a node **property** column (a secondary index): a seed + predicate like ``MATCH (m {id: 42})`` on a column that is not the node-id + binding becomes a positional gather instead of an ``O(N)`` scan. Duplicate + values are fine (all matching rows are gathered). Integer columns without + nulls only — anything else declines to the scan. Opt-in per column. They are **sidecars over row positions**: your ``.edges`` / ``.nodes`` frames are never reordered or copied, and the resident footprint is visible per index via @@ -93,12 +99,36 @@ API): g = g.gfql_index_all() # out+in adjacency + node_id (the one-liner) g = g.gfql_index_edges("forward") # or just one direction: 'forward'|'reverse'|'both' g = g.create_index("edge_out_adj") # or one kind: 'edge_out_adj'|'edge_in_adj'|'node_id' + g = g.gfql_index_node_props(["id"]) # secondary indexes on node property columns g.show_indexes() # pandas DataFrame: kind, engine, n_keys, nbytes, valid g = g.drop_index() # drop all (or drop_index("edge_out_adj")) Unlike ``gfql_index_all()``, an explicit ``create_index("node_id")`` **raises** on non-unique node ids rather than skipping. +Seeding on a property (secondary index) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``node_id`` index covers the column bound as the node id. A query that seeds on +a *different* column — a business key such as ``MATCH (m {id: 42})`` when the graph +is keyed by something else — otherwise scans the whole node table to find its seed. +``node_prop`` indexes that column instead: + +.. code-block:: python + + g = g.gfql_index_all().gfql_index_node_props(["id"]) # skips unindexable columns + g = g.create_index("node_prop", column="id") # or one column, raising if it cannot + + # equivalently over the Cypher DDL / JSON surfaces + g.gfql('CREATE GFQL INDEX FOR node_prop ON id') + g = g.drop_index("node_prop", column="id") # or drop_index("node_prop") for all + +When several indexed columns appear in one seed predicate, the planner gathers on the +**most selective** one (estimated for free from the index's own offsets) and applies +the remaining predicates to those candidates, so results never depend on which index +happens to be resident. As with every kind, a missing, stale, or cost-gated-out index +simply falls back to the scan. + What uses the index today ------------------------- diff --git a/graphistry/compute/chain.py b/graphistry/compute/chain.py index 5f4a927662..ce319f45f8 100644 --- a/graphistry/compute/chain.py +++ b/graphistry/compute/chain.py @@ -606,6 +606,10 @@ def _handle_boundary_calls( logger.debug('Boundary call pattern detected: prefix=%s, middle=%s, suffix=%s', len(prefix), len(middle), len(suffix)) + # Function-scope import: `gfql.index` transitively imports this module, so a + # module-scope import would be a cycle. + from .gfql.index.handoff import IndexedBindingsHandoff, attach_handoff, set_handoff + g_temp = self indexed_middle_state = None indexed_middle_attempted = False @@ -641,7 +645,8 @@ def _handle_boundary_calls( suffix[0].params.get("binding_ops") == serialize_binding_ops(middle) or ( suffix[0].params.get("binding_ops") is None - and any(getattr(op, "_name", None) is not None for op in middle) + and any(op._name is not None for op in middle + if isinstance(op, (ASTNode, ASTEdge))) ) ) and suffix[0].params.get("source") is None @@ -662,21 +667,15 @@ def _handle_boundary_calls( engine=engine_concrete, ) if indexed_middle_state is not None: - g_temp = self.bind() - setattr( - g_temp, - "_gfql_indexed_bindings_state", - (serialize_binding_ops(middle), indexed_middle_state), - ) - setattr( - g_temp, - "_gfql_rows_edge_aliases", - tuple( + g_temp = attach_handoff(self, IndexedBindingsHandoff( + binding_ops=serialize_binding_ops(middle), + state=indexed_middle_state, + edge_aliases=tuple( op._name for op in middle if isinstance(op, ASTEdge) and isinstance(op._name, str) ), - ) + )) if middle and indexed_middle_state is None: logger.debug('Executing middle operations: %s', middle) @@ -691,11 +690,9 @@ def _handle_boundary_calls( ) if indexed_middle_attempted and indexed_middle_state is None: - setattr( - g_temp, - "_gfql_indexed_bindings_declined_ops", - serialize_binding_ops(middle), - ) + set_handoff(g_temp, IndexedBindingsHandoff( + binding_ops=serialize_binding_ops(middle), + )) if suffix: logger.debug('Executing boundary suffix calls: %s', suffix) diff --git a/graphistry/compute/dataframe/join.py b/graphistry/compute/dataframe/join.py index dc8fc97aa2..62dc13336a 100644 --- a/graphistry/compute/dataframe/join.py +++ b/graphistry/compute/dataframe/join.py @@ -294,3 +294,124 @@ def _uniq(df: DataFrameT, col: str, name: str) -> DataFrameT: if right_keep: right_eval = right_eval[list(right_keep)] return left_eval, right_eval, mid_values + + +def estimate_inner_join_rows( + left: DataFrameT, + right: DataFrameT, + *, + left_on: str, + right_on: str, + engine: Engine, +) -> int: + """Rows an inner join WOULD emit, without materializing it. + + Sum over matched keys of (left count * right count) — a group-by on each side + instead of the join itself, so a planner can cost-gate a hop before paying for + it. + """ + if len(left) == 0 or len(right) == 0: + return 0 + left_n, right_n = "__gfql_join_left_n__", "__gfql_join_right_n__" + if engine in POLARS_ENGINES: + import polars as pl + + left_counts = left.group_by(left_on).len().rename({"len": left_n}) # type: ignore[operator] + right_counts = right.group_by(right_on).len().rename({"len": right_n}) # type: ignore[operator] + value = ( + left_counts.join(right_counts, left_on=left_on, right_on=right_on, how="inner") + .select((pl.col(left_n) * pl.col(right_n)).sum()) + .item() + ) + return 0 if value is None else int(value) + + left_counts = left.groupby(left_on, sort=False).size().reset_index() + left_counts.columns = [left_on, left_n] + right_counts = right.groupby(right_on, sort=False).size().reset_index() + right_counts.columns = [right_on, right_n] + counts = left_counts.merge( + right_counts, left_on=left_on, right_on=right_on, how="inner", sort=False, + ) + if len(counts) == 0: + return 0 + return int((counts[left_n] * counts[right_n]).sum()) + + +def path_ordered_expand_join( + state: DataFrameT, + step: DataFrameT, + *, + current_col: str, + from_col: str, + to_col: str, + path_order_col: str, + tiebreak_cols: Sequence[str], + alias: Optional[str], + engine: Engine, +) -> DataFrameT: + """Expand a path-bag by one step, preserving (path, tiebreak...) row order. + + ``state[current_col]`` joins ``step[from_col]``; ``step[to_col]`` becomes the new + ``current_col``. A synthetic ``path_order_col`` pins the incoming row order so the + result is deterministic under any engine's join, and the bookkeeping columns are + dropped on the way out. ``alias``, when given, also exposes the new current value + under that name. + """ + drop_after = [from_col, path_order_col, *tiebreak_cols] + if engine in POLARS_ENGINES: + import polars as pl + + joined = ( + state.with_row_index(path_order_col) # type: ignore[operator] + .join(step, left_on=current_col, right_on=from_col, how="inner") + .sort([path_order_col, *tiebreak_cols]) + .drop(current_col) + .rename({to_col: current_col}) + ) + if isinstance(alias, str): + joined = joined.with_columns(pl.col(current_col).alias(alias)) + return cast( + DataFrameT, + joined.drop([col for col in drop_after if col in joined.columns]), + ) + + import numpy as np + + out = state.assign(**{path_order_col: np.arange(len(state))}).merge( + step, left_on=current_col, right_on=from_col, how="inner", sort=False, + ) + if len(out): + sort_cols = [path_order_col, *tiebreak_cols] + out = ( + out.sort_values(sort_cols, kind="stable") if engine == Engine.PANDAS + else out.sort_values(sort_cols) + ) + out = out.drop(columns=[current_col]).rename(columns={to_col: current_col}) + if isinstance(alias, str): + out = out.assign(**{alias: out[current_col]}) + return cast( + DataFrameT, + out.drop(columns=[col for col in drop_after if col in out.columns]), + ) + + +def semijoin_by_column( + frame: DataFrameT, + keys: DataFrameT, + *, + left_on: str, + right_on: str, + engine: Engine, +) -> DataFrameT: + """Rows of ``frame`` whose ``left_on`` value appears in ``keys[right_on]``.""" + if engine in POLARS_ENGINES: + return cast( + DataFrameT, + frame.join( # type: ignore[call-arg] + keys.select(right_on).unique(), # type: ignore[operator] + left_on=left_on, + right_on=right_on, + how="semi", # type: ignore[arg-type] + ), + ) + return cast(DataFrameT, frame[frame[left_on].isin(keys[right_on])]) diff --git a/graphistry/compute/gfql/index/__init__.py b/graphistry/compute/gfql/index/__init__.py index 2482d9de04..4a44c58312 100644 --- a/graphistry/compute/gfql/index/__init__.py +++ b/graphistry/compute/gfql/index/__init__.py @@ -18,7 +18,8 @@ from .api import ( create_index, drop_index, show_indexes, gfql_index_edges, gfql_index_all, gfql_index_node_props, - get_registry, set_registry, get_index_policy, maybe_index_hop, index_name, index_trace, + get_registry, set_registry, get_index_policy, with_index_policy, maybe_index_hop, + index_name, index_trace, ) from .wire import ( CreateIndex, DropIndex, ShowIndexes, IndexOp, apply_index_op, index_op_from_json, @@ -35,7 +36,7 @@ "EDGE_OUT_ADJ", "EDGE_IN_ADJ", "NODE_ID", "NODE_PROP", "ADJ_KINDS", "ALL_KINDS", "AdjacencyIndex", "NodeIdIndex", "NodePropIndex", "create_index", "drop_index", "show_indexes", "gfql_index_edges", - "gfql_index_all", "gfql_index_node_props", "get_registry", "set_registry", "get_index_policy", "maybe_index_hop", "index_name", + "gfql_index_all", "gfql_index_node_props", "get_registry", "with_index_policy", "set_registry", "get_index_policy", "maybe_index_hop", "index_name", "index_trace", "CreateIndex", "DropIndex", "ShowIndexes", "IndexOp", "apply_index_op", "index_op_from_json", "is_index_op", "is_index_op_json", diff --git a/graphistry/compute/gfql/index/api.py b/graphistry/compute/gfql/index/api.py index 7251d4cf36..6d35dd121e 100644 --- a/graphistry/compute/gfql/index/api.py +++ b/graphistry/compute/gfql/index/api.py @@ -112,6 +112,13 @@ def get_index_policy(g: Plottable) -> IndexPolicy: return cast(IndexPolicy, getattr(g, POLICY_ATTR, "use")) +def with_index_policy(g: Plottable, policy: IndexPolicy) -> Plottable: + """A copy of ``g`` carrying ``policy`` (the attribute lives only in this module).""" + out = g.bind() + setattr(out, POLICY_ATTR, policy) + return out + + def _attach(g: Plottable, registry: GfqlIndexRegistry) -> Plottable: res = copy.copy(g) setattr(res, REGISTRY_ATTR, registry) diff --git a/graphistry/compute/gfql/index/bindings.py b/graphistry/compute/gfql/index/bindings.py index 5dbeee597b..0cb5487b21 100644 --- a/graphistry/compute/gfql/index/bindings.py +++ b/graphistry/compute/gfql/index/bindings.py @@ -10,9 +10,9 @@ from dataclasses import dataclass from numbers import Integral -from typing import Any, Dict, List, Mapping, Optional, Sequence, Tuple, cast +from typing import Any, Dict, List, Mapping, Optional, Sequence, Tuple, Union, cast -from graphistry.Engine import Engine +from graphistry.Engine import Engine, df_concat from graphistry.Plottable import Plottable from graphistry.compute.typing import DataFrameT @@ -21,10 +21,18 @@ _trace_active, get_index_policy, get_registry, + with_index_policy, ) +from graphistry.compute.dataframe.join import ( + estimate_inner_join_rows, + path_ordered_expand_join, + semijoin_by_column, +) + from .cost import cost_gate_frac from .engine_arrays import array_namespace, col_to_array, take_rows from .lookup import ( + lookup_degree, lookup_edge_rows, lookup_node_rows, lookup_prop_rows, @@ -37,6 +45,7 @@ AdjacencyIndex, GfqlIndexRegistry, NodeIdIndex, + NodePropIndex, ) from .traverse import _indices_for_direction @@ -88,13 +97,16 @@ def _simple_filter_dict(value: Any, *, allow_empty: bool = True) -> bool: ) -def _integer_index(index: Any) -> bool: - key_dtype = getattr(getattr(index, "keys_sorted", None), "dtype", None) - other_dtype = getattr(getattr(index, "other_values", None), "dtype", None) - key_ok = getattr(key_dtype, "kind", None) in ("i", "u") - if isinstance(index, NodeIdIndex): +def _integer_index(index: Union[AdjacencyIndex, NodeIdIndex, NodePropIndex]) -> bool: + """Whether an index's keys (and, for adjacency, its neighbor ids) are integral. + + The vectorized gather promotes dtypes rather than narrowing, so non-integral + keys are declined rather than risking a lossy compare. + """ + key_ok = index.keys_sorted.dtype.kind in ("i", "u") + if not isinstance(index, AdjacencyIndex): return key_ok - return key_ok and getattr(other_dtype, "kind", None) in ("i", "u") + return key_ok and index.other_values.dtype.kind in ("i", "u") def _filter_compatible(frame: DataFrameT, filter_dict: Optional[dict]) -> bool: @@ -149,23 +161,7 @@ def _with_marker(frame: DataFrameT, name: Optional[str], engine: Engine) -> Data if engine == Engine.POLARS: # Native Polars intentionally omits pandas' alias-marker residue. return frame - out = frame.copy() - out[name] = True - return cast(DataFrameT, out) - - -def _concat(frames: Sequence[DataFrameT], engine: Engine) -> DataFrameT: - if engine == Engine.POLARS: - import polars as pl - - return cast(DataFrameT, pl.concat(list(frames), how="vertical")) # type: ignore[type-var] - if engine == Engine.CUDF: - import cudf # type: ignore - - return cast(DataFrameT, cudf.concat(list(frames), ignore_index=True)) - import pandas as pd - - return cast(DataFrameT, pd.concat(list(frames), ignore_index=True)) + return cast(DataFrameT, frame.assign(**{name: True})) def _frame_with_positions( @@ -179,9 +175,7 @@ def _frame_with_positions( DataFrameT, frame.with_columns(pl.Series(_EDGE_ORD, np.asarray(positions))), # type: ignore[operator] ) - out = frame.copy() - out[_EDGE_ORD] = positions - return cast(DataFrameT, out) + return cast(DataFrameT, frame.assign(**{_EDGE_ORD: positions})) def _orient_edges( @@ -217,9 +211,9 @@ def one(from_col: str, to_col: str, orient: int) -> DataFrameT: ) else: - work = gathered.copy() + work = gathered if isinstance(alias, str): - work[alias] = True + work = gathered.assign(**{alias: True}) payload = payload + [alias] renames = {col: f"{alias}.{col}" for col in payload} else: @@ -229,153 +223,25 @@ def one(from_col: str, to_col: str, orient: int) -> DataFrameT: out = work.rename(columns={from_col: _FROM, to_col: _TO}) if renames: out = out.rename(columns=renames) - out[_ORIENT_ORD] = orient - return cast(DataFrameT, out) + return cast(DataFrameT, out.assign(**{_ORIENT_ORD: orient})) if direction == "undirected": forward = one(src, dst, 0) reverse = one(dst, src, 1) if engine == Engine.POLARS: reverse = reverse.select(forward.columns) # type: ignore[operator] - return _concat([forward, reverse], engine) + return cast(DataFrameT, df_concat(engine)([forward, reverse], ignore_index=True)) if direction == "reverse": return one(dst, src, 0) return one(src, dst, 0) -def _estimate_join_rows( - state: DataFrameT, oriented: DataFrameT, engine: Engine, -) -> int: - if len(state) == 0 or len(oriented) == 0: - return 0 - if engine == Engine.POLARS: - import polars as pl - - left = state.group_by(_CURRENT).len().rename({"len": _LEFT_N}) # type: ignore[operator] - right = oriented.group_by(_FROM).len().rename({"len": _RIGHT_N}) # type: ignore[operator] - value = ( - left.join(right, left_on=_CURRENT, right_on=_FROM, how="inner") - .select((pl.col(_LEFT_N) * pl.col(_RIGHT_N)).sum()) - .item() - ) - return 0 if value is None else int(value) - - left = state.groupby(_CURRENT, sort=False).size().reset_index() - left.columns = [_CURRENT, _LEFT_N] - right = oriented.groupby(_FROM, sort=False).size().reset_index() - right.columns = [_FROM, _RIGHT_N] - counts = left.merge( - right, left_on=_CURRENT, right_on=_FROM, how="inner", sort=False, - ) - if len(counts) == 0: - return 0 - return int((counts[_LEFT_N] * counts[_RIGHT_N]).sum()) - - -def _join_state( - state: DataFrameT, - oriented: DataFrameT, - *, - node_alias: Optional[str], - engine: Engine, -) -> DataFrameT: - if engine == Engine.POLARS: - import polars as pl - - joined = ( - state.with_row_index(_PATH_ORD) # type: ignore[operator] - .join(oriented, left_on=_CURRENT, right_on=_FROM, how="inner") - .sort([_PATH_ORD, _ORIENT_ORD, _EDGE_ORD]) - .drop(_CURRENT) - .rename({_TO: _CURRENT}) - ) - if isinstance(node_alias, str): - joined = joined.with_columns(pl.col(_CURRENT).alias(node_alias)) - return cast( - DataFrameT, - joined.drop([ - col for col in (_FROM, _PATH_ORD, _EDGE_ORD, _ORIENT_ORD) - if col in joined.columns - ]), - ) - - out = state.copy() - xp, _ = array_namespace(engine) - out[_PATH_ORD] = xp.arange(len(out)) # type: ignore[call-overload] - out = out.merge( - oriented, left_on=_CURRENT, right_on=_FROM, how="inner", sort=False, - ) - if len(out): - if engine == Engine.PANDAS: - out = out.sort_values( - [_PATH_ORD, _ORIENT_ORD, _EDGE_ORD], kind="stable", - ) - else: - out = out.sort_values([_PATH_ORD, _ORIENT_ORD, _EDGE_ORD]) - out = out.drop(columns=[_CURRENT]).rename(columns={_TO: _CURRENT}) - if isinstance(node_alias, str): - out[node_alias] = out[_CURRENT] - return cast( - DataFrameT, - out.drop( - columns=[ - col for col in (_FROM, _PATH_ORD, _EDGE_ORD, _ORIENT_ORD) - if col in out.columns - ], - ), - ) - - def _policy_is_active() -> bool: from graphistry.compute.gfql.call.executor import _thread_local return getattr(_thread_local, "policy", None) is not None -def _filter_oriented_endpoints( - oriented: DataFrameT, - next_nodes: DataFrameT, - node_id: str, - engine: Engine, -) -> DataFrameT: - if engine == Engine.POLARS: - return cast( - DataFrameT, - oriented.join( # type: ignore[call-arg] - next_nodes.select(node_id).unique(), # type: ignore[operator] - left_on=_TO, - right_on=node_id, - how="semi", # type: ignore[arg-type] - ), - ) - return cast( - DataFrameT, - oriented[oriented[_TO].isin(next_nodes[node_id])].copy(), - ) - - - -def _lookup_degree(index: AdjacencyIndex, frontier: Any, xp: Any) -> int: - """Native O(frontier) degree estimate before CSR range expansion.""" - keys = index.keys_sorted - if int(keys.shape[0]) == 0 or int(frontier.shape[0]) == 0: - return 0 - values = frontier - if values.dtype != keys.dtype: - common = xp.promote_types(values.dtype, keys.dtype) - values = values.astype(common) - keys = keys.astype(common) - positions = xp.searchsorted(keys, values) - clipped = xp.where( - positions < keys.shape[0], positions, keys.shape[0] - 1, - ) - hits = clipped[keys[clipped] == values] - if int(hits.shape[0]) == 0: - return 0 - counts = index.group_offsets[hits + 1] - index.group_offsets[hits] - return int(counts.sum()) - - def _seed_rows_via_property_index( registry: GfqlIndexRegistry, nodes: DataFrameT, @@ -456,9 +322,8 @@ def _try_indexed_connected_bindings_state( node_id, src, dst = str(node_id), str(src), str(dst) aliases = [ - getattr(op, "_name", None) - for op in ops - if isinstance(getattr(op, "_name", None), str) + op._name for op in ops + if isinstance(op, (ASTNode, ASTEdge)) and isinstance(op._name, str) ] if ( len(aliases) != len(set(aliases)) @@ -590,9 +455,9 @@ def _try_indexed_connected_bindings_state( if isinstance(first_alias, str): state = state.with_columns(pl.col(_CURRENT).alias(first_alias)) else: - state = first_nodes[[node_id]].copy().rename(columns={node_id: _CURRENT}) + state = first_nodes[[node_id]].rename(columns={node_id: _CURRENT}) if isinstance(first_alias, str): - state[first_alias] = state[_CURRENT] + state = state.assign(**{first_alias: state[_CURRENT]}) estimated_rows = int(state.shape[0]) policy = get_index_policy(base_graph) @@ -609,7 +474,7 @@ def _try_indexed_connected_bindings_state( if int(frontier.shape[0]) >= threshold: return None gather_estimate = sum( - _lookup_degree(index, frontier, xp) for index in edge_indexes + lookup_degree(index, frontier, xp) for index in edge_indexes ) if ( policy != "force" @@ -645,17 +510,24 @@ def _try_indexed_connected_bindings_state( next_nodes = take_rows(nodes, node_rows, engine) next_nodes = _filter_frame(next_nodes, next_op.filter_dict, engine) next_alias_frame = _with_marker(next_nodes, next_op._name, engine) - oriented = _filter_oriented_endpoints( - oriented, next_nodes, node_id, engine, + oriented = semijoin_by_column( + oriented, next_nodes, left_on=_TO, right_on=node_id, engine=engine, ) - estimated_rows = _estimate_join_rows(state, oriented, engine) + estimated_rows = estimate_inner_join_rows( + state, oriented, left_on=_CURRENT, right_on=_FROM, engine=engine, + ) if policy != "force" and estimated_rows > 0 and estimated_rows >= n_edges: return None - state = _join_state( + state = path_ordered_expand_join( state, oriented, - node_alias=next_op._name, + current_col=_CURRENT, + from_col=_FROM, + to_col=_TO, + path_order_col=_PATH_ORD, + tiebreak_cols=(_ORIENT_ORD, _EDGE_ORD), + alias=next_op._name, engine=engine, ) if isinstance(next_op._name, str): @@ -677,7 +549,7 @@ def _connected_decline_reason( alias_prefilters: Optional[Any], ) -> str: """Classify a completed safe decline without changing canonical behavior.""" - from graphistry.compute.ast import ASTEdge + from graphistry.compute.ast import ASTEdge, ASTNode if engine not in (Engine.PANDAS, Engine.CUDF, Engine.POLARS): return "unsupported_engine" @@ -711,21 +583,19 @@ def _connected_decline_reason( if not _integer_index(valid): return "unsupported_dtype" - if get_index_policy(base_graph) != "force" and ops: - first_filter = getattr(ops[0], "filter_dict", None) - if isinstance(first_filter, Mapping) and first_filter: - first_nodes = _filter_frame(nodes, cast(dict, first_filter), engine) - frontier_n = int(first_nodes.shape[0]) - adjacency = [ - cast(AdjacencyIndex, valid) - for kind, frame, columns in required - for valid in [registry.get_valid(kind, frame, columns, engine)] - if kind != NODE_ID and valid is not None - ] - if adjacency and frontier_n >= cost_gate_frac(engine) * min( - index.n_keys for index in adjacency - ): - return "cost_frontier" + if get_index_policy(base_graph) != "force": + # Was cost the ONLY thing in the way? Re-run the real gate with the cost + # checks disabled rather than re-deriving them here: a second copy of the + # thresholds drifts, and on engines with a tighter gate it mislabelled + # structurally-unsupported shapes as "cost_frontier". Trace-only path. + if _try_indexed_connected_bindings_state( + with_index_policy(base_graph, "force"), + ops, + engine=engine, + start_nodes=start_nodes, + alias_prefilters=alias_prefilters, + ) is not None: + return "cost_frontier" return "unsupported_shape" @@ -738,8 +608,13 @@ def try_indexed_connected_bindings_state( alias_prefilters: Optional[Any] = None, ) -> Optional[IndexedBindingsState]: """Attempt the indexed path; only explicit safe declines fall through.""" + from graphistry.compute.ast import ASTNode + hop_count = max(0, (len(ops) - 1) // 2) - first_filter = getattr(ops[0], "filter_dict", None) if ops else None + first_op_any = ops[0] if ops else None + first_filter = ( + first_op_any.filter_dict if isinstance(first_op_any, ASTNode) else None + ) node_id = base_graph._node public_seed_scan = not ( isinstance(first_filter, Mapping) diff --git a/graphistry/compute/gfql/index/cypher_ddl.py b/graphistry/compute/gfql/index/cypher_ddl.py index e289a93e49..b1c03f000a 100644 --- a/graphistry/compute/gfql/index/cypher_ddl.py +++ b/graphistry/compute/gfql/index/cypher_ddl.py @@ -19,7 +19,7 @@ from .types import IndexKind from .wire import CreateIndex, DropIndex, ShowIndexes, IndexOp -_KIND = r"(?Pedge_out_adj|edge_in_adj|node_id)" +_KIND = r"(?Pedge_out_adj|edge_in_adj|node_id|node_prop)" _CREATE_PATTERN = ( r"^\s*CREATE\s+GFQL\s+INDEX\s+(?:(?P[A-Za-z_]\w*)\s+)?FOR\s+" + _KIND diff --git a/graphistry/compute/gfql/index/handoff.py b/graphistry/compute/gfql/index/handoff.py new file mode 100644 index 0000000000..35e8b45275 --- /dev/null +++ b/graphistry/compute/gfql/index/handoff.py @@ -0,0 +1,76 @@ +"""The chain-boundary -> row-materializer handoff for indexed fixed-hop bindings. + +The boundary decides ONCE whether the resident indexes can serve a fixed-hop +pattern, then the canonical row materializer consumes that decision instead of +re-deriving it. This module owns the whole contract — the dataclass, the single +attribute it rides on, and the only attribute access — so callers stay typed and +no other module hand-rolls ``getattr``/``setattr`` for it. + +The attribute rides on a Plottable because that is how the row pipeline already +threads per-execution context (``_gfql_rows_base_graph``, ``_gfql_start_nodes``); +it is internal, always attached to an internal copy, and cleared before the +result is handed back to the caller. +""" +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple + +from graphistry.Engine import Engine +from graphistry.utils.json import JSONVal + +if TYPE_CHECKING: + from graphistry.Plottable import Plottable + from .bindings import IndexedBindingsState + + +HANDOFF_ATTR = "_gfql_indexed_bindings_handoff" + + +@dataclass(frozen=True) +class IndexedBindingsHandoff: + """One boundary decision about one exact plan. + + ``state is None`` records a DECLINE, which is as important as a serve: without + it the row materializer would re-attempt the same plan (and re-record the same + trace decision) after the canonical traversal already ran. + """ + + binding_ops: List[Dict[str, JSONVal]] + state: Optional["IndexedBindingsState"] = None + edge_aliases: Tuple[str, ...] = field(default=()) + + def serves(self, binding_ops: List[Dict[str, JSONVal]], engine: Engine) -> bool: + """True when this handoff carries a usable state for exactly this plan.""" + return ( + self.state is not None + and self.state.engine == engine + and self.binding_ops == binding_ops + ) + + def declined(self, binding_ops: List[Dict[str, JSONVal]]) -> bool: + """True when this exact plan was already tried and safely declined.""" + return self.state is None and self.binding_ops == binding_ops + + +def attach_handoff(g: "Plottable", handoff: IndexedBindingsHandoff) -> "Plottable": + """Return an internal copy of ``g`` carrying ``handoff`` (never mutates ``g``).""" + out = g.bind() + setattr(out, HANDOFF_ATTR, handoff) + return out + + +def set_handoff(g: Any, handoff: IndexedBindingsHandoff) -> None: + """Attach ``handoff`` to a graph the caller already owns.""" + setattr(g, HANDOFF_ATTR, handoff) + + +def read_handoff(g: Any) -> Optional[IndexedBindingsHandoff]: + """The boundary decision riding on ``g``, if any.""" + return getattr(g, HANDOFF_ATTR, None) + + +def clear_handoff(g: Any) -> None: + """Drop the handoff so it never escapes on a user-visible result.""" + if hasattr(g, HANDOFF_ATTR): + delattr(g, HANDOFF_ATTR) diff --git a/graphistry/compute/gfql/index/lookup.py b/graphistry/compute/gfql/index/lookup.py index 7406fbdc40..61e4b0eb3e 100644 --- a/graphistry/compute/gfql/index/lookup.py +++ b/graphistry/compute/gfql/index/lookup.py @@ -6,7 +6,7 @@ """ from __future__ import annotations -from typing import Tuple +from typing import Any, Tuple from .registry import AdjacencyIndex, NodeIdIndex, NodePropIndex from .types import ArrayLike, ArrayNamespace @@ -95,56 +95,67 @@ def lookup_node_rows(index: NodeIdIndex, ids: ArrayLike, xp: ArrayNamespace) -> return index.row_positions[pos_clipped[hit]] -def lookup_prop_rows(index: NodePropIndex, values: ArrayLike, xp: ArrayNamespace) -> ArrayLike: - """values (backend array) -> node row positions of every row holding one of them. +def _csr_hit_positions(keys: ArrayLike, values: ArrayLike, xp: ArrayNamespace) -> ArrayLike: + """Group positions in ``keys`` for the ``values`` that are actually present. - Same searchsorted + CSR range-expansion as the adjacency lookup, so a seed - predicate on an indexed property costs O(V_q log U + matches) instead of the - O(N) scan. Order is unspecified here; callers that need frame order sort. + The shared front half of every CSR probe: promote to a common dtype (never + narrow — an int64 id cast to int32 keys wraps and false-matches), searchsorted, + then verify membership. Returns the matched group positions. """ - keys = index.keys_sorted - empty = index.row_positions[:0] U = int(keys.shape[0]) if U == 0 or int(values.shape[0]) == 0: - return empty - - v = values - if v.dtype != keys.dtype: - common = xp.promote_types(v.dtype, keys.dtype) - v = v.astype(common) + return xp.zeros(0, dtype=xp.int64) + if values.dtype != keys.dtype: + common = xp.promote_types(values.dtype, keys.dtype) + values = values.astype(common) keys = keys.astype(common) + pos = xp.searchsorted(keys, values) + clipped = xp.where(pos < U, pos, U - 1) + return clipped[keys[clipped] == values] - pos = xp.searchsorted(keys, v) - pos_clipped = xp.where(pos < U, pos, U - 1) - hit = keys[pos_clipped] == v - pos_hit = pos_clipped[hit] - if int(pos_hit.shape[0]) == 0: - return empty - start = index.group_offsets[pos_hit] - counts = index.group_offsets[pos_hit + 1] - start +def _csr_group_sizes(index: Any, positions: ArrayLike) -> ArrayLike: + """Row count of each CSR group named by ``positions``.""" + return index.group_offsets[positions + 1] - index.group_offsets[positions] + + +def csr_match_count(index: Any, values: ArrayLike, xp: ArrayNamespace) -> int: + """How many rows a CSR gather of ``values`` would return — offsets only, no + gather. The planner's free selectivity/degree estimate.""" + positions = _csr_hit_positions(index.keys_sorted, values, xp) + if int(positions.shape[0]) == 0: + return 0 + return int(_csr_group_sizes(index, positions).sum()) + + +def csr_gather_rows(index: Any, values: ArrayLike, xp: ArrayNamespace) -> ArrayLike: + """Row positions of every row whose key is in ``values`` (CSR range expansion).""" + positions = _csr_hit_positions(index.keys_sorted, values, xp) + empty = index.row_positions[:0] + if int(positions.shape[0]) == 0: + return empty + start = index.group_offsets[positions] + counts = _csr_group_sizes(index, positions) total = int(counts.sum()) if total == 0: return empty return index.row_positions[_expand_ranges(start, counts, total, xp)] +def lookup_prop_rows(index: NodePropIndex, values: ArrayLike, xp: ArrayNamespace) -> ArrayLike: + """values -> node row positions of every row holding one of them. + + Order is unspecified here; callers that need frame order sort. + """ + return csr_gather_rows(index, values, xp) + + def prop_match_count(index: NodePropIndex, values: ArrayLike, xp: ArrayNamespace) -> int: - """How many rows ``lookup_prop_rows`` would return — the free selectivity - estimate the planner cost-gates on (CSR offsets only, no gather).""" - keys = index.keys_sorted - U = int(keys.shape[0]) - if U == 0 or int(values.shape[0]) == 0: - return 0 - v = values - if v.dtype != keys.dtype: - common = xp.promote_types(v.dtype, keys.dtype) - v = v.astype(common) - keys = keys.astype(common) - pos = xp.searchsorted(keys, v) - pos_clipped = xp.where(pos < U, pos, U - 1) - hit = keys[pos_clipped] == v - pos_hit = pos_clipped[hit] - if int(pos_hit.shape[0]) == 0: - return 0 - return int((index.group_offsets[pos_hit + 1] - index.group_offsets[pos_hit]).sum()) + """Rows ``lookup_prop_rows`` would return — the free selectivity estimate.""" + return csr_match_count(index, values, xp) + + +def lookup_degree(index: AdjacencyIndex, frontier: ArrayLike, xp: ArrayNamespace) -> int: + """Total incident-edge count for a frontier — the hop's fanout estimate, + computed from CSR offsets before any range expansion.""" + return csr_match_count(index, frontier, xp) diff --git a/graphistry/compute/gfql/index/wire.py b/graphistry/compute/gfql/index/wire.py index 12322e3edf..23ccf2e793 100644 --- a/graphistry/compute/gfql/index/wire.py +++ b/graphistry/compute/gfql/index/wire.py @@ -96,12 +96,23 @@ def apply_index_op(g: Any, op: IndexOp, *, engine: Any = "auto") -> Any: CreateIndex/DropIndex -> new Plottable; ShowIndexes -> pandas DataFrame. """ + from graphistry.Engine import resolve_engine from .api import create_index, drop_index, show_indexes, get_registry, _is_resident_index_valid + from .registry import NODE_PROP + if isinstance(op, CreateIndex): if not op.replace: reg = get_registry(g) - if reg.has(op.kind) and _is_resident_index_valid(g, op.kind, engine): + if op.kind == NODE_PROP: + # Property indexes are keyed by COLUMN, not kind: reuse only the + # index for THIS column, and only while it is still valid. + if op.column is not None and reg.get_node_prop_valid( + op.column, getattr(g, "_nodes", None), + resolve_engine(engine, g), + ) is not None: + return g + elif reg.has(op.kind) and _is_resident_index_valid(g, op.kind, engine): return g # valid resident index reuse return create_index(g, op.kind, column=op.column, name=op.name, engine=engine) if isinstance(op, DropIndex): @@ -113,16 +124,35 @@ def apply_index_op(g: Any, op: IndexOp, *, engine: Any = "auto") -> Any: reg = get_registry(g) kind = next((k for k, ix in reg.indexes.items() if getattr(ix, "name", None) == op.name), None) + column = op.column + if kind is None: + # Property indexes are column-keyed, so resolve names there too. + prop_col = next((c for c in reg.node_prop_cols() + if reg.node_props[c].name == op.name), None) + if prop_col is not None: + kind, column = NODE_PROP, prop_col if kind is None: if op.missing_ok: return g # IF EXISTS semantics: dropping a missing index is a no-op + resident = sorted( + [getattr(ix, 'name', k) for k, ix in reg.indexes.items()] + + [reg.node_props[c].name or c for c in reg.node_prop_cols()] + ) raise ValueError( f"DROP GFQL INDEX: no resident index named {op.name!r} " - f"(resident: {sorted(getattr(ix, 'name', k) for k, ix in reg.indexes.items())})" + f"(resident: {resident})" ) - if kind is not None and not op.missing_ok and not get_registry(g).has(kind): - raise ValueError(f"DROP GFQL INDEX: no resident index of kind {kind!r}") - return drop_index(g, kind) + return drop_index(g, kind, column=column) + if kind is not None and not op.missing_ok: + reg = get_registry(g) + is_resident = ( + (op.column in reg.node_prop_cols() if op.column is not None + else bool(reg.node_prop_cols())) + if kind == NODE_PROP else reg.has(kind) + ) + if not is_resident: + raise ValueError(f"DROP GFQL INDEX: no resident index of kind {kind!r}") + return drop_index(g, kind, column=op.column) if isinstance(op, ShowIndexes): return show_indexes(g) raise ValueError(f"Unknown index op: {op!r}") diff --git a/graphistry/compute/gfql/lazy/engine/polars/chain.py b/graphistry/compute/gfql/lazy/engine/polars/chain.py index 7917cb8a78..0dce446752 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/chain.py +++ b/graphistry/compute/gfql/lazy/engine/polars/chain.py @@ -582,26 +582,32 @@ def chain_polars(self: Plottable, ops, start_nodes: Optional[Any] = None) -> Plo "use engine='pandas' for this chain." ) + from graphistry.compute.chain import serialize_binding_ops + from graphistry.compute.gfql.index.handoff import ( + IndexedBindingsHandoff, attach_handoff, set_handoff, + ) + indexed_state, indexed_attempted = _try_indexed_middle_polars(self, middle, suffix, start_nodes) if indexed_state is not None: - from graphistry.compute.chain import serialize_binding_ops # Skip the canonical traversal: the compact indexed path bag already IS the # binding rows the suffix asks for. Hand it to the unchanged native rows # materializer through an internal graph copy. - g_cur = self.bind() - setattr(g_cur, "_gfql_indexed_bindings_state", (serialize_binding_ops(middle), indexed_state)) - setattr( - g_cur, - "_gfql_rows_edge_aliases", - tuple(op._name for op in middle if isinstance(op, ASTEdge) and isinstance(op._name, str)), - ) + g_cur = attach_handoff(self, IndexedBindingsHandoff( + binding_ops=serialize_binding_ops(middle), + state=indexed_state, + edge_aliases=tuple( + op._name for op in middle + if isinstance(op, ASTEdge) and isinstance(op._name, str) + ), + )) else: g_cur = _chain_traversal_polars(self, middle, start_nodes) if indexed_attempted: # Record the exact declined plan so the rows materializer does not # re-attempt (and re-record) the same decision. - from graphistry.compute.chain import serialize_binding_ops - setattr(g_cur, "_gfql_indexed_bindings_declined_ops", serialize_binding_ops(middle)) + set_handoff(g_cur, IndexedBindingsHandoff( + binding_ops=serialize_binding_ops(middle), + )) if suffix: g_cur = _run_calls_polars(g_cur, suffix, start_nodes, base_graph=self, middle=middle) return g_cur @@ -636,7 +642,7 @@ def _try_indexed_middle_polars(g, middle, suffix, start_nodes): binding_ops == serialize_binding_ops(middle) or ( binding_ops is None - and any(getattr(op, "_name", None) is not None for op in middle) + and any(op._name is not None for op in middle) ) ): return None, False diff --git a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py index fcd3953015..1db3b30fb5 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py +++ b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py @@ -1168,17 +1168,16 @@ def _names(lf: pl.LazyFrame) -> List[str]: # The chain boundary may already have decided this exact plan (served or # declined) before the canonical traversal; reuse that decision instead of # recomputing it — and re-recording a duplicate trace step. - precomputed = getattr(g, "_gfql_indexed_bindings_state", None) - declined_ops = getattr(g, "_gfql_indexed_bindings_declined_ops", None) - indexed_state = None - if precomputed is not None: - precomputed_ops, precomputed_state = precomputed - if ( - precomputed_ops == list(binding_ops) - and precomputed_state.engine == engine_concrete - ): - indexed_state = precomputed_state - if indexed_state is None and declined_ops != list(binding_ops): + from graphistry.compute.gfql.index.handoff import read_handoff + + handoff = read_handoff(g) + plan = list(binding_ops) + indexed_state = ( + handoff.state + if handoff is not None and handoff.serves(plan, engine_concrete) + else None + ) + if indexed_state is None and not (handoff is not None and handoff.declined(plan)): indexed_state = indexed_bindings.try_indexed_connected_bindings_state( base_graph, ops, diff --git a/graphistry/compute/gfql/row/frame_ops.py b/graphistry/compute/gfql/row/frame_ops.py index 61c1f44029..fd99b3c6ad 100644 --- a/graphistry/compute/gfql/row/frame_ops.py +++ b/graphistry/compute/gfql/row/frame_ops.py @@ -56,14 +56,11 @@ def _alias_true_mask(table_df: Any, source: str) -> Any: def row_table(ctx: RowPipelineCtx, table_df: Any) -> "Plottable": """Return a plottable that treats ``table_df`` as the active row table.""" - indexed_bindings_state = getattr(ctx, "_gfql_indexed_bindings_state", None) + from graphistry.compute.gfql.index.handoff import clear_handoff, read_handoff + + handoff = read_handoff(ctx) out = ctx.bind() - for attr in ( - "_gfql_indexed_bindings_state", - "_gfql_indexed_bindings_declined_ops", - ): - if hasattr(out, attr): - delattr(out, attr) + clear_handoff(out) # internal plumbing must never escape on a user-visible result # polars has no row index, so reset_index is both unnecessary and absent. if not _is_polars(table_df): table_df = table_df.reset_index(drop=True) @@ -72,10 +69,10 @@ def row_table(ctx: RowPipelineCtx, table_df: Any) -> "Plottable": out._edges = _empty_like(ctx._edges) else: out._edges = _empty_like(table_df) - if indexed_bindings_state is not None and out._edges is not None: - indexed_edge_aliases = tuple( - getattr(ctx, "_gfql_rows_edge_aliases", None) or () - ) + if handoff is not None and handoff.state is not None and out._edges is not None: + # The canonical traversal we skipped is what would have added these alias + # marker columns to the (zero-row) edge frame; synthesize them for parity. + indexed_edge_aliases = handoff.edge_aliases if indexed_edge_aliases: missing = [ alias @@ -97,9 +94,9 @@ def row_table(ctx: RowPipelineCtx, table_df: Any) -> "Plottable": for col in out._edges.columns if col not in indexed_edge_aliases ] - for alias in missing: - out._edges[alias] = True - out._edges = out._edges[list(indexed_edge_aliases) + original_cols] + out._edges = out._edges.assign( + **{alias: True for alias in missing} + )[list(indexed_edge_aliases) + original_cols] out._source = None out._destination = None out._edge = ctx._edge if ctx._edge is not None and ctx._edge in table_df.columns else None diff --git a/graphistry/compute/gfql/row/pipeline.py b/graphistry/compute/gfql/row/pipeline.py index 97d94c004c..46eaebd38b 100644 --- a/graphistry/compute/gfql/row/pipeline.py +++ b/graphistry/compute/gfql/row/pipeline.py @@ -3608,21 +3608,14 @@ def _gfql_connected_bindings_state( base_df = self._nodes if self._nodes is not None else self._edges engine = resolve_engine(EngineAbstract.AUTO, base_df) start_nodes = getattr(self, "_gfql_start_nodes", None) - precomputed = getattr(self, "_gfql_indexed_bindings_state", None) - if precomputed is not None: - precomputed_ops, precomputed_state = precomputed - if ( - precomputed_ops == serialize_binding_ops(ops) - and not alias_prefilters - and precomputed_state.engine == engine - ): - return precomputed_state.state, precomputed_state.alias_frames - declined_ops = getattr(self, "_gfql_indexed_bindings_declined_ops", None) - previously_declined = ( - declined_ops == serialize_binding_ops(ops) - if declined_ops is not None - else False - ) + from graphistry.compute.gfql.index.handoff import read_handoff + + handoff = read_handoff(self) + binding_ops = serialize_binding_ops(ops) + if handoff is not None and handoff.serves(binding_ops, engine) and not alias_prefilters: + assert handoff.state is not None # narrowed by serves() + return handoff.state.state, handoff.state.alias_frames + previously_declined = handoff is not None and handoff.declined(binding_ops) if ( not previously_declined and not RowPipelineMixin._gfql_is_shortest_path_scalar_binding_ops(ops) @@ -5191,12 +5184,9 @@ def __init__(self, g: "Plottable") -> None: self._gfql_start_nodes = getattr(g, "_gfql_start_nodes", None) self._gfql_rows_base_graph = getattr(g, "_gfql_rows_base_graph", None) self._gfql_rows_edge_aliases = getattr(g, "_gfql_rows_edge_aliases", None) - self._gfql_indexed_bindings_declined_ops = getattr( - g, "_gfql_indexed_bindings_declined_ops", None - ) - self._gfql_indexed_bindings_state = getattr( - g, "_gfql_indexed_bindings_state", None - ) + from graphistry.compute.gfql.index.handoff import HANDOFF_ATTR, read_handoff + + setattr(self, HANDOFF_ATTR, read_handoff(g)) self._nodes = g._nodes self._edges = g._edges self._node = g._node diff --git a/graphistry/tests/compute/gfql/index/test_index.py b/graphistry/tests/compute/gfql/index/test_index.py index 0303ce0495..baab166c2f 100644 --- a/graphistry/tests/compute/gfql/index/test_index.py +++ b/graphistry/tests/compute/gfql/index/test_index.py @@ -145,6 +145,46 @@ def test_wire_roundtrip(graph): show = g.gfql({"type": "ShowIndexes"}) assert show.shape[0] == 1 +def test_node_prop_ddl_and_wire_surfaces(graph): + """The property index must be reachable from all three surfaces (review of #1777): + Python, Cypher DDL, and the JSON wire protocol — including targeted drops.""" + from graphistry.compute.gfql.index import NODE_PROP + + # Cypher DDL + assert parse_index_ddl("CREATE GFQL INDEX FOR node_prop ON lab").column == "lab" + g = graph.gfql("CREATE GFQL INDEX FOR node_prop ON lab") + assert get_registry(g).node_prop_cols() == ("lab",) + assert g.gfql("SHOW GFQL INDEXES").shape[0] == 1 + + # JSON wire, incl. a second column, then a TARGETED drop of just one of them + g = g.gfql({"type": "CreateIndex", "kind": "node_prop", "column": "id"}) + assert get_registry(g).node_prop_cols() == ("id", "lab") + g2 = g.gfql({"type": "DropIndex", "kind": "node_prop", "column": "lab"}) + assert get_registry(g2).node_prop_cols() == ("id",) + + # kind-wide drop, and idempotent re-create of a still-valid resident index + assert get_registry(g.gfql({"type": "DropIndex", "kind": "node_prop"})).node_prop_cols() == () + again = g.gfql({"type": "CreateIndex", "kind": "node_prop", "column": "id"}) + assert get_registry(again).node_props["id"] is get_registry(g).node_props["id"] + + +def test_node_prop_drop_resident_does_not_raise(graph): + """`has()` only knows kind-keyed indexes, so a resident property index must not + report itself missing (review of #1777).""" + g = graph.gfql("CREATE GFQL INDEX FOR node_prop ON lab") + assert get_registry(g.gfql("DROP GFQL INDEX FOR node_prop")).node_prop_cols() == () + with pytest.raises(ValueError): + graph.gfql("DROP GFQL INDEX FOR node_prop") # none resident, no IF EXISTS + assert graph.gfql("DROP GFQL INDEX IF EXISTS FOR node_prop") is not None + + +def test_node_prop_drop_by_name(graph): + """A custom-named property index resolves by name, like the kind-keyed ones.""" + g = graph.create_index("node_prop", column="lab", name="by_lab") + assert get_registry(g).node_props["lab"].name == "by_lab" + assert get_registry(g.gfql("DROP GFQL INDEX by_lab")).node_prop_cols() == () + + def test_create_rebuilds_stale_resident_index(): g = graphistry.edges(pd.DataFrame({"src": [0, 1], "dst": [1, 2]}), "src", "dst").materialize_nodes() gi = g.gfql("CREATE GFQL INDEX FOR edge_out_adj") diff --git a/graphistry/tests/compute/gfql/index/test_indexed_bindings.py b/graphistry/tests/compute/gfql/index/test_indexed_bindings.py index b92e22492d..b1a642bba8 100644 --- a/graphistry/tests/compute/gfql/index/test_indexed_bindings.py +++ b/graphistry/tests/compute/gfql/index/test_indexed_bindings.py @@ -666,6 +666,62 @@ def test_node_property_index_prefers_the_most_selective_column( assert widths and widths[0] == 1 # 'public' (1 match) beats 'grp' (4 matches) +@pytest.mark.parametrize( + "seed,indexed_column,expect_gathered", + [ + pytest.param({"public": 100}, "public", True, id="selective-uses-index"), + pytest.param({"grp": 0}, "grp", False, id="unselective-keeps-scan"), + ], +) +def test_node_property_index_cost_gate_under_policy_use( + seed: Dict[str, Any], + indexed_column: str, + expect_gathered: bool, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Under the default `use` policy the gate must reject a non-selective predicate. + + `force` skips the gate, so the other property-index cases cannot exercise it. + The crossover is pinned here via the public knob rather than relying on the + engine default, so the test states the threshold it is testing: at 25%, + `public` (1 of 12 rows) gathers through the index and `grp` (4 of 12) does not. + Either way the answer is the canonical one. + """ + from graphistry.compute.ast import rows as rows_call + from graphistry.compute.gfql.index import reset_cost_gate_frac, set_cost_gate_frac + + set_cost_gate_frac(Engine.PANDAS, 0.25) + try: + g = _prop_graph("pandas", columns=(indexed_column,)) + query = [ + n(seed, name="a"), e_forward({"type": "A"}, name="r"), n(name="b"), rows_call(), + ] + with monkeypatch.context() as m: + expected = _run(g, query, "pandas", m, generic=True, index_policy="use") + + import graphistry.compute.gfql.index.bindings as indexed_bindings + + widths: List[int] = [] + original = indexed_bindings._filter_frame + + def record(frame: Any, *args: Any, **kwargs: Any) -> Any: + widths.append(int(frame.shape[0])) + return original(frame, *args, **kwargs) + + monkeypatch.setattr(indexed_bindings, "_filter_frame", record) + actual, _ = _trace_run(g, query, "pandas", index_policy="use") + finally: + reset_cost_gate_frac(Engine.PANDAS) + + _assert_result_exact(actual, expected, "pandas") + assert widths + n_nodes = int(g._nodes.shape[0]) + if expect_gathered: + assert widths[0] < n_nodes # indexed candidates only + else: + assert widths[0] == n_nodes # gate declined -> canonical scan + + def test_node_property_index_shows_and_drops() -> None: from graphistry.compute.gfql.index import NODE_PROP, get_registry @@ -680,6 +736,70 @@ def test_node_property_index_shows_and_drops() -> None: assert get_registry(g.drop_index()).is_empty() +@pytest.mark.parametrize( + "suffix_params,seeded,reason", + [ + pytest.param({"source": "a"}, False, "rows-with-source", id="rows-source"), + pytest.param({"alias_endpoints": {"a": "src"}}, False, "alias-endpoints", id="alias-endpoints"), + pytest.param({"alias_prefilters": {"b": {"rank": 1}}}, False, "prefiltered", id="alias-prefilters"), + pytest.param({}, True, "seeded re-entry", id="carried-seed"), + ], +) +def test_polars_early_gate_refuses_unsupported_boundaries( + suffix_params: Dict[str, Any], + seeded: bool, + reason: str, +) -> None: + """The polars bypass must not engage on shapes its rows call does not consume. + + Engaging on any of these would hand the materializer a compact state for a plan + it is not executing, so each refusal condition is asserted on the gate directly. + """ + pytest.importorskip("polars") + from graphistry.compute.ast import rows as rows_call + from graphistry.compute.gfql.lazy.engine.polars.chain import _try_indexed_middle_polars + + g = _graph("polars") + middle = [n({"public": 100}, name="a"), e_forward({"type": "A"}, name="r"), n(name="b")] + start_nodes = g._nodes.head(1) if seeded else None + + state, attempted = _try_indexed_middle_polars( + g, middle, [rows_call(**suffix_params)], start_nodes + ) + assert state is None, f"polars gate engaged on {reason}" + assert attempted is False, f"polars gate recorded an attempt on {reason}" + + +def test_polars_early_gate_requires_the_whole_middle() -> None: + """binding_ops that do not cover the middle must not take the bypass.""" + pytest.importorskip("polars") + from graphistry.compute.ast import rows as rows_call + from graphistry.compute.chain import serialize_binding_ops + from graphistry.compute.gfql.lazy.engine.polars.chain import _try_indexed_middle_polars + + g = _graph("polars") + middle = [n({"public": 100}, name="a"), e_forward({"type": "A"}, name="r"), n(name="b")] + + other = serialize_binding_ops([n({"public": 101}, name="a"), e_forward(), n(name="b")]) + state, attempted = _try_indexed_middle_polars( + g, middle, [rows_call(binding_ops=other)], None + ) + assert state is None and attempted is False # a different plan + + unnamed = [n({"public": 100}), e_forward({"type": "A"}), n()] + state, attempted = _try_indexed_middle_polars(g, unnamed, [rows_call()], None) + assert state is None and attempted is False # rewrite would not install them + + # the supported shape DOES engage (guards against a vacuous negative suite); + # `force` skips the cost gate, which a 12-row fixture would otherwise trip + from graphistry.compute.gfql.index import with_index_policy + + state, attempted = _try_indexed_middle_polars( + with_index_policy(g, "force"), middle, [rows_call()], None + ) + assert attempted is True and state is not None + + @pytest.mark.parametrize("engine", ENGINES) def test_indexed_execution_is_pure( engine: str, @@ -700,6 +820,7 @@ def test_indexed_execution_is_pure( pd.testing.assert_frame_equal(_to_pandas(g._edges), edges_before) +@pytest.mark.parametrize("engine", ENGINES) @pytest.mark.parametrize( "ops,kwargs", [ @@ -758,18 +879,25 @@ def test_indexed_execution_is_pure( ], ) def test_unsupported_shapes_decline_before_work( + engine: str, ops: Sequence[Any], kwargs: Dict[str, Any], ) -> None: import graphistry.compute.gfql.index.bindings as indexed_bindings from graphistry.compute.gfql.index import index_trace + call_kwargs = dict(kwargs) + if engine != "pandas" and "start_nodes" in call_kwargs: + native_seed, _ = _native_frames( + engine, call_kwargs["start_nodes"], _base_frames()[1] + ) + call_kwargs["start_nodes"] = native_seed with index_trace() as captured: out = indexed_bindings.try_indexed_connected_bindings_state( - _graph("pandas"), + _graph(engine), ops, - engine=Engine.PANDAS, - **kwargs, + engine=ENGINE_ENUM[engine], + **call_kwargs, ) assert out is None decisions = [ From d85b421f809aae1dfe6c04ae97089d24253619fb Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 25 Jul 2026 16:47:19 -0700 Subject: [PATCH 3/6] review: declare the GFQL execution-context fields; narrow index-build suppression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-up review nits. **Declared fields instead of smuggled attributes.** `with_index_policy` did `setattr(out, POLICY_ATTR, policy)`, and the whole `_gfql_*` execution context was attached to instances by name and read back with `getattr(..., default)` — untyped, and every reader had to remember the default. The family is now DECLARED on `Plottable` with its defaults on `PlotterBase` (`bind()` is `copy.copy`, so a per-graph value still travels with the instance): `_gfql_index_policy`, `_gfql_index_registry`, `_gfql_indexed_bindings_handoff`, `_gfql_rows_base_graph`, `_gfql_start_nodes`, `_gfql_rows_edge_aliases`, `_gfql_shortest_path_backend`. `RowPipelineMixin` mirrors them because `_RowPipelineAdapter` is not a `PlotterBase`, and the `RowPipelineCtx` protocol declares the three it reads. Call sites in this stack's files then become ordinary attribute access: `index/handoff.py` drops to ZERO dynamic attribute calls, and `api.py`'s policy and registry accessors, both chain boundaries, both row pipelines, and `frame_ops` stop using `getattr`/`setattr` for this family. That immediately surfaced a latent bug: `_SyntheticRowGraph`, a hand-rolled Plottable stand-in in the cypher lowering, set three of the fields and silently omitted `_gfql_rows_edge_aliases` (and the handoff). Under `getattr(..., None)` it read as "absent"; under typed access it raised, which is the point. It now constructs the full context. EXCLUDED deliberately: `_gfql_native_sp_cache` (a different subsystem's cache, no call site in this stack) and the repo-wide sweep of the same pattern outside these files — both are separate changes, not this PR's scope. **Narrowed suppression.** `gfql_index_node_props` wrapped `create_index` in `except ValueError`, which swallowed caller mistakes too — a typo'd column name silently left the query unindexed. `create_index` now raises the purpose-built `GfqlIndexUnsupportedError` (a `ValueError` subclass, so existing handlers keep working) for exactly the two conditions the DATA cannot support: duplicate node ids and an unindexable property dtype. The convenience builders catch only that; everything else propagates. `gfql_index_all` gets the same treatment for `node_id`. Tests pin both directions: unindexable dtypes are skipped, a missing column raises. Validated: full CPU `graphistry/tests/compute` 6260 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 70 passed; ruff clean; mypy unchanged vs master. An intermediate run caught 63 failures from the `_SyntheticRowGraph` gap and 3 mypy `no-redef`s from duplicated mixin annotations — both fixed here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 --- graphistry/Plottable.py | 14 +++++++ graphistry/PlotterBase.py | 16 ++++++++ graphistry/compute/chain.py | 5 +-- graphistry/compute/gfql/cypher/lowering.py | 5 +++ graphistry/compute/gfql/index/__init__.py | 4 +- graphistry/compute/gfql/index/api.py | 40 +++++++++++++------ graphistry/compute/gfql/index/handoff.py | 22 +++++----- .../compute/gfql/lazy/engine/polars/chain.py | 5 +-- .../gfql/lazy/engine/polars/row_pipeline.py | 8 ++-- graphistry/compute/gfql/row/frame_ops.py | 21 +++++----- graphistry/compute/gfql/row/pipeline.py | 38 ++++++++++-------- .../gfql/index/test_indexed_bindings.py | 27 +++++++++---- 12 files changed, 135 insertions(+), 70 deletions(-) diff --git a/graphistry/Plottable.py b/graphistry/Plottable.py index 970dd228ec..7f3388b962 100644 --- a/graphistry/Plottable.py +++ b/graphistry/Plottable.py @@ -22,6 +22,9 @@ from graphistry.models.surfaces.graphistry_frontend.url_params import URLParamsDict if TYPE_CHECKING: + from graphistry.compute.gfql.index.handoff import IndexedBindingsHandoff + from graphistry.compute.gfql.index.policy import IndexPolicy + from graphistry.compute.gfql.index.registry import GfqlIndexRegistry try: from umap import UMAP except: @@ -56,6 +59,17 @@ class Plottable(Protocol): session: ClientSession _pygraphistry: AuthManagerProtocol + # GFQL execution context. Declared here (rather than smuggled onto instances + # with setattr) so every access is typed and the defaults live in one place — + # see PlotterBase for the values. Internal: not part of the user surface. + _gfql_index_policy: "IndexPolicy" + _gfql_index_registry: Optional["GfqlIndexRegistry"] + _gfql_indexed_bindings_handoff: Optional["IndexedBindingsHandoff"] + _gfql_rows_base_graph: Optional["Plottable"] + _gfql_start_nodes: Optional[Any] + _gfql_rows_edge_aliases: Optional[Any] + _gfql_shortest_path_backend: str + _edges : Any _nodes : Any _source : Optional[str] diff --git a/graphistry/PlotterBase.py b/graphistry/PlotterBase.py index c512b734e0..5494adbc16 100644 --- a/graphistry/PlotterBase.py +++ b/graphistry/PlotterBase.py @@ -1,5 +1,10 @@ from graphistry.Plottable import Plottable, RenderModes, RenderModesConcrete from typing import Any, Callable, Dict, List, Optional, Union, Tuple, cast, overload, TYPE_CHECKING + +if TYPE_CHECKING: + from graphistry.compute.gfql.index.handoff import IndexedBindingsHandoff + from graphistry.compute.gfql.index.policy import IndexPolicy + from graphistry.compute.gfql.index.registry import GfqlIndexRegistry from typing_extensions import Literal from graphistry.io.types import ComplexEncodingsDict from graphistry.models.collections import CollectionsInput @@ -213,6 +218,17 @@ class PlotterBase(Plottable): The class supports convenience methods for mixing calls across Pandas, NetworkX, and IGraph. """ + # GFQL execution context defaults (fields declared on Plottable). Immutable + # scalars / None only, so sharing the class attribute is safe; `bind()` is a + # shallow copy, so a per-graph value set on an instance travels with it. + _gfql_index_policy: "IndexPolicy" = "use" + _gfql_index_registry: Optional["GfqlIndexRegistry"] = None + _gfql_indexed_bindings_handoff: Optional["IndexedBindingsHandoff"] = None + _gfql_rows_base_graph: Optional["Plottable"] = None + _gfql_start_nodes: Optional[Any] = None + _gfql_rows_edge_aliases: Optional[Any] = None + _gfql_shortest_path_backend: str = "auto" + _defaultNodeId = NODE _defaultEdgeSourceId = SRC _defaultEdgeDestinationId = DST diff --git a/graphistry/compute/chain.py b/graphistry/compute/chain.py index 1f3ccb63fc..b62bd23918 100644 --- a/graphistry/compute/chain.py +++ b/graphistry/compute/chain.py @@ -724,9 +724,8 @@ def _handle_boundary_calls( if suffix: logger.debug('Executing boundary suffix calls: %s', suffix) if start_nodes is not None: - setattr(g_temp, "_gfql_start_nodes", start_nodes) - setattr(g_temp, "_gfql_rows_base_graph", suffix_base_graph) - setattr(g_temp, "_gfql_shortest_path_backend", getattr(g_temp, "_gfql_shortest_path_backend", "auto")) + g_temp._gfql_start_nodes = start_nodes + g_temp._gfql_rows_base_graph = suffix_base_graph if ( middle and any(getattr(op, "_name", None) is not None for op in middle) diff --git a/graphistry/compute/gfql/cypher/lowering.py b/graphistry/compute/gfql/cypher/lowering.py index 65895bd6fa..833b2117db 100644 --- a/graphistry/compute/gfql/cypher/lowering.py +++ b/graphistry/compute/gfql/cypher/lowering.py @@ -2130,8 +2130,13 @@ def __init__(self, table_df: Any) -> None: self._destination = None self._edge = None self._g = self + # Full GFQL execution context (see Plottable): this stand-in flows through + # the same row pipeline, so every field must exist or typed access breaks. self._gfql_start_nodes = None self._gfql_rows_base_graph = None + self._gfql_rows_edge_aliases = None + self._gfql_indexed_bindings_handoff = None + self._gfql_index_policy = "use" self._gfql_shortest_path_backend = "auto" def bind(self) -> "_SyntheticRowGraph": diff --git a/graphistry/compute/gfql/index/__init__.py b/graphistry/compute/gfql/index/__init__.py index 4a44c58312..774dc5149a 100644 --- a/graphistry/compute/gfql/index/__init__.py +++ b/graphistry/compute/gfql/index/__init__.py @@ -18,6 +18,7 @@ from .api import ( create_index, drop_index, show_indexes, gfql_index_edges, gfql_index_all, gfql_index_node_props, + GfqlIndexUnsupportedError, get_registry, set_registry, get_index_policy, with_index_policy, maybe_index_hop, index_name, index_trace, ) @@ -36,7 +37,8 @@ "EDGE_OUT_ADJ", "EDGE_IN_ADJ", "NODE_ID", "NODE_PROP", "ADJ_KINDS", "ALL_KINDS", "AdjacencyIndex", "NodeIdIndex", "NodePropIndex", "create_index", "drop_index", "show_indexes", "gfql_index_edges", - "gfql_index_all", "gfql_index_node_props", "get_registry", "with_index_policy", "set_registry", "get_index_policy", "maybe_index_hop", "index_name", + "gfql_index_all", "gfql_index_node_props", "GfqlIndexUnsupportedError", + "get_registry", "with_index_policy", "set_registry", "get_index_policy", "maybe_index_hop", "index_name", "index_trace", "CreateIndex", "DropIndex", "ShowIndexes", "IndexOp", "apply_index_op", "index_op_from_json", "is_index_op", "is_index_op_json", diff --git a/graphistry/compute/gfql/index/api.py b/graphistry/compute/gfql/index/api.py index 6d35dd121e..97ac61a632 100644 --- a/graphistry/compute/gfql/index/api.py +++ b/graphistry/compute/gfql/index/api.py @@ -32,6 +32,17 @@ POLICY_ATTR = "_gfql_index_policy" REGISTRY_ATTR = "_gfql_index_registry" + +class GfqlIndexUnsupportedError(ValueError): + """The DATA cannot support this index (duplicate node ids, an unindexable + property dtype). Distinct from a caller mistake — a missing column, an unknown + kind, unbound edges — which stays a plain ``ValueError`` and must propagate. + + Subclasses ``ValueError`` so existing ``except ValueError`` callers keep + working; the convenience builders catch only THIS type, so a real failure is + never silently skipped. + """ + # --- lightweight, thread-local index decision trace (for gfql_explain) ------- import threading as _threading _TRACE = _threading.local() @@ -105,23 +116,24 @@ def _record_indexed_traversal( _seed_deg_sum = seed_deg_sum def get_registry(g: Plottable) -> GfqlIndexRegistry: - return cast(GfqlIndexRegistry, getattr(g, REGISTRY_ATTR, EMPTY_REGISTRY)) + registry = g._gfql_index_registry + return registry if registry is not None else EMPTY_REGISTRY def get_index_policy(g: Plottable) -> IndexPolicy: - return cast(IndexPolicy, getattr(g, POLICY_ATTR, "use")) + return g._gfql_index_policy def with_index_policy(g: Plottable, policy: IndexPolicy) -> Plottable: - """A copy of ``g`` carrying ``policy`` (the attribute lives only in this module).""" + """A copy of ``g`` carrying ``policy`` (never mutates ``g``).""" out = g.bind() - setattr(out, POLICY_ATTR, policy) + out._gfql_index_policy = policy return out def _attach(g: Plottable, registry: GfqlIndexRegistry) -> Plottable: res = copy.copy(g) - setattr(res, REGISTRY_ATTR, registry) + res._gfql_index_registry = registry return res @@ -214,7 +226,7 @@ def create_index( _check_column(column, node_col, kind) node_idx = build_node_id_index(g2._nodes, node_col, eng) if node_idx is None: - raise ValueError( + raise GfqlIndexUnsupportedError( f"Cannot build a {NODE_ID!r} index: node id column {node_col!r} has " f"duplicate values (a node-id index requires unique ids). Seeded " f"traversal still works via the un-indexed node materialization path." @@ -237,7 +249,7 @@ def create_index( ) prop_idx = build_node_prop_index(g2._nodes, column, eng) if prop_idx is None: - raise ValueError( + raise GfqlIndexUnsupportedError( f"Cannot build a {NODE_PROP!r} index on {column!r}: only integer " f"columns without nulls are indexable today. Seeded queries still " f"work via the un-indexed scan path." @@ -330,14 +342,16 @@ def gfql_index_node_props(g: Plottable, columns: Sequence[str], engine: EngineAbstractType = EngineAbstract.AUTO) -> Plottable: """Convenience: build node property indexes for ``columns`` (skips unindexable). - Skipping mirrors ``gfql_index_all``'s node_id behaviour — a column that cannot - be indexed keeps the correct scan path. ``create_index(NODE_PROP, column=...)`` - still raises, since the caller asked for that column specifically.""" + Skipping mirrors ``gfql_index_all``'s node_id behaviour — a column whose dtype + this index cannot serve keeps the correct scan path. ONLY that case is skipped: + a missing column, an unknown kind, or any unexpected failure propagates. + ``create_index(NODE_PROP, column=...)`` still raises for everything, since the + caller asked for that column specifically.""" for column in columns: try: g = create_index(g, NODE_PROP, column=column, engine=engine) - except ValueError: - pass + except GfqlIndexUnsupportedError: + continue # dtype this index cannot serve -> keep the correct scan path return g @@ -353,7 +367,7 @@ def gfql_index_all(g: Plottable, g = gfql_index_edges(g, "both", engine=engine) try: g = create_index(g, NODE_ID, engine=engine) - except ValueError: + except GfqlIndexUnsupportedError: pass # non-unique node ids -> skip the node_id accelerator (adjacency still built) return g diff --git a/graphistry/compute/gfql/index/handoff.py b/graphistry/compute/gfql/index/handoff.py index 35e8b45275..bce38a9a86 100644 --- a/graphistry/compute/gfql/index/handoff.py +++ b/graphistry/compute/gfql/index/handoff.py @@ -6,10 +6,10 @@ attribute it rides on, and the only attribute access — so callers stay typed and no other module hand-rolls ``getattr``/``setattr`` for it. -The attribute rides on a Plottable because that is how the row pipeline already -threads per-execution context (``_gfql_rows_base_graph``, ``_gfql_start_nodes``); -it is internal, always attached to an internal copy, and cleared before the -result is handed back to the caller. +The field is DECLARED on Plottable (with its default on PlotterBase) rather than +smuggled on with ``setattr``, so every access here is ordinary typed attribute +access. It is internal, always attached to an internal copy, and cleared before +the result is handed back to the caller. """ from __future__ import annotations @@ -24,9 +24,6 @@ from .bindings import IndexedBindingsState -HANDOFF_ATTR = "_gfql_indexed_bindings_handoff" - - @dataclass(frozen=True) class IndexedBindingsHandoff: """One boundary decision about one exact plan. @@ -56,21 +53,20 @@ def declined(self, binding_ops: List[Dict[str, JSONVal]]) -> bool: def attach_handoff(g: "Plottable", handoff: IndexedBindingsHandoff) -> "Plottable": """Return an internal copy of ``g`` carrying ``handoff`` (never mutates ``g``).""" out = g.bind() - setattr(out, HANDOFF_ATTR, handoff) + out._gfql_indexed_bindings_handoff = handoff return out def set_handoff(g: Any, handoff: IndexedBindingsHandoff) -> None: - """Attach ``handoff`` to a graph the caller already owns.""" - setattr(g, HANDOFF_ATTR, handoff) + """Attach ``handoff`` to an object the caller is itself constructing.""" + g._gfql_indexed_bindings_handoff = handoff def read_handoff(g: Any) -> Optional[IndexedBindingsHandoff]: """The boundary decision riding on ``g``, if any.""" - return getattr(g, HANDOFF_ATTR, None) + return g._gfql_indexed_bindings_handoff def clear_handoff(g: Any) -> None: """Drop the handoff so it never escapes on a user-visible result.""" - if hasattr(g, HANDOFF_ATTR): - delattr(g, HANDOFF_ATTR) + g._gfql_indexed_bindings_handoff = None diff --git a/graphistry/compute/gfql/lazy/engine/polars/chain.py b/graphistry/compute/gfql/lazy/engine/polars/chain.py index 0a0d037eea..3d2b70bdc9 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/chain.py +++ b/graphistry/compute/gfql/lazy/engine/polars/chain.py @@ -373,9 +373,8 @@ def _run_calls_polars(g_cur, calls, start_nodes, base_graph, middle): return g_cur if start_nodes is not None: - setattr(g_cur, "_gfql_start_nodes", start_nodes) - setattr(g_cur, "_gfql_rows_base_graph", base_graph) - setattr(g_cur, "_gfql_shortest_path_backend", getattr(g_cur, "_gfql_shortest_path_backend", "auto")) + g_cur._gfql_start_nodes = start_nodes + g_cur._gfql_rows_base_graph = base_graph if ( middle diff --git a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py index 1db3b30fb5..f99c9cf845 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py +++ b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py @@ -700,7 +700,7 @@ def names(frame: Any) -> List[str]: for alias in [op._name] if isinstance(alias, str) } - setattr(out, "_gfql_rows_edge_aliases", edge_aliases) + out._gfql_rows_edge_aliases = edge_aliases return out @@ -1119,7 +1119,7 @@ def _names(lf: pl.LazyFrame) -> List[str]: if nodes is None or edges is None or node_id is None or src is None or dst is None: return None seed_ids_lf: Optional[Any] = None # LazyFrame; Any avoids the union-typed seed_nodes.join mismatch - start_nodes = getattr(g, "_gfql_start_nodes", None) + start_nodes = g._gfql_start_nodes if start_nodes is not None: # Bounded WITH->MATCH re-entry (#1273): the carried WITH rows seed the first # alias. Constrain the first node alias to the carried ids via a semi-join — @@ -1157,7 +1157,7 @@ def _names(lf: pl.LazyFrame) -> List[str]: from graphistry.compute.gfql.index import bindings as indexed_bindings from graphistry.compute.gfql.lazy import active_target, ExecutionTarget from graphistry.Engine import Engine - base_graph = getattr(g, "_gfql_rows_base_graph", None) + base_graph = g._gfql_rows_base_graph if base_graph is None: base_graph = g engine_concrete = ( @@ -1435,7 +1435,7 @@ def _names(lf: pl.LazyFrame) -> List[str]: and edge_op.direction == "forward" and not RowPipelineMixin._gfql_node_filter_has_label(next_op.filter_dict) ): - base_graph = getattr(g, "_gfql_rows_base_graph", None) + base_graph = g._gfql_rows_base_graph base_nodes = getattr(base_graph, "_nodes", None) if base_nodes is None: return None diff --git a/graphistry/compute/gfql/row/frame_ops.py b/graphistry/compute/gfql/row/frame_ops.py index fd99b3c6ad..78277b104d 100644 --- a/graphistry/compute/gfql/row/frame_ops.py +++ b/graphistry/compute/gfql/row/frame_ops.py @@ -22,6 +22,9 @@ class RowPipelineCtx(Protocol): _nodes: Any _edges: Any _edge: Any + _gfql_rows_base_graph: Optional["Plottable"] + _gfql_start_nodes: Any + _gfql_rows_edge_aliases: Any def bind(self) -> "Plottable": ... def _gfql_binding_ops_row_table( self, @@ -102,17 +105,15 @@ def row_table(ctx: RowPipelineCtx, table_df: Any) -> "Plottable": out._edge = ctx._edge if ctx._edge is not None and ctx._edge in table_df.columns else None if out._node is not None and out._node not in table_df.columns: out._node = None - base_graph = getattr(ctx, "_gfql_rows_base_graph", None) + base_graph = ctx._gfql_rows_base_graph if base_graph is None: - base_graph = getattr(ctx, "_g", None) + base_graph = getattr(ctx, "_g", None) # adapter-only back-reference if base_graph is not None: - setattr(out, "_gfql_rows_base_graph", base_graph) - start_nodes = getattr(ctx, "_gfql_start_nodes", None) - if start_nodes is not None: - setattr(out, "_gfql_start_nodes", start_nodes) - edge_aliases = getattr(ctx, "_gfql_rows_edge_aliases", None) - if edge_aliases is not None: - setattr(out, "_gfql_rows_edge_aliases", edge_aliases) + out._gfql_rows_base_graph = base_graph + if ctx._gfql_start_nodes is not None: + out._gfql_start_nodes = ctx._gfql_start_nodes + if ctx._gfql_rows_edge_aliases is not None: + out._gfql_rows_edge_aliases = ctx._gfql_rows_edge_aliases return cast("Plottable", out) @@ -127,7 +128,7 @@ def empty_frame( elif ctx._edges is not None: template_df = ctx._edges else: - base_graph = getattr(ctx, "_gfql_rows_base_graph", None) + base_graph = ctx._gfql_rows_base_graph if base_graph is None: base_graph = getattr(ctx, "_g", None) if base_graph is not None: diff --git a/graphistry/compute/gfql/row/pipeline.py b/graphistry/compute/gfql/row/pipeline.py index 2aeb1f9487..1f39f1cf39 100644 --- a/graphistry/compute/gfql/row/pipeline.py +++ b/graphistry/compute/gfql/row/pipeline.py @@ -81,6 +81,7 @@ if TYPE_CHECKING: from graphistry.Plottable import Plottable + from graphistry.compute.gfql.index.handoff import IndexedBindingsHandoff from graphistry.compute.ast import ASTObject from graphistry.compute.gfql.expr_parser import ExprNode @@ -196,10 +197,17 @@ def is_row_pipeline_call(function: str) -> bool: class RowPipelineMixin: + # Mirrors the GFQL execution-context fields declared on Plottable: this mixin + # is also used by `_RowPipelineAdapter`, which is not a PlotterBase, so the + # defaults must exist here too for typed access to be total. + _gfql_index_policy: str = "use" + _gfql_indexed_bindings_handoff: Optional["IndexedBindingsHandoff"] = None + _gfql_shortest_path_backend: str = "auto" + _g: Any - _gfql_start_nodes: Any - _gfql_rows_base_graph: Any - _gfql_rows_edge_aliases: Any + _gfql_start_nodes: Any = None + _gfql_rows_base_graph: Any = None + _gfql_rows_edge_aliases: Any = None _nodes: Any _edges: Any _node: Any @@ -2759,7 +2767,7 @@ def _gfql_resolve_token(self, table_df: Any, token: str) -> Any: # identity column (alias.{node_id_col}). This lets expressions like # count(post) work when the table has post.id, post.name, etc. (#880) if "." not in txt and RowPipelineMixin._gfql_has_bindings_alias_prefix(table_df, txt): - edge_aliases = getattr(self, "_gfql_rows_edge_aliases", None) + edge_aliases = self._gfql_rows_edge_aliases if edge_aliases is not None and txt in edge_aliases: # Relationship aliases should render as entities (parity with # Cypher RETURN ) instead of collapsing to id-like @@ -3607,7 +3615,7 @@ def _gfql_connected_bindings_state( base_df = self._nodes if self._nodes is not None else self._edges engine = resolve_engine(EngineAbstract.AUTO, base_df) - start_nodes = getattr(self, "_gfql_start_nodes", None) + start_nodes = self._gfql_start_nodes from graphistry.compute.gfql.index.handoff import read_handoff handoff = read_handoff(self) @@ -3861,7 +3869,7 @@ def _gfql_connected_bindings_row_table_from_ops( for alias in [getattr(op, "_name", None)] if isinstance(op, ASTEdge) and isinstance(alias, str) } - setattr(out, "_gfql_rows_edge_aliases", edge_aliases) + out._gfql_rows_edge_aliases = edge_aliases return out def _gfql_add_missing_binding_columns( @@ -4119,7 +4127,7 @@ def _gfql_shortest_path_scalar_bindings_row_table( return self._gfql_row_table(self._gfql_empty_frame()) # Try native igraph/cugraph backend first; fall back to BFS - sp_backend = getattr(self, "_gfql_shortest_path_backend", "auto") + sp_backend = self._gfql_shortest_path_backend reachable_hops = self._gfql_shortest_path_scalar_native( seed_table, ops, start_alias, end_alias, hop_column, backend=sp_backend ) @@ -4190,7 +4198,7 @@ def _gfql_cartesian_node_bindings_row_table( base_df = self._nodes if self._nodes is not None else self._edges engine = resolve_engine(EngineAbstract.AUTO, base_df) - start_nodes = getattr(self, "_gfql_start_nodes", None) + start_nodes = self._gfql_start_nodes join_col = RowPipelineMixin._gfql_fresh_col_name(base_nodes.columns, "__gfql_bindings_join__") bindings: Optional[Any] = None anonymous_cols: List[str] = [] @@ -5180,15 +5188,13 @@ class _RowPipelineAdapter(RowPipelineMixin): """Adapter for row-pipeline calls without requiring global ComputeMixin inheritance.""" def __init__(self, g: "Plottable") -> None: + from graphistry.compute.gfql.index.handoff import read_handoff + self._g = g - self._gfql_start_nodes = getattr(g, "_gfql_start_nodes", None) - self._gfql_rows_base_graph = getattr(g, "_gfql_rows_base_graph", None) - self._gfql_rows_edge_aliases = getattr(g, "_gfql_rows_edge_aliases", None) - from graphistry.compute.gfql.index.handoff import read_handoff, set_handoff - - handoff = read_handoff(g) - if handoff is not None: - set_handoff(self, handoff) + self._gfql_start_nodes = g._gfql_start_nodes + self._gfql_rows_base_graph = g._gfql_rows_base_graph + self._gfql_rows_edge_aliases = g._gfql_rows_edge_aliases + self._gfql_indexed_bindings_handoff = read_handoff(g) self._nodes = g._nodes self._edges = g._edges self._node = g._node diff --git a/graphistry/tests/compute/gfql/index/test_indexed_bindings.py b/graphistry/tests/compute/gfql/index/test_indexed_bindings.py index b1a642bba8..55e9a6d367 100644 --- a/graphistry/tests/compute/gfql/index/test_indexed_bindings.py +++ b/graphistry/tests/compute/gfql/index/test_indexed_bindings.py @@ -632,19 +632,32 @@ def test_node_property_index_lifecycle_falls_back( def test_node_property_index_declines_unindexable_columns() -> None: - from graphistry.compute.gfql.index import NODE_PROP, create_index, get_registry + """Unindexable DTYPE is skippable; a caller mistake is not. + + The convenience builder suppresses exactly one condition — a column whose dtype + this index cannot serve. A missing column, a missing argument, or any other + failure must propagate, or a typo would silently leave the query unindexed. + """ + from graphistry.compute.gfql.index import ( + NODE_PROP, GfqlIndexUnsupportedError, create_index, get_registry, + ) g = _prop_graph("pandas", columns=()) for column in ("kind", "maybe"): # object dtype, float-with-null - with pytest.raises(ValueError): + with pytest.raises(GfqlIndexUnsupportedError): create_index(g, NODE_PROP, column=column) - with pytest.raises(ValueError): - create_index(g, NODE_PROP, column="nosuch") - with pytest.raises(ValueError): - create_index(g, NODE_PROP) # column is required for a property index - # the convenience wrapper skips instead of raising, and indexes what it can + # caller mistakes are NOT the skippable kind + for bad in ({"column": "nosuch"}, {}): + with pytest.raises(ValueError) as excinfo: + create_index(g, NODE_PROP, **bad) + assert not isinstance(excinfo.value, GfqlIndexUnsupportedError) + + # the convenience wrapper skips the unindexable dtypes and indexes what it can g2 = g.gfql_index_node_props(["kind", "maybe", "public"]) assert get_registry(g2).node_prop_cols() == ("public",) + # ...but does NOT swallow a real failure + with pytest.raises(ValueError): + g.gfql_index_node_props(["nosuch"]) def test_node_property_index_prefers_the_most_selective_column( From 8f6460d8589dd734b0abb167e8b251c8cfd2675a Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 25 Jul 2026 17:21:37 -0700 Subject: [PATCH 4/6] review: finish the field conversion and annotate the added defs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review asked whether every field-access site was handled. It was not — an audit of all seven declared fields found four non-test sites still using `getattr`/`setattr` (`gfql_unified.py` x2, `polars/pattern_apply.py` x2), now converted. Repo-wide, non-test dynamic access to this family is zero. More important, the same audit found the `_SyntheticRowGraph` bug class was NOT fully closed: both hand-rolled Plottable stand-ins — that class and `_RowPipelineAdapter` (via `RowPipelineMixin`) — were missing `_gfql_index_registry`, which `get_registry` now reads directly. Reachable: `bindings.py` calls `get_registry(base_graph)` and the row pipeline's `base_graph` can be the adapter. Both now carry it, so typed access is total for every object that flows through these paths. Also annotated the defs this stack added that were bare: the polars `_try_indexed_middle_polars` gate (now `-> Tuple[Optional[IndexedBindingsState], bool]`) and the two `ComputeMixin` index wrappers. ComputeMixin is only ~30% annotated, so matching the local style would have been defensible, but new public surface should not add to that debt. Validated: full CPU `graphistry/tests/compute` 6260 passed with only master's pre-existing `test_chain_dask_edges`; cuDF lane 70 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 --- graphistry/compute/ComputeMixin.py | 7 ++++--- graphistry/compute/gfql/cypher/lowering.py | 1 + graphistry/compute/gfql/lazy/engine/polars/chain.py | 12 ++++++++++-- .../compute/gfql/lazy/engine/polars/pattern_apply.py | 4 ++-- graphistry/compute/gfql/row/pipeline.py | 2 ++ graphistry/compute/gfql_unified.py | 4 ++-- 6 files changed, 21 insertions(+), 9 deletions(-) diff --git a/graphistry/compute/ComputeMixin.py b/graphistry/compute/ComputeMixin.py index 966cc969d1..736d5853f8 100644 --- a/graphistry/compute/ComputeMixin.py +++ b/graphistry/compute/ComputeMixin.py @@ -1,6 +1,6 @@ import numpy as np import pandas as pd -from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Sequence, Union from typing_extensions import Literal from graphistry.Engine import Engine, EngineAbstract, EngineAbstractType, POLARS_ENGINES, resolve_engine, df_to_engine, df_concat, safe_merge from graphistry.Plottable import Plottable @@ -29,6 +29,7 @@ ) if TYPE_CHECKING: + from graphistry.compute.gfql.index.types import IndexKind from graphistry.compute.gfql.index.explain import GfqlExplainReport from graphistry.compute.gfql.index.policy import IndexPolicy from graphistry.compute.gfql.query_types import GFQLQuery @@ -638,7 +639,7 @@ def create_index(self, kind, *, column=None, name=None, engine='auto'): from graphistry.compute.gfql.index import create_index as _ci return _ci(self, kind, column=column, name=name, engine=engine) - def drop_index(self, kind=None, *, column=None): + def drop_index(self, kind: Optional['IndexKind'] = None, *, column: Optional[str] = None) -> 'Plottable': """Drop one resident GFQL index (by kind, or one property index by column) or all (kind=None). Idempotent; returns a new Plottable.""" from graphistry.compute.gfql.index import drop_index as _di return _di(self, kind, column=column) @@ -658,7 +659,7 @@ def gfql_index_all(self, engine='auto'): from graphistry.compute.gfql.index import gfql_index_all as _gia return _gia(self, engine=engine) - def gfql_index_node_props(self, columns, engine='auto'): + def gfql_index_node_props(self, columns: Sequence[str], engine: EngineAbstractType = 'auto') -> 'Plottable': """Convenience: build node PROPERTY indexes for ``columns`` (secondary indexes). A seed predicate on a non-key column (``{id: 42}`` when the graph's node id diff --git a/graphistry/compute/gfql/cypher/lowering.py b/graphistry/compute/gfql/cypher/lowering.py index 833b2117db..91b0e0c09a 100644 --- a/graphistry/compute/gfql/cypher/lowering.py +++ b/graphistry/compute/gfql/cypher/lowering.py @@ -2137,6 +2137,7 @@ def __init__(self, table_df: Any) -> None: self._gfql_rows_edge_aliases = None self._gfql_indexed_bindings_handoff = None self._gfql_index_policy = "use" + self._gfql_index_registry = None self._gfql_shortest_path_backend = "auto" def bind(self) -> "_SyntheticRowGraph": diff --git a/graphistry/compute/gfql/lazy/engine/polars/chain.py b/graphistry/compute/gfql/lazy/engine/polars/chain.py index 3d2b70bdc9..dd5dbf42bd 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/chain.py +++ b/graphistry/compute/gfql/lazy/engine/polars/chain.py @@ -7,7 +7,7 @@ (no silent pandas fallback). Deferred: variable-length/multi-hop edge sub-cases, some undirected multi-edge combos, node query=. """ -from typing import Any, List, Optional, Tuple, cast +from typing import TYPE_CHECKING, Any, List, Optional, Tuple, cast from typing_extensions import TypedDict @@ -17,6 +17,9 @@ from graphistry.Plottable import Plottable from graphistry.compute.ast import ASTObject, ASTNode, ASTEdge + +if TYPE_CHECKING: + from graphistry.compute.gfql.index.bindings import IndexedBindingsState from .hop_eager import ensure_nodes_polars from .dtypes import is_lazy, colnames, endpoint_ids from .degrees import get_degrees_polars, get_indegrees_polars, get_outdegrees_polars @@ -614,7 +617,12 @@ def chain_polars(self: Plottable, ops, start_nodes: Optional[Any] = None) -> Plo return g_cur -def _try_indexed_middle_polars(g, middle, suffix, start_nodes): +def _try_indexed_middle_polars( + g: Plottable, + middle: List[ASTObject], + suffix: List[ASTObject], + start_nodes: Optional[Any], +) -> Tuple[Optional["IndexedBindingsState"], bool]: """Attempt the indexed fixed-hop path BEFORE the canonical polars traversal. Returns ``(state_or_None, attempted)``. The shape gate mirrors the pandas diff --git a/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py b/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py index 2b2b7e36e4..b1cf40eb1d 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py +++ b/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py @@ -26,7 +26,7 @@ def _rows_base_graph(g: Plottable) -> Plottable: """The ORIGINAL graph threaded by the boundary lane (`_gfql_rows_base_graph` setattr convention, mirrored from the pandas lane at compute/chain.py) — the one sanctioned dynamic read, contained here; falls back to the current graph.""" - base = getattr(g, "_gfql_rows_base_graph", None) + base = g._gfql_rows_base_graph return base if base is not None else g @@ -60,7 +60,7 @@ def rows_binding_ops_polars(g: Plottable, binding_ops: Sequence[Dict[str, JSONVa nodes = base_graph._nodes if nodes is None or node_id not in nodes.columns: return None - start_nodes = getattr(g, "_gfql_start_nodes", None) + start_nodes = g._gfql_start_nodes if start_nodes is not None: # start-nodes constrain the scan like the pandas prev_node_wavefront does. # Normalize to EAGER: an eager.join(LazyFrame) raises and would silently diff --git a/graphistry/compute/gfql/row/pipeline.py b/graphistry/compute/gfql/row/pipeline.py index 1f39f1cf39..6c485acc1d 100644 --- a/graphistry/compute/gfql/row/pipeline.py +++ b/graphistry/compute/gfql/row/pipeline.py @@ -82,6 +82,7 @@ if TYPE_CHECKING: from graphistry.Plottable import Plottable from graphistry.compute.gfql.index.handoff import IndexedBindingsHandoff + from graphistry.compute.gfql.index.registry import GfqlIndexRegistry from graphistry.compute.ast import ASTObject from graphistry.compute.gfql.expr_parser import ExprNode @@ -201,6 +202,7 @@ class RowPipelineMixin: # is also used by `_RowPipelineAdapter`, which is not a PlotterBase, so the # defaults must exist here too for typed access to be total. _gfql_index_policy: str = "use" + _gfql_index_registry: Optional["GfqlIndexRegistry"] = None _gfql_indexed_bindings_handoff: Optional["IndexedBindingsHandoff"] = None _gfql_shortest_path_backend: str = "auto" diff --git a/graphistry/compute/gfql_unified.py b/graphistry/compute/gfql_unified.py index 5ece7cb218..1932c8b0ab 100644 --- a/graphistry/compute/gfql_unified.py +++ b/graphistry/compute/gfql_unified.py @@ -1120,7 +1120,7 @@ def _execute_compiled_query_chain_non_union( and getattr(_first_op, "function", None) == "rows" and getattr(_first_op, "params", {}).get("binding_ops") is not None ): - setattr(dispatch_graph, "_gfql_start_nodes", start_nodes) + dispatch_graph._gfql_start_nodes = start_nodes result = _chain_dispatch(dispatch_graph, compiled_query.chain, engine, policy, context, start_nodes=start_nodes) if compiled_query.empty_result_row is not None: @@ -1848,7 +1848,7 @@ def gfql(self: Plottable, raise dispatch_self = self - setattr(dispatch_self, "_gfql_shortest_path_backend", shortest_path_backend) + dispatch_self._gfql_shortest_path_backend = shortest_path_backend compiled_query = None if where_param and isinstance(query, (dict, ASTLet)): From 75877e9d3ceadd72b59d79d205a19ae78bd45faa Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 25 Jul 2026 17:39:46 -0700 Subject: [PATCH 5/6] review: precise types for the row-pipeline surfaces; scope the polars decline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three more review points on the files this stack touches. **`row/frame_ops.py` declared its context protocol as `Any`.** The `RowPipelineCtx` fields are now typed — `Optional[DataFrameT]` for the frames, `Optional[str]` for the id bindings, `Optional[Plottable]` for the base graph and the adapter's `_g` back-reference, `Optional[Iterable[str]]` for the edge aliases — via a `TYPE_CHECKING` import of `DataFrameT`. Three polars-on-`DataFrameT` calls that the sharper types expose take localized `# type: ignore`s, which is the documented convention for engine-polymorphic frames here. **`row/frame_ops.py` still used `getattr`.** All of them are gone: `_g` is now a declared field (defaulted on `PlotterBase`, set only by adapters) and the two `getattr(base_graph, "_nodes"/"_edges", None)` — pre-existing, but pointless once the fields are declared — are direct access. That file now has ZERO `getattr`. **`polars/row_pipeline.py` swallowed unexpected exceptions.** Extracting `_finish_binding_rows_polars` made the INDEXED path share the generic path's `except pl.exceptions.SchemaError: return None`. That rationale is specific to the generic builder — polars will not unify int/float join keys the way pandas implicitly does, so declining is honest — whereas the indexed state comes from a helper that already verified those dtypes, so a `SchemaError` there is a BUG being converted into a silent slow-path fallback. The finisher now takes `decline_on_schema_error`; the indexed caller passes `False` and lets it raise. On the `Any`s in that same file: `ops` is now `Sequence[ASTObject]` and the alias narrowing is real `isinstance` rather than `getattr`, but `state`/`alias_frames` stay `Any` ON PURPOSE — the same parameter receives a polars eager frame, a polars LazyFrame, and the engine-polymorphic `DataFrameT` the indexed helper returns, so a precise union only buys `# type: ignore`s. The comment says so. Validated on this branch: CPU 6232 passed with only master's pre-existing `test_chain_dask_edges`; cuDF 60 passed; ruff clean; mypy unchanged vs master, and `--warn-unused-ignores` finds none of the new suppressions unnecessary. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 --- graphistry/PlotterBase.py | 1 + .../gfql/lazy/engine/polars/row_pipeline.py | 25 +++++++++++++--- graphistry/compute/gfql/row/frame_ops.py | 30 +++++++++++-------- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/graphistry/PlotterBase.py b/graphistry/PlotterBase.py index 5494adbc16..709d9f37cc 100644 --- a/graphistry/PlotterBase.py +++ b/graphistry/PlotterBase.py @@ -228,6 +228,7 @@ class PlotterBase(Plottable): _gfql_start_nodes: Optional[Any] = None _gfql_rows_edge_aliases: Optional[Any] = None _gfql_shortest_path_backend: str = "auto" + _g: Optional["Plottable"] = None # set only on row-pipeline adapters _defaultNodeId = NODE _defaultEdgeSourceId = SRC diff --git a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py index f99c9cf845..33cf2d98e7 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py +++ b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py @@ -638,14 +638,27 @@ def _rewrap(g: Plottable, table_df: Any) -> Plottable: def _finish_binding_rows_polars( g: Plottable, - ops: Sequence[Any], + ops: "Sequence[ASTObject]", + # Deliberately Any, not a polars union: the SAME parameter receives a polars + # eager frame, a polars LazyFrame, and the engine-polymorphic `DataFrameT` the + # indexed helper returns. A precise union here only buys `# type: ignore`s. state: Any, alias_frames: Dict[str, Any], node_id: str, attach_prop_aliases: Optional[Sequence[str]], + *, + decline_on_schema_error: bool, ) -> Optional[Plottable]: - """Canonical property attachment/materialization for generic or indexed state.""" + """Canonical property attachment/materialization for generic or indexed state. + + ``decline_on_schema_error`` reflects WHOSE state this is. The generic builder + joins frames polars will not unify the way pandas implicitly does (int vs float + join keys), so a ``SchemaError`` there is an honest decline. The indexed state + comes from a helper that already checked those dtypes, so a ``SchemaError`` + there is a BUG and must surface rather than become a silent slow-path fallback. + """ import polars as pl + from graphistry.compute.ast import ASTEdge, ASTNode from graphistry.compute.gfql.lazy import collect as _lazy_collect def names(frame: Any) -> List[str]: @@ -659,10 +672,10 @@ def names(frame: Any) -> List[str]: attach_set = ( None if attach_prop_aliases is None else set(attach_prop_aliases) ) - node_aliases = [ + node_aliases: List[str] = [ op._name for op in ops[::2] - if isinstance(getattr(op, "_name", None), str) + if isinstance(op, (ASTNode, ASTEdge)) and isinstance(op._name, str) ] for alias in node_aliases: if attach_set is not None and alias not in attach_set: @@ -691,6 +704,8 @@ def names(frame: Any) -> List[str]: else state ) except pl.exceptions.SchemaError: + if not decline_on_schema_error: + raise return None out = _rewrap(g, out_df) @@ -1192,6 +1207,7 @@ def _names(lf: pl.LazyFrame) -> List[str]: indexed_state.alias_frames, str(node_id), attach_prop_aliases, + decline_on_schema_error=False, # our own state: a schema clash is a bug ) for idx, op in enumerate(ops): @@ -1457,6 +1473,7 @@ def _names(lf: pl.LazyFrame) -> List[str]: return _finish_binding_rows_polars( g, ops, state, alias_frames, node_id, attach_prop_aliases, + decline_on_schema_error=True, # pandas-vs-polars join-key dtype divergence ) except pl.exceptions.SchemaError: return None diff --git a/graphistry/compute/gfql/row/frame_ops.py b/graphistry/compute/gfql/row/frame_ops.py index 78277b104d..000433334d 100644 --- a/graphistry/compute/gfql/row/frame_ops.py +++ b/graphistry/compute/gfql/row/frame_ops.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any, Dict, List, Optional, Sequence, cast +from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Optional, Sequence, cast import pandas as pd @@ -11,6 +11,7 @@ if TYPE_CHECKING: from typing import Protocol from graphistry.Plottable import Plottable + from graphistry.compute.typing import DataFrameT class RowPipelineCtx(Protocol): """Structural contract the row-pipeline frame ops need from their host graph. @@ -19,12 +20,15 @@ class RowPipelineCtx(Protocol): former ``ctx: Any`` so the attribute/method access is type-checked instead of duck-typed. Type-check-only (annotations are strings under ``from __future__ import annotations``) — zero runtime effect, no runtime Protocol import.""" - _nodes: Any - _edges: Any - _edge: Any + _nodes: Optional["DataFrameT"] + _edges: Optional["DataFrameT"] + _edge: Optional[str] + _node: Optional[str] + # Back-reference to the graph an adapter wraps; None on a graph that IS one. + _g: Optional["Plottable"] _gfql_rows_base_graph: Optional["Plottable"] - _gfql_start_nodes: Any - _gfql_rows_edge_aliases: Any + _gfql_start_nodes: Optional["DataFrameT"] + _gfql_rows_edge_aliases: Optional[Iterable[str]] def bind(self) -> "Plottable": ... def _gfql_binding_ops_row_table( self, @@ -107,7 +111,7 @@ def row_table(ctx: RowPipelineCtx, table_df: Any) -> "Plottable": out._node = None base_graph = ctx._gfql_rows_base_graph if base_graph is None: - base_graph = getattr(ctx, "_g", None) # adapter-only back-reference + base_graph = ctx._g # adapter-only back-reference if base_graph is not None: out._gfql_rows_base_graph = base_graph if ctx._gfql_start_nodes is not None: @@ -130,11 +134,11 @@ def empty_frame( else: base_graph = ctx._gfql_rows_base_graph if base_graph is None: - base_graph = getattr(ctx, "_g", None) + base_graph = ctx._g if base_graph is not None: - template_df = getattr(base_graph, "_nodes", None) + template_df = base_graph._nodes if template_df is None: - template_df = getattr(base_graph, "_edges", None) + template_df = base_graph._edges if template_df is not None: if columns is None: @@ -220,7 +224,7 @@ def rows( raise ValueError(f"rows(source=...) alias column not found: {source!r}") if _is_polars(table_df): import polars as pl - table_df = table_df.filter(pl.col(source).fill_null(False).cast(pl.Boolean)) + table_df = table_df.filter(pl.col(source).fill_null(False).cast(pl.Boolean)) # type: ignore[arg-type] else: table_df = table_df.loc[_alias_true_mask(table_df, source)] @@ -264,7 +268,7 @@ def count_table( import polars as pl if source is not None: # LazyFrame lacks .columns without a resolve; collect_schema is lazy-safe. - cols = table_df.collect_schema().names() + cols = table_df.collect_schema().names() # type: ignore[operator] if source not in cols: raise ValueError( f"count_table(source=...) alias column not found: {source!r}" @@ -272,7 +276,7 @@ def count_table( count_expr = pl.col(source).fill_null(False).cast(pl.Boolean).sum() else: count_expr = pl.len() - res = table_df.select(count_expr.alias(alias)) + res = table_df.select(count_expr.alias(alias)) # type: ignore[operator] # eager DataFrame.select -> DataFrame (no collect); LazyFrame.select -> LazyFrame. if hasattr(res, "collect"): res = res.collect() From ab911249d549df6acfef15bc5458726164ee76c4 Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 25 Jul 2026 18:15:07 -0700 Subject: [PATCH 6/6] review: type the execution-context fields and the polars path bag precisely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review: the declared fields and the polars finisher were still `Any`. `_gfql_start_nodes` is now `Optional[DataFrameT]` and `_gfql_rows_edge_aliases` `Optional[Iterable[str]]` on `Plottable`, `PlotterBase`, and `RowPipelineMixin` (`_g` / `_gfql_rows_base_graph` are `Optional[Plottable]`). Note `Plottable` types its own `_nodes`/`_edges` as `Any`; matching that would have been consistent, but new fields should not add to that debt. `_finish_binding_rows_polars`'s `state`/`alias_frames` are now a CONSTRAINED TypeVar over polars eager/lazy frames rather than `Any`. A plain union does not work here — `state.join(lookup)` needs both sides to be the same flavour, which only a TypeVar expresses. The engine-polymorphic `DataFrameT` the indexed helper returns is narrowed once, at that call, with a comment; the generic caller carries a single `[misc]` because its own (pre-existing) branches mix eager and lazy frames, so inference cannot pick one there. Two `[operator]` ignores cover polars calls on `DataFrameT`-typed seeds — the documented convention for engine-polymorphic frames. The precision paid for itself immediately: mypy flagged that `_RowPipelineAdapter.bind()` dereferences `_g`, which the declaration made `Optional`. Narrowing the adapter's own attribute would have broken its structural match against `RowPipelineCtx` (a protocol's mutable attributes are invariant), so `bind()` asserts the constructor's invariant instead. Validated on this branch: CPU 6232 passed with only master's pre-existing `test_chain_dask_edges`; cuDF 60 passed; ruff clean; mypy unchanged vs master. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NxxTFpKiqA4FQa9Pjy8yR1 --- graphistry/Plottable.py | 7 +++-- graphistry/PlotterBase.py | 7 +++-- .../gfql/lazy/engine/polars/pattern_apply.py | 2 +- .../gfql/lazy/engine/polars/row_pipeline.py | 31 +++++++++++++------ graphistry/compute/gfql/row/pipeline.py | 12 ++++--- 5 files changed, 37 insertions(+), 22 deletions(-) diff --git a/graphistry/Plottable.py b/graphistry/Plottable.py index 7f3388b962..52fdb5a03a 100644 --- a/graphistry/Plottable.py +++ b/graphistry/Plottable.py @@ -1,4 +1,4 @@ -from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Set, Tuple, Union, Protocol, overload +from typing import TYPE_CHECKING, Any, Callable, Dict, Iterable, List, Optional, Set, Tuple, Union, Protocol, overload from typing_extensions import Literal, runtime_checkable import pandas as pd @@ -22,6 +22,7 @@ from graphistry.models.surfaces.graphistry_frontend.url_params import URLParamsDict if TYPE_CHECKING: + from graphistry.compute.typing import DataFrameT from graphistry.compute.gfql.index.handoff import IndexedBindingsHandoff from graphistry.compute.gfql.index.policy import IndexPolicy from graphistry.compute.gfql.index.registry import GfqlIndexRegistry @@ -66,8 +67,8 @@ class Plottable(Protocol): _gfql_index_registry: Optional["GfqlIndexRegistry"] _gfql_indexed_bindings_handoff: Optional["IndexedBindingsHandoff"] _gfql_rows_base_graph: Optional["Plottable"] - _gfql_start_nodes: Optional[Any] - _gfql_rows_edge_aliases: Optional[Any] + _gfql_start_nodes: Optional["DataFrameT"] + _gfql_rows_edge_aliases: Optional[Iterable[str]] _gfql_shortest_path_backend: str _edges : Any diff --git a/graphistry/PlotterBase.py b/graphistry/PlotterBase.py index 709d9f37cc..ba0920e8ae 100644 --- a/graphistry/PlotterBase.py +++ b/graphistry/PlotterBase.py @@ -1,7 +1,8 @@ from graphistry.Plottable import Plottable, RenderModes, RenderModesConcrete -from typing import Any, Callable, Dict, List, Optional, Union, Tuple, cast, overload, TYPE_CHECKING +from typing import Any, Callable, Dict, Iterable, List, Optional, Union, Tuple, cast, overload, TYPE_CHECKING if TYPE_CHECKING: + from graphistry.compute.typing import DataFrameT from graphistry.compute.gfql.index.handoff import IndexedBindingsHandoff from graphistry.compute.gfql.index.policy import IndexPolicy from graphistry.compute.gfql.index.registry import GfqlIndexRegistry @@ -225,8 +226,8 @@ class PlotterBase(Plottable): _gfql_index_registry: Optional["GfqlIndexRegistry"] = None _gfql_indexed_bindings_handoff: Optional["IndexedBindingsHandoff"] = None _gfql_rows_base_graph: Optional["Plottable"] = None - _gfql_start_nodes: Optional[Any] = None - _gfql_rows_edge_aliases: Optional[Any] = None + _gfql_start_nodes: Optional["DataFrameT"] = None + _gfql_rows_edge_aliases: Optional[Iterable[str]] = None _gfql_shortest_path_backend: str = "auto" _g: Optional["Plottable"] = None # set only on row-pipeline adapters diff --git a/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py b/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py index b1cf40eb1d..1c0f0bbfaf 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py +++ b/graphistry/compute/gfql/lazy/engine/polars/pattern_apply.py @@ -67,7 +67,7 @@ def rows_binding_ops_polars(g: Plottable, binding_ops: Sequence[Dict[str, JSONVa # decline the whole wavefront-constrained case (wave-2 W2-3). sn = start_nodes.collect() if isinstance(start_nodes, pl.LazyFrame) else start_nodes try: - nodes = nodes.join(sn.select(node_id).unique(), on=node_id, how="semi") + nodes = nodes.join(sn.select(node_id).unique(), on=node_id, how="semi") # type: ignore[operator] # polars op on a DataFrameT-typed seed except Exception: return None try: diff --git a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py index 33cf2d98e7..277e570991 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py +++ b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py @@ -22,6 +22,12 @@ from graphistry.compute.gfql.expr_parser import ExprNode, FunctionCall from graphistry.compute.ast import ASTObject + # Within ONE call the path bag and its per-alias frames are the same polars + # flavour — the generic builder works in LazyFrames, the indexed one in eager + # frames. A constrained TypeVar says that; a plain union does not, and then + # `state.join(lookup)` cannot type-check. + PolarsFrameT = TypeVar("PolarsFrameT", "pl.DataFrame", "pl.LazyFrame") + from graphistry.Plottable import Plottable from graphistry.utils.json import JSONVal # Engine-neutral wire-format payload types (ASTCall.params). Shapes are safelist-validated @@ -639,11 +645,8 @@ def _rewrap(g: Plottable, table_df: Any) -> Plottable: def _finish_binding_rows_polars( g: Plottable, ops: "Sequence[ASTObject]", - # Deliberately Any, not a polars union: the SAME parameter receives a polars - # eager frame, a polars LazyFrame, and the engine-polymorphic `DataFrameT` the - # indexed helper returns. A precise union here only buys `# type: ignore`s. - state: Any, - alias_frames: Dict[str, Any], + state: "PolarsFrameT", + alias_frames: Dict[str, "PolarsFrameT"], node_id: str, attach_prop_aliases: Optional[Sequence[str]], *, @@ -661,7 +664,7 @@ def _finish_binding_rows_polars( from graphistry.compute.ast import ASTEdge, ASTNode from graphistry.compute.gfql.lazy import collect as _lazy_collect - def names(frame: Any) -> List[str]: + def names(frame: "PolarsFrameT") -> List[str]: return ( frame.collect_schema().names() if isinstance(frame, pl.LazyFrame) @@ -1148,7 +1151,7 @@ def _names(lf: pl.LazyFrame) -> List[str]: sn = _df_to_engine(sn, _Engine.POLARS) if node_id not in sn.columns: return None - seed_ids = sn.select(pl.col(node_id)).drop_nulls() + seed_ids = sn.select(pl.col(node_id)).drop_nulls() # type: ignore[operator] # polars op on a DataFrameT-typed seed if seed_ids.height != seed_ids.unique().height: return None seed_ids_lf = seed_ids.lazy() @@ -1203,8 +1206,11 @@ def _names(lf: pl.LazyFrame) -> List[str]: return _finish_binding_rows_polars( g, ops, - indexed_state.state, - indexed_state.alias_frames, + # engine-polymorphic by declaration; on the polars branch the helper + # builds polars frames, so narrow once here rather than widening the + # finisher's contract back to Any + cast("pl.DataFrame", indexed_state.state), + cast(Dict[str, "pl.DataFrame"], indexed_state.alias_frames), str(node_id), attach_prop_aliases, decline_on_schema_error=False, # our own state: a schema clash is a bug @@ -1471,7 +1477,12 @@ def _names(lf: pl.LazyFrame) -> List[str]: alias_frames[next_alias] = next_nodes node_aliases.append(next_alias) - return _finish_binding_rows_polars( + # The finisher's frame type is a constrained TypeVar so `state.join(lookup)` + # type-checks. The GENERIC builder above mixes eager and lazy frames across + # its (pre-existing) branches, so inference cannot pick one here; the + # indexed caller binds cleanly. Narrow just this call rather than widening + # the finisher back to `Any`. + return _finish_binding_rows_polars( # type: ignore[misc] g, ops, state, alias_frames, node_id, attach_prop_aliases, decline_on_schema_error=True, # pandas-vs-polars join-key dtype divergence ) diff --git a/graphistry/compute/gfql/row/pipeline.py b/graphistry/compute/gfql/row/pipeline.py index 6c485acc1d..5aec80b971 100644 --- a/graphistry/compute/gfql/row/pipeline.py +++ b/graphistry/compute/gfql/row/pipeline.py @@ -5,7 +5,7 @@ import warnings from functools import lru_cache from types import ModuleType -from typing import TYPE_CHECKING, Any, Callable, Dict, List, Mapping, NoReturn, Optional, Sequence, Tuple, cast +from typing import TYPE_CHECKING, Any, Callable, Dict, Iterable, List, Mapping, NoReturn, Optional, Sequence, Tuple, cast from typing_extensions import Literal import pandas as pd @@ -81,6 +81,7 @@ if TYPE_CHECKING: from graphistry.Plottable import Plottable + from graphistry.compute.typing import DataFrameT from graphistry.compute.gfql.index.handoff import IndexedBindingsHandoff from graphistry.compute.gfql.index.registry import GfqlIndexRegistry from graphistry.compute.ast import ASTObject @@ -206,10 +207,10 @@ class RowPipelineMixin: _gfql_indexed_bindings_handoff: Optional["IndexedBindingsHandoff"] = None _gfql_shortest_path_backend: str = "auto" - _g: Any - _gfql_start_nodes: Any = None - _gfql_rows_base_graph: Any = None - _gfql_rows_edge_aliases: Any = None + _g: Optional["Plottable"] = None + _gfql_start_nodes: Optional["DataFrameT"] = None + _gfql_rows_base_graph: Optional["Plottable"] = None + _gfql_rows_edge_aliases: Optional[Iterable[str]] = None _nodes: Any _edges: Any _node: Any @@ -5205,6 +5206,7 @@ def __init__(self, g: "Plottable") -> None: self._edge = g._edge def bind(self) -> "Plottable": + assert self._g is not None # set by __init__; kept Optional for the protocol return self._g.bind()