Merge pull request #1842 from graphistry/perf/gfql-transformer-class-once

perf(gfql): re-land #1837 onto master (transformer class built once, 2.5-2.8x compile)

Author: lmeyerov

CHANGELOG.md

@@ -16,6 +16,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.

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(

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"
+    )