diff --git a/CHANGELOG.md b/CHANGELOG.md index a354a45a13..a7a0e6433c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - **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 compile: the row-expression Transformer class is built once per process, not per parse** (2.5-2.8x compile on the cached-parser path): `@dataclass` re-executed its generated `__init__`/`__eq__` source on every rebuild, ~40% of compile time. Instances are still created per parse, so nothing stateful is shared; the class cache is registered as a process singleton in the GFQL cache registry with a survives-clear pin. - **Compiled Cypher plans are shared process-wide instead of per-`Plottable`, so a one-shot query stops recompiling a plan the process already has.** `compile_cypher_query(parse_cypher(query), params=..., node_dtypes=...)` never sees the graph, so its result is a pure function of the query text, params, node dtypes and resolved engine. The cache nevertheless hung off the caller's `Plottable` by `setattr`, which partitioned it by something that cannot change the answer: the *second* query on a graph was fast and the *first* was not, and a one-shot query was always the first. Measured on dgx-spark under the perf lock at graph-benchmark 20k, that cost the first query on a `Plottable` **+1.6 to +2.5 ms (+21% to +52%)** versus the second, on q3/q4/q5/q7/q9 alike, while a bind-only control cost 0.02 ms. The cache is now a bounded module-level LRU (128 entries) under a lock, keyed by `(language, query, params, node dtypes, resolved engine)`; values are `@dataclass(frozen=True)` chains and plans, so no DataFrame is reachable from a cached entry and a process-lifetime cache cannot pin user data. Removing the `setattr` also drops one of the three `plottable-setattr` sites the type-hygiene guard tracks. Position-balanced A/B against master `99c149758` (Kuzu 0.11.3 in the same session, cold binding, RUNS=51, 4 slots per arm, per-slot medians, values byte-identical on every arm) moves **every** cell at both scales by +0.6 to +2.7 ms and changes four verdicts: at 20k the board goes 4W/1T/4L to **5W/2T/2L** (q5 loss to win, q4 loss to tie) and at 100k 5W/2T/2L to **6W/2T/1L** (q4 tie to win, q7 loss to tie). - **The polars two-hop `count(*)` builds ONE lazy plan instead of five eager collects**: `MATCH (a {..})-[{..}]->(b {..})-[{..}]->(c {..}) [WHERE ..] RETURN count(*)` lowers to a fast path that computes the answer as a degree product — semi-join each edge arm against its two node domains, count in/out degree per middle node, sum `in*out`. Every one of those ops was issued EAGERLY, i.e. as its own `lazy().collect()`, so each intermediate materialized in full and with every edge column still attached, and no filter or projection could be pushed across an op boundary: profiled on the 20k graph-benchmark dataset the four-semi-join chain built a **199,939-row** wide frame that the degree join immediately reduced to 120,586, and 88–96% of the query's wall time sat inside those collects. The DISTINCT-domain case — the three node domains and/or the two edge matches are not all equal, which is what a `WHERE` on the middle or end node produces — is now expressed as a single lazy plan collected once, so polars pushes the src/dst projection into the semi-joins. The algebra is unchanged (same semi-joins, same `group_by().len()`, same `(in*out).sum().fill_null(0).cast(Int64)`), so the value is identical including openCypher's count-over-no-rows `0` on an empty match. The EQUAL-domain case is deliberately NOT routed through it: that shape takes a degree-count branch whose counts are memoized on the Plottable across calls, and fusing it would trade a cross-call cache hit for a per-call replan — it is byte-identical to before, and a test asserts the fused lane is not even *called* for it. The lane DECLINES (falling through to the untouched eager code, never answering differently) for non-eager-polars frames and for an edge column already carrying a degree-counter name. Not a GPU change: like the eager code and the fused two-star lane, it collects on CPU polars for both `polars` and `polars-gpu`. **Measured** on dgx-spark, matched cross-engine graph-benchmark lane (all nine OLAP queries, same query text per engine), one perf lock per scale, position-balanced `K M S S M K` slots ×2, per-slot medians (never best-of), rows and canonical values compared on every cell: **`engine='polars'` distinct-domain two-hop `count(*)` `16.91 → 10.02 ms` at 20k (−40.7%) and `68.04 → 37.96 ms` at 100k (−44.2%)**, per-slot ranges non-overlapping at both scales (20k 15.90–18.05 vs 9.23–10.29; 100k 65.9–69.7 vs 36.5–39.3). Against same-session embedded Kuzu that turns the 20k cell from a 1.55× LOSS into a **1.09× WIN** (Kuzu 10.90 ms, ranges non-overlapping) and widens the 100k cell from a 1.25× win to **2.23×** (Kuzu 84.73 ms). The equal-domain cell is UNCHANGED by this PR at both scales (20k 2.44 → 2.30 ms, 100k 5.46 → 5.62 ms — ranges overlap = TIE, which is the point of the measurement). That cell's headline number is NOT one-shot-honest and this changelog does not claim it as a win: the degree counts it reports are memoized across calls onto the caller's `Plottable` (#1825), so a warm repeat call is fast while a one-shot query loses to embedded Kuzu; this PR neither causes nor fixes #1825. What this PR does do for that shape is make its MEMO-MISS branch — the branch a one-shot query, a rebound `Plottable`, or any future removal of the cross-call memo actually runs — build the two degree frames as ONE lazy plan (`collect_all` over a shared filtered-edge sub-plan) instead of materializing the whole filtered edge frame and grouping it twice eagerly. Same algebra, same values; the memo HIT returns before reaching it, so a warm call is untouched. **Measured** the same way (dgx-spark, perf lock, position-balanced `A B B A B A A B` slots, per-slot medians, 21 runs + 5 warmup per slot, value- and row-identical across arms): equal-domain memo-MISS **`10.94 → 8.25 ms` at 20k (−24.5%, ranges 10.24–11.10 vs 7.65–8.71, non-overlapping)** and **`48.27 → 33.13 ms` at 100k (−31.4%, 47.65–48.89 vs 32.68–33.66, non-overlapping)**; the same query issued ONE-SHOT on a fresh `Plottable` goes `12.19 → 9.18 ms` at 20k and `49.67 → 34.62 ms` at 100k. The memo-HIT cell (20k 1.84 → 2.14 ms, 100k 5.41 → 5.84 ms), the distinct-domain cell, and a `bind_only` control all come back as TIEs, which is what shows the change is confined to the miss branch. Every other cell on both engines is a TIE. The pandas arm — untouched by this change — reproduces the reference board to +0.3%…+4.1%, which is what shows the harness matches it. Value identity is the gate throughout: a differential over 972 query shapes × 6 graphs (5,832 comparisons, 4,815 with the fused lane engaged) found zero divergences from the eager code or from the pandas oracle, and pinned tests cover multiplicity (parallel edges, self-loops, duplicate node rows), empty matches, non-numeric ids, degenerate column bindings, and both declines. diff --git a/graphistry/compute/gfql/expr_parser.py b/graphistry/compute/gfql/expr_parser.py index 08d3aadc2f..46d40fea2a 100644 --- a/graphistry/compute/gfql/expr_parser.py +++ b/graphistry/compute/gfql/expr_parser.py @@ -341,7 +341,32 @@ def _parse_number_token(token: str) -> Union[int, float]: return int(token) -def _build_transformer() -> _TransformerLike: +@lru_cache(maxsize=1) +def _ast_builder_class() -> Callable[[], _TransformerLike]: + """Define the row-expression transformer class ONCE per process, and return the class. + + This used to be the body of ``_build_transformer``, which meant the three types below -- + two ``@dataclass(frozen=True)`` helpers and the Transformer itself -- were re-created on + every call. ``@dataclass`` generates ``__init__``/``__eq__``/``__hash__`` as source and + ``exec``s it, so each rebuild ran the compiler. + + That was the single largest cost in GFQL query compilation. Profiling q7 of the matched + graph-benchmark (31 cold compiles, 0.649 s total) attributed **0.261 s -- 40% of all + compile time -- to this function**: 434 calls (one per ``_parse_expr_cached`` miss) + producing 868 dataclass creations, 0.120 s of which was ``builtins.exec`` alone. By + comparison the whole LALR(1) parse was 0.151 s. Lowering, not parsing, dominates GFQL's + compile, and this was most of lowering. + + Returning the CLASS rather than an instance is deliberate: ``_build_transformer`` still + constructs a fresh transformer per call, so no state is shared between parses and the + change is a pure construction-cost win with no re-entrancy or thread-safety question to + argue about. Only the type creation is hoisted, and a type is a function of the code. + + Cached with ``maxsize=1`` rather than defined at module scope because ``Transformer`` comes + from ``_lark_imports()``, which is lazy: a module-level class statement would make importing + this module require lark, breaking the minimal installs that only touch the non-expression + surfaces. + """ _, Transformer, _ = _lark_imports() def _is_token(value: Any) -> bool: @@ -717,7 +742,25 @@ def is_null(self, items: Sequence[Any]) -> IsNullOp: def is_not_null(self, items: Sequence[Any]) -> IsNullOp: return IsNullOp(value=cast(ExprNode, _strip_tokens(items)[0]), negated=True) - return cast(_TransformerLike, _AstBuilder()) + return _AstBuilder + + +register_process_singleton( + _ast_builder_class, + "returns the row-expression Transformer CLASS, a function of the code; cached because " + "dataclass re-execs generated source on every rebuild (40 percent of GFQL compile time); " + "instances are still created per parse, so nothing stateful is shared", +) + + +def _build_transformer() -> _TransformerLike: + """A fresh transformer instance, over a class built once per process. + + Instantiation is cheap -- ``_AstBuilder.__init__`` only forwards ``visit_tokens=True`` to + Lark and assigns no attributes. Building the class is what was expensive; see + ``_ast_builder_class``. + """ + return _ast_builder_class()() def _rebuild_expr_node( diff --git a/graphistry/tests/compute/gfql/test_expr_transformer_class_built_once.py b/graphistry/tests/compute/gfql/test_expr_transformer_class_built_once.py new file mode 100644 index 0000000000..a0231f5fb5 --- /dev/null +++ b/graphistry/tests/compute/gfql/test_expr_transformer_class_built_once.py @@ -0,0 +1,88 @@ +"""The row-expression transformer CLASS is built once per process; instances are not shared. + +Why this is worth a test rather than just a commit message: the expensive thing was never +parsing. Profiling q7 of the matched graph benchmark (31 cold compiles, 0.649 s total) put +**0.261 s — 40% of all compile time — in ``_build_transformer``**, which was re-creating two +``@dataclass(frozen=True)`` helpers and a Lark ``Transformer`` subclass on every +``_parse_expr_cached`` miss: 434 calls producing 868 dataclass creations, 0.120 s of it in +``builtins.exec`` because ``@dataclass`` generates ``__init__``/``__eq__`` as source and execs +it. The whole LALR(1) parse, by comparison, was 0.151 s. + +So the invariant has two halves and both matter. Build the class **once** (or the cost returns), +and keep instantiating **per parse** (or a future stateful transformer silently starts sharing +state between unrelated queries). A test that only checked the first half would bless a +regression into the second. +""" + +from __future__ import annotations + +import pytest + + +def test_the_transformer_class_is_created_once_per_process() -> None: + """The whole point: identical class object, so no dataclass rebuild.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import _ast_builder_class + + first = _ast_builder_class() + assert _ast_builder_class() is first, ( + "the transformer class is being rebuilt; @dataclass will re-exec generated source on " + "every row-expression parse, which was 40% of GFQL compile time" + ) + + +def test_each_parse_still_gets_its_own_transformer_instance() -> None: + """The other half. Sharing one instance would be a bigger win and a worse idea: Lark + transformers are only incidentally stateless here, and a shared instance would make that + an unwritten requirement of every future method added to the class.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import _ast_builder_class, _build_transformer + + a = _build_transformer() + b = _build_transformer() + assert a is not b, "transformer instances are being shared between parses" + assert type(a) is type(b) is _ast_builder_class() + + +def test_the_transformer_class_survives_a_cache_clear() -> None: + """``gfql_clear_caches()`` must NOT drop it — it is a function of the code, not of any + query, exactly like the Lark parser objects. Clearing it would put class construction back + inside every 'cold' measurement, which is a cost no caller can pay twice.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import _ast_builder_class + from graphistry.compute.gfql_unified import gfql_clear_caches + + before = _ast_builder_class() + gfql_clear_caches() + assert _ast_builder_class() is before + + +@pytest.mark.parametrize( + "expr", + [ + "age > 30", + "toLower(name) = 'bob'", + "a.x IS NULL", + "a.x IS NOT NULL", + "CASE WHEN age > 30 THEN 1 ELSE 0 END", + "count(DISTINCT a.id)", + "(age > 30) AND (score < 5.5)", + "a.x IN [1, 2, 3]", + ], +) +def test_parsing_is_unchanged_by_the_hoist(expr: str) -> None: + """Parity. Hoisting a class definition must not move a single AST node. + + Also exercises the two hoisted dataclasses specifically: ``_CaseArm`` via CASE/WHEN and + ``_FunctionArgs`` via a DISTINCT aggregate — they are closed over by the transformer, so a + botched hoist would surface here rather than in a generic expression. + """ + pytest.importorskip("lark") + from graphistry.compute.gfql.expr_parser import parse_expr + from graphistry.compute.gfql_unified import gfql_clear_caches + + first = repr(parse_expr(expr)) + gfql_clear_caches() + assert repr(parse_expr(expr)) == first, ( + f"{expr!r} parses differently across a cache clear" + )