From 35cf7daca809fe0a52c78286b1b06a089411a396 Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Wed, 29 Jul 2026 19:08:49 -0700 Subject: [PATCH 1/3] fix(gfql): gfql_clear_caches cleared the wrong name and silently did nothing The Cypher AST memo is an lru_cache on _parse_cypher_cached; parse_cypher itself carries no cache. gfql_clear_caches() looked up cache_clear with getattr(obj, ..., None) and skipped a None result, so naming parse_cypher made the call a silent no-op and every 'cold-process' number measured through it was wrong. - parser.py gains clear_cypher_parser_caches(), mirroring clear_expr_parser_caches - every clear in gfql_clear_caches is now UNCONDITIONAL: a wrong name raises - the 3 Lark LALR parser lru_caches stay uncleared on purpose (grammar, not input) - new test enumerates every @lru_cache in the GFQL tree by AST and fails unless it is either cleared or exempted with a written reason Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YYZRXegrALuXd3NHH5evqx --- graphistry/compute/gfql/cypher/parser.py | 17 ++ graphistry/compute/gfql_unified.py | 24 ++- .../test_clear_caches_covers_every_cache.py | 190 ++++++++++++++++++ 3 files changed, 222 insertions(+), 9 deletions(-) create mode 100644 graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py diff --git a/graphistry/compute/gfql/cypher/parser.py b/graphistry/compute/gfql/cypher/parser.py index d4814ad9a6..9a4fda8cb7 100644 --- a/graphistry/compute/gfql/cypher/parser.py +++ b/graphistry/compute/gfql/cypher/parser.py @@ -2192,6 +2192,23 @@ def _parse_cypher_cached(query: str) -> Union[CypherQuery, CypherUnionQuery, Cyp return node +def clear_cypher_parser_caches() -> None: + """Empty the Cypher query->AST memo. Called by ``gfql_clear_caches``. + + Clears ``_parse_cypher_cached`` ONLY. The three ``@lru_cache(maxsize=1)`` Lark parser + objects (``_parser_lalr``, ``_pattern_parser``, ``_where_predicate_chain_parser``) are + deliberately left alone: they hold the LALR(1) parse tables, which are a pure function + of the grammar rather than of any query, are built once per process, and cost far more + to rebuild than any query parse. Emptying them would make a "cold" measurement include + grammar construction -- a cost no caller can ever pay twice. + + Note ``parse_cypher`` itself carries NO cache and therefore no ``cache_clear``; the memo + lives on the ``_parse_cypher_cached`` body it delegates to. Clearing the wrong name here + silently did nothing for as long as it went unnoticed. + """ + _parse_cypher_cached.cache_clear() + + def _validate_graph_bindings(node: Union[CypherQuery, CypherGraphQuery]) -> None: """Validate graph binding names: no duplicates, no forward/circular refs.""" bindings = node.graph_bindings diff --git a/graphistry/compute/gfql_unified.py b/graphistry/compute/gfql_unified.py index 93d8510bec..7c435958e8 100644 --- a/graphistry/compute/gfql_unified.py +++ b/graphistry/compute/gfql_unified.py @@ -1597,18 +1597,24 @@ def gfql_clear_caches() -> None: server cannot reclaim the memory on demand). Caches keyed to a specific graph are NOT touched: they live on their ``Plottable`` and - die with it. + die with it. Neither are the process-lifetime *singletons* -- Lark parser objects, + compiled regexes, dependency probes -- which are a function of the code, not of any + input; see ``clear_cypher_parser_caches`` and + ``graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py``, which fails + if a new memo appears and is neither cleared here nor exempted with a written reason. + + Every clear is UNCONDITIONAL. An earlier version looked its targets up with + ``getattr(obj, "cache_clear", None)`` and skipped whatever came back ``None``, so + naming the wrong object -- ``parse_cypher``, whose memo actually lives on the + ``_parse_cypher_cached`` body it delegates to -- turned the whole call into a silent + no-op. A cache-clearing function that cannot fail is indistinguishable from one that + does nothing. """ with _COMPILED_STRING_QUERY_CACHE_LOCK: _COMPILED_STRING_QUERY_CACHE.clear() - for cached in (parse_cypher,): - clear = getattr(cached, "cache_clear", None) - if clear is not None: - clear() - try: - from graphistry.compute.gfql.expr_parser import clear_expr_parser_caches - except ImportError: - return + from graphistry.compute.gfql.cypher.parser import clear_cypher_parser_caches + clear_cypher_parser_caches() + from graphistry.compute.gfql.expr_parser import clear_expr_parser_caches clear_expr_parser_caches() diff --git a/graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py b/graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py new file mode 100644 index 0000000000..cacd888b12 --- /dev/null +++ b/graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py @@ -0,0 +1,190 @@ +"""``gfql_clear_caches()`` must account for EVERY process-lifetime GFQL cache. + +Not "clear every cache" -- some must survive, and that is the point. The rule is that no +memo may be *unaccounted for*: each one is either emptied by ``gfql_clear_caches`` or listed +in ``EXEMPT`` below with a written reason. Adding an ``@lru_cache`` to the GFQL tree and +saying nothing fails this file. + +Why the enumeration is the test rather than a checklist in a docstring: a stale cache that +nobody remembers is a *correctness* bug (results become order-dependent, so a test that +passes alone fails in a suite) and a *measurement* bug. It produced a real published error -- +a "cold-process" benchmark arm was reported as costing 2.3-10.2 ms of query compilation when +the Cypher AST memo was in fact never being emptied, because the clear targeted +``parse_cypher`` while the ``lru_cache`` sits on the ``_parse_cypher_cached`` body it +delegates to. The old ``getattr(obj, "cache_clear", None)`` lookup skipped the miss in +silence. Nothing failed; the number was simply wrong for days. + +STATIC + FUNCTIONAL: the AST scan catches a *new* cache, and the runtime assertions catch a +clear that stops working. +""" + +from __future__ import annotations + +import ast +from pathlib import Path +from typing import Dict, Set + +import pytest + +GFQL_TREE = Path(__file__).resolve().parents[3] / "compute" / "gfql" +GFQL_UNIFIED = Path(__file__).resolve().parents[3] / "compute" / "gfql_unified.py" + + +# Emptied by gfql_clear_caches(). These are keyed by CALLER INPUT, so they grow with traffic +# and their contents change what a later call costs. +CLEARED: Set[str] = { + "_parse_cypher_cached", # cypher query text -> frozen AST (maxsize=512) + "_parse_expr_cached", # row-expression text -> ExprNode (maxsize=1024) +} + +# Deliberately NOT emptied. Every entry is a maxsize=1 singleton that is a pure function of +# the CODE, not of any input: it cannot grow, and rebuilding it costs strictly more than the +# work it saves. Emptying these would make a "cold" measurement include one-time process +# setup that no caller can ever pay twice. +EXEMPT: Dict[str, str] = { + "_parser_lalr": + "Lark LALR(1) tables for the whole-query grammar; function of the grammar, built " + "once per process, and far dearer than any single parse it serves", + "_pattern_parser": + "Lark LALR(1) tables for the pattern-fragment start rule; same reasoning", + "_where_predicate_chain_parser": + "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", + "_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", + "_gfql_expr_runtime_parser_bundle": + "same: an import-resolution bundle, None on a minimal install", + "_gfql_cudf_list_sort_requires_host_bridge": + "probes the installed cuDF version for a segfaulting list-sort; the installed " + "version cannot change inside a process", + "_ddl_prefix_re": + "a compiled regex over a module-level pattern constant", + "_ddl_res": + "compiled regexes over module-level pattern constants", +} + + +def _lru_cached_functions() -> Dict[str, Path]: + """Every ``@lru_cache``/``@cache``-decorated def in the GFQL tree, name -> file.""" + found: Dict[str, Path] = {} + files = sorted(GFQL_TREE.rglob("*.py")) + [GFQL_UNIFIED] + for path in files: + tree = ast.parse(path.read_text(), filename=str(path)) + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + for dec in node.decorator_list: + target = dec.func if isinstance(dec, ast.Call) else dec + name = ( + target.attr if isinstance(target, ast.Attribute) + else target.id if isinstance(target, ast.Name) + else "" + ) + if name in ("lru_cache", "cache"): + found[node.name] = path + return found + + +def test_every_gfql_cache_is_either_cleared_or_exempted_with_a_reason() -> None: + """THE LOCK. A new memo in the GFQL tree must be classified, in writing, right here.""" + discovered = _lru_cached_functions() + assert discovered, "the AST scan found no caches at all -- the scan itself is broken" + + unaccounted = sorted(set(discovered) - CLEARED - set(EXEMPT)) + assert not unaccounted, ( + "these @lru_cache functions are neither cleared by gfql_clear_caches() nor exempted:\n" + + "\n".join(f" {n} ({discovered[n]})" for n in unaccounted) + + "\n\nDecide which it is. If it is keyed by caller input, clear it and add it to " + "CLEARED. If it is a process-lifetime singleton that is a function of the code, add " + "it to EXEMPT with the reason. Do not leave it silent: an unaccounted cache makes " + "results order-dependent and makes 'cold' measurements wrong." + ) + + stale = sorted((CLEARED | set(EXEMPT)) - set(discovered)) + assert not stale, ( + f"these names are classified here but no longer exist as caches: {stale}. Delete the " + "entries so the lists keep describing the code." + ) + + +def test_every_exemption_carries_a_real_reason() -> None: + """An exemption with an empty or placeholder reason is just a silent cache again.""" + for name, reason in EXEMPT.items(): + assert len(reason.split()) >= 6, f"{name}: reason is too thin to be a reason" + assert "TODO" not in reason.upper(), f"{name}: TODO is not a justification" + + +def test_clear_caches_actually_empties_the_cypher_ast_memo() -> None: + """THE REGRESSION PIN for the published-number bug. + + ``parse_cypher`` has no ``cache_clear`` of its own -- the memo is on the + ``_parse_cypher_cached`` body. Clearing the wrong name used to be a silent no-op. + """ + pytest.importorskip("lark") + from graphistry.compute.gfql.cypher import parser as cypher_parser + from graphistry.compute.gfql_unified import gfql_clear_caches + + assert not hasattr(cypher_parser.parse_cypher, "cache_clear"), ( + "parse_cypher grew a cache_clear; the indirection this test guards has changed, so " + "re-read gfql_clear_caches() before relaxing anything here" + ) + + cypher_parser.parse_cypher("MATCH (n) RETURN n") + assert cypher_parser._parse_cypher_cached.cache_info().currsize > 0 + + gfql_clear_caches() + assert cypher_parser._parse_cypher_cached.cache_info().currsize == 0, ( + "gfql_clear_caches() left the Cypher AST memo populated -- every 'cold-process' " + "measurement taken through it is invalid" + ) + + +def test_clear_caches_actually_empties_the_row_expression_memo() -> None: + pytest.importorskip("lark") + from graphistry.compute.gfql import expr_parser + from graphistry.compute.gfql_unified import gfql_clear_caches + + expr_parser.parse_expr("age > 30") + assert expr_parser._parse_expr_cached.cache_info().currsize > 0 + + gfql_clear_caches() + assert expr_parser._parse_expr_cached.cache_info().currsize == 0 + + +def test_clear_caches_leaves_the_lalr_tables_built() -> None: + """The exemption is load-bearing, so pin it: rebuilding the LALR tables on every clear + would put grammar construction inside any 'cold' number measured through this call.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.cypher import parser as cypher_parser + from graphistry.compute.gfql_unified import gfql_clear_caches + + before = cypher_parser._parser_lalr() + gfql_clear_caches() + assert cypher_parser._parser_lalr() is before, ( + "the whole-query LALR parser was rebuilt by gfql_clear_caches()" + ) + + +def test_clear_caches_raises_rather_than_skipping_a_missing_target() -> None: + """FAIL LOUD. If a clear target loses its ``cache_clear``, the call must break, not + quietly do less than it says.""" + pytest.importorskip("lark") + from graphistry.compute.gfql.cypher import parser as cypher_parser + from graphistry.compute.gfql_unified import gfql_clear_caches + + original = cypher_parser._parse_cypher_cached + + class _NoClear: + def __call__(self, query: str) -> object: + raise AssertionError("not called") + + try: + cypher_parser._parse_cypher_cached = _NoClear() # type: ignore[assignment] + with pytest.raises(AttributeError): + gfql_clear_caches() + finally: + cypher_parser._parse_cypher_cached = original # type: ignore[assignment] + + gfql_clear_caches() # and it still works once the real target is back From 03dd18ee7d02cf42578780ebad9a1f85fab8f48f Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 1 Aug 2026 17:33:32 -0700 Subject: [PATCH 2/3] fix(gfql): audit found one more uncleared memo; guard now sees dict-style caches Audit of every GFQL-tree file for process-lifetime caches (2026-08-01): - 11 @lru_cache functions: all already classified (2 cleared, 9 exempt singletons). - ONE unaccounted cache: _SINGLE_ALIAS_CACHE in the polars row pipeline, an OrderedDict keyed by (predicate string, alias, schema). Keyed by caller input, so it belongs to the class gfql_clear_caches promises to empty; it sat in the guard's blind spot because the AST scan only saw decorators. - Config registries (_COST_GATE_FRAC_OVERRIDES, lazy-mode overrides) and per-invocation locals are correctly out of scope: clearing them would change semantics, not cost. gfql_clear_caches now empties the single-alias lowering memo unconditionally (module import is safe on polars-free installs; its polars imports are all TYPE_CHECKING-guarded). The guard test gains a second AST scan for module-level dict/OrderedDict/set bindings named cache/memo, so the next hand-rolled memo fails the lock instead of hiding, and a functional pin proves the new clear works. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr --- .../gfql/lazy/engine/polars/row_pipeline.py | 10 +++ graphistry/compute/gfql_unified.py | 11 ++- .../test_clear_caches_covers_every_cache.py | 77 ++++++++++++++++++- 3 files changed, 94 insertions(+), 4 deletions(-) diff --git a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py index f8d80f1834..26f6967d58 100644 --- a/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py +++ b/graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py @@ -724,6 +724,16 @@ def _bare_column_ast(node: ExprNode, alias: str) -> Optional[ExprNode]: _SINGLE_ALIAS_CACHE_MAX = 512 +def clear_single_alias_predicate_cache() -> None: + """Empty the ``lower_single_alias_predicate`` memo. + + Called by ``gfql_clear_caches``: this memo is keyed by caller input (predicate string, + alias, schema), so it grows with traffic and its contents change what a later call + costs -- exactly the cache class that clear function promises to empty. + """ + _SINGLE_ALIAS_CACHE.clear() + + def _single_alias_cache_key( expr: str, alias: str, schema: Mapping[str, "pl.DataType"], columns_nan_free: bool ) -> _SingleAliasKey: diff --git a/graphistry/compute/gfql_unified.py b/graphistry/compute/gfql_unified.py index 7c435958e8..6f996cb6cf 100644 --- a/graphistry/compute/gfql_unified.py +++ b/graphistry/compute/gfql_unified.py @@ -1590,8 +1590,9 @@ def _node_dtypes_cache_key( def gfql_clear_caches() -> None: """Empty every PROCESS-LIFETIME GFQL cache. - GFQL memoizes work that is a pure function of its inputs: compiled plans here, and the - parse caches behind ``parse_cypher`` and the row-expression parser. All are bounded, so + GFQL memoizes work that is a pure function of its inputs: compiled plans here, the + parse caches behind ``parse_cypher`` and the row-expression parser, and the polars + single-alias predicate-lowering memo. All are bounded, so this is not a leak valve -- it exists because a process-lifetime cache that cannot be emptied is untestable (results become order-dependent) and unbudgetable (a long-lived server cannot reclaim the memory on demand). @@ -1616,6 +1617,12 @@ def gfql_clear_caches() -> None: clear_cypher_parser_caches() from graphistry.compute.gfql.expr_parser import clear_expr_parser_caches clear_expr_parser_caches() + # Import is safe on polars-free installs: the module's polars imports are all + # TYPE_CHECKING-guarded, so the memo exists (empty) even when the engine cannot run. + from graphistry.compute.gfql.lazy.engine.polars.row_pipeline import ( + clear_single_alias_predicate_cache, + ) + clear_single_alias_predicate_cache() def _compile_string_query( diff --git a/graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py b/graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py index cacd888b12..cd7fd347bd 100644 --- a/graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py +++ b/graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py @@ -33,8 +33,10 @@ # Emptied by gfql_clear_caches(). These are keyed by CALLER INPUT, so they grow with traffic # and their contents change what a later call costs. CLEARED: Set[str] = { - "_parse_cypher_cached", # cypher query text -> frozen AST (maxsize=512) - "_parse_expr_cached", # row-expression text -> ExprNode (maxsize=1024) + "_parse_cypher_cached", # cypher query text -> frozen AST (maxsize=512) + "_parse_expr_cached", # row-expression text -> ExprNode (maxsize=1024) + "_COMPILED_STRING_QUERY_CACHE", # (query, engine, params, dtypes) -> plan (max 128) + "_SINGLE_ALIAS_CACHE", # (expr, alias, schema) -> lowered polars expr (max 512) } # Deliberately NOT emptied. Every entry is a maxsize=1 singleton that is a pure function of @@ -87,10 +89,59 @@ def _lru_cached_functions() -> Dict[str, Path]: return found +def _module_dict_caches() -> Dict[str, Path]: + """Every MODULE-LEVEL dict/OrderedDict/set binding whose name says cache/memo. + + The decorator scan above cannot see hand-rolled memos -- ``_SINGLE_ALIAS_CACHE`` in the + polars row pipeline sat exactly in that blind spot: an ``OrderedDict`` keyed by caller + input, invisible to an ``@lru_cache`` scan, and silently absent from + ``gfql_clear_caches`` until the 2026-08-01 audit. This scan is name-based (``cache`` or + ``memo`` in the binding name, case-insensitive) over top-level assignments only: a memo + whose name hides what it is has worse problems than this file can catch. + """ + found: Dict[str, Path] = {} + files = sorted(GFQL_TREE.rglob("*.py")) + [GFQL_UNIFIED] + for path in files: + tree = ast.parse(path.read_text(), filename=str(path)) + for node in tree.body: # top-level only: function locals die with the call + targets = [] + if isinstance(node, ast.Assign): + targets = [t for t in node.targets if isinstance(t, ast.Name)] + value = node.value + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + targets = [node.target] + value = node.value + else: + continue + if value is None: + continue + is_container = ( + isinstance(value, (ast.Dict, ast.Set)) + or ( + isinstance(value, ast.Call) + and isinstance(value.func, ast.Name) + and value.func.id in ("dict", "OrderedDict", "WeakValueDictionary", "set") + ) + ) + if not is_container: + continue + for target in targets: + lowered = target.id.lower() + if "cache" in lowered or "memo" in lowered: + found[target.id] = path + return found + + def test_every_gfql_cache_is_either_cleared_or_exempted_with_a_reason() -> None: """THE LOCK. A new memo in the GFQL tree must be classified, in writing, right here.""" discovered = _lru_cached_functions() assert discovered, "the AST scan found no caches at all -- the scan itself is broken" + dict_caches = _module_dict_caches() + assert dict_caches, ( + "the module-dict scan found nothing, yet _COMPILED_STRING_QUERY_CACHE and " + "_SINGLE_ALIAS_CACHE exist -- the scan itself is broken" + ) + discovered = {**discovered, **dict_caches} unaccounted = sorted(set(discovered) - CLEARED - set(EXEMPT)) assert not unaccounted, ( @@ -188,3 +239,25 @@ def __call__(self, query: str) -> object: cypher_parser._parse_cypher_cached = original # type: ignore[assignment] gfql_clear_caches() # and it still works once the real target is back + + +def test_clear_caches_actually_empties_the_single_alias_lowering_memo() -> None: + """THE 2026-08-01 AUDIT PIN. ``_SINGLE_ALIAS_CACHE`` is an OrderedDict, not an + ``@lru_cache``, so the decorator scan never saw it and ``gfql_clear_caches`` never + cleared it: a populated lowering memo makes a 'cold' polars row-pipeline measurement + warm without any visible failure.""" + pytest.importorskip("lark") + pl = pytest.importorskip("polars") + from graphistry.compute.gfql.lazy.engine.polars import row_pipeline + from graphistry.compute.gfql_unified import gfql_clear_caches + + schema = pl.DataFrame({"age": [1]}).schema + row_pipeline.lower_single_alias_predicate("age > 30", "n", schema) + assert len(row_pipeline._SINGLE_ALIAS_CACHE) > 0, ( + "the probe call did not populate the memo; fix the probe before trusting this test" + ) + + gfql_clear_caches() + assert len(row_pipeline._SINGLE_ALIAS_CACHE) == 0, ( + "gfql_clear_caches() left the single-alias lowering memo populated" + ) From 4a3f7f870c2863274aa83480c3167a53f1074e0e Mon Sep 17 00:00:00 2001 From: Leo Meyerovich Date: Sat, 1 Aug 2026 17:49:03 -0700 Subject: [PATCH 3/3] test(gfql): run the cache-coverage lock in the polars lane The new functional pin for _SINGLE_ALIAS_CACHE needs polars installed, and polars exists only in the test-polars lane's explicit file list. The lane completeness meta-test caught the omission. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr --- bin/test-polars.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/test-polars.sh b/bin/test-polars.sh index d765fff687..3a1740d3d5 100755 --- a/bin/test-polars.sh +++ b/bin/test-polars.sh @@ -22,6 +22,9 @@ python -m pytest --version # fails if any module-level polars-gated test file is absent from it (or listed but gone). POLARS_TEST_FILES=( graphistry/tests/compute/test_polars.py + # cache-coverage lock: its static scans run everywhere, but the functional pin for the + # polars single-alias lowering memo can only execute where polars is installed + graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py graphistry/tests/compute/gfql/test_engine_polars_hop.py graphistry/tests/compute/gfql/test_engine_polars_chain.py graphistry/tests/compute/gfql/test_engine_polars_row_pipeline.py