perf(gfql): build the row-expression transformer class once, not per parse (compile 2.5-2.8x)

graphistry/compute/gfql/expr_parser.py

@@ -333,7 +333,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:
@@ -709,7 +734,17 @@ 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
+
+
+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_clear_caches_covers_every_cache.py

@@ -53,6 +53,11 @@
         "Lark LALR(1) tables for the flat WHERE-chain start rule; same reasoning",
     "_parser":
         "Lark LALR(1) tables for the row-expression grammar; same reasoning",
+    "_ast_builder_class":
+        "returns the row-expression Transformer CLASS, a function of the code; it is cached "
+        "because @dataclass re-execs generated __init__/__eq__ source on every rebuild, which "
+        "was 40% of GFQL compile time. Instances are still created per parse, so nothing "
+        "stateful is shared",
     "_where_rows_expr_parser_fn":
         "binds imported callables and returns None when lark is absent; a resolved import "
         "is not stale state, and re-resolving it cannot change the answer",

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