Skip to content

Commit 2c72119

Browse files
lmeyerovclaude
andcommitted
test(gfql): decide #1793's clear-vs-restore question, and pin it (#1793 review)
Review asked whether `clear_row_exec_context` should RESTORE the value the graph carried on entry rather than NULLing it — a fair question, because `attach_row_exec_context` INHERITS on the way in (a `None` argument keeps what `g` already carries), so an outer scope's value can reach an inner execution and be dropped by it. Determination: NULL is correct and restore would reintroduce #1786. Three reasons, each now carried by a test rather than by argument: 1. `clear` is PURE. It returns `g.bind()` and never writes through to the object it was handed, so an outer scope that set the field still has it afterwards. There is no caller state to save. That is exactly what separates this from #1786, which WAS an in-place write onto the caller's own graph — restore is the fix for a mutation, and there is no mutation. 2. The only channel restore would change is the RETURN VALUE, and putting the seed back there IS the second half of #1786 ("the result of a WITH query carries the seed, and a follow-up query on that result is answered against it"). Measured: hand-restoring the seed onto a result changes the answer of the next query on it (7 -> 2 -> 1 rows). 3. No execution frame inherits a context it did not set. Instrumenting `attach` over `graphistry/tests/compute` recorded 3907 calls and ZERO inheriting ones. 53 DID enter on a graph already carrying a seed (nested boundary frames) but each was handed the IDENTICAL `start_nodes` parameter, so the frame still owns what it sets. The cross-segment WITH seed travels as the explicit `start_nodes` PARAMETER (`chain_impl(..., start_nodes=)`, `_compiled_query_reentry_state`), never through the graph field, so the field's lifetime is exactly one boundary-call run. `test_exec_context_scoping.py` re-runs that ownership measurement as an assertion over a corpus of the shapes that reach every attach site, so a future path that starts relying on inheritance reopens this decision loudly instead of silently losing an outer value. MUTATION-CHECKED IN THE DECIDING DIRECTION: implementing save/restore at all three attach sites (chain, native polars chain, gfql_unified) fails 4 of the new tests on every runnable engine — while the existing #1793 suite (`test_reentry_caller_graph_immutability.py`) passes UNCHANGED. Those tests could not distinguish the two designs; this file can, which is why it exists. Engine-parametrized over pandas/polars/cuDF/polars-gpu with a fixed engine list plus a runtime probe (not `available_nonpandas_engines()`, which silently shrinks): a non-runnable engine reports SKIPPED, it does not vanish from the report. Also added to `bin/test-polars.sh` — the file has no module-level `importorskip`, so nothing would otherwise have flagged that its polars params run in no lane (#1795 class). RUNTIME DELTA: zero. No production line changed; the only non-test edits are the `clear_row_exec_context` docstring and a CHANGELOG entry, so no pyg-bench lane run is required (CB5). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
1 parent 233b64c commit 2c72119

4 files changed

Lines changed: 336 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
2424
- **Seeded typed-hop property projection keeps its fast path through DISTINCT / ORDER BY / SKIP / LIMIT**: those trailing row ops are plain frame operations, so the fast path now delegates them to the canonical chain instead of declining the whole query.
2525

2626
### Changed
27+
- **Row execution context: `clear_row_exec_context` NULLs rather than restores, and that is now pinned rather than assumed** (tests + comments only; zero runtime delta — no production line changed). Review of #1793 asked whether the exit should RESTORE what the graph carried on entry instead of nulling, since `attach_row_exec_context` INHERITS on the way in (a `None` argument keeps what `g` already carries). Settled as NULL, for three reasons now each carried by a test: (1) `clear` is pure — it returns `g.bind()` and never writes through to the object it was handed, so an outer scope that set the field still has it afterwards and there is no caller state to save; that is what separates this from #1786, which WAS an in-place write onto the caller's own graph, and restore is the fix for a mutation. (2) The only channel restore would change is the RETURN value, and putting the seed back there IS the second half of #1786 — measured, a seed hand-restored onto a result changes the answer of the next query run on that result (7 → 2 → 1 rows). (3) No execution frame inherits a context it did not set: instrumenting `attach` over `graphistry/tests/compute` recorded 3907 calls and zero inheriting ones (53 entered on a graph already carrying a seed, but each was handed the identical `start_nodes` parameter, so the frame still owns the value), because the cross-segment `WITH` seed travels as the explicit `start_nodes` PARAMETER and never through the graph field. The new `test_exec_context_scoping.py` re-runs that ownership measurement as an assertion, so a future path that starts relying on inheritance reopens the decision loudly instead of silently losing an outer value. Mutation-checked in the deciding direction: implementing save/restore at all three attach sites fails 4 of the new tests on every runnable engine, while the existing #1793 suite passes unchanged — i.e. those tests could not distinguish the two designs and this file can.
2728
- **`is_lazy` is a type predicate, so the polars engine's eager/lazy split is checked instead of asserted**: `dtypes.is_lazy` decides WHICH member of the two-member `PolarsFrame` union a frame is, but it was declared `-> bool`, so that answer was discarded at the call boundary — an eager-only attribute (`.height`, `.columns`, `.schema`) reached after a lazy guard was unprovable, and the native polars chain closed the gap with `cast("pl.DataFrame", frame)`. A cast is not a type: it re-asserts, unchecked, the exact fact the predicate had just established, once per call site, and a wrong one surfaces as a production `AttributeError` rather than a CI error. `is_lazy` is now declared `-> TypeIs["pl.LazyFrame"]` (PEP 742), which narrows the *negative* branch as well as the positive — the eager side is the one that needed it, so `TypeGuard` would not have done — and the cast is gone. Four previously untyped chain-combine helpers (`_semi`, `_combine_edges`, `_apply_node_names`, and the `_known_empty` guard) now carry real signatures: `_semi`'s two frames share the `PolarsT` TypeVar because polars joins do not mix eagerness, and the two combine helpers take the `_LazyShim` collect-once duck-type they are actually called with. Typing `_apply_node_names` also retired a `getattr(next_step, "edges_empty", None)` — the shim declares that tri-state in `__slots__`, so a typo is now a checker error instead of a silent `None` that would have re-armed the very cardinality gate the guard disarms. Internal only: no runtime behaviour, no public API, and the `TypeIs` import is `TYPE_CHECKING`-only, so no new runtime dependency floor.
2829
- **GFQL execution context is declared rather than attached at runtime**: the private `_gfql_*` per-execution fields (index policy and registry, row-pipeline base graph, carried seed nodes, edge aliases, shortest-path backend, and the indexed-bindings handoff) are now declared on `Plottable` with defaults on `PlotterBase`, instead of being set with `setattr` and read back with `getattr(..., default)`. No public API or behaviour change; internal call sites are typed, and hand-rolled `Plottable` stand-ins must now construct the full context.
2930

bin/test-polars.sh

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ POLARS_TEST_FILES=(
1919
graphistry/tests/compute/gfql/test_engine_polars_row_pipeline.py
2020
graphistry/tests/compute/gfql/test_engine_polars_binding_rows.py
2121
graphistry/tests/compute/gfql/test_engine_polars_with_match_reentry.py
22+
# engine-parametrized: its pandas params run in test-gfql-core, but the polars params
23+
# only ever run here (the file has no module-level importorskip, so nothing else flags it)
24+
graphistry/tests/compute/gfql/test_exec_context_scoping.py
2225
graphistry/tests/compute/gfql/test_engine_polars_cypher_conformance.py
2326
graphistry/tests/compute/gfql/test_engine_polars_conformance_matrix.py
2427
graphistry/tests/compute/gfql/test_polars_string_predicate_nonstring.py

graphistry/compute/gfql/exec_context.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,29 @@ def clear_row_exec_context(g: "Plottable") -> "Plottable":
4848
``WITH`` query carries the seed, and a follow-up query on that result -- a different
4949
graph entirely -- is answered against it (the same #1786 defect, one hop removed).
5050
Returns ``g`` unchanged when there is nothing to strip, so the common path is free.
51+
52+
WHY ``None`` AND NOT "RESTORE WHAT ``g`` CARRIED ON ENTRY". ``attach`` above INHERITS on
53+
the way in (a ``None`` argument keeps what ``g`` already carries), so the asymmetry is
54+
real and the save/restore question is a fair one. Three reasons it is settled the other
55+
way, each pinned by ``tests/compute/gfql/test_exec_context_scoping.py``:
56+
57+
* This function is PURE -- it returns ``g.bind()`` and never writes through to the object
58+
it was handed, so an outer scope that set the field still has it afterwards. There is no
59+
caller state to save. That is what separates this from #1786, which WAS an in-place
60+
write onto the caller's own graph; restoring is the fix for a mutation, and there is no
61+
mutation here.
62+
* The only channel restore would change is the RETURN VALUE, and putting the seed back
63+
there IS the second half of #1786. Measured: hand-restoring the seed onto a result
64+
changes the answer of the next query run on that result (7 -> 2 -> 1 rows).
65+
* No execution frame ever inherits a context it did not set. The cross-segment ``WITH``
66+
seed travels as the explicit ``start_nodes`` PARAMETER (``chain_impl(..., start_nodes=)``,
67+
``_compiled_query_reentry_state``), never through this field, so the field's lifetime is
68+
exactly ONE boundary-call run. Instrumenting ``attach`` over ``tests/compute`` recorded
69+
3907 calls and ZERO inheriting ones. (53 of them DID enter on a graph already carrying a
70+
seed -- nested boundary frames -- but each was handed the IDENTICAL ``start_nodes``
71+
parameter, so the value is still owned by the frame that sets it.) The test above re-runs
72+
that as an assertion, so a future path that starts relying on inheritance reopens this
73+
decision loudly.
5174
"""
5275
if g._gfql_start_nodes is None and g._gfql_rows_base_graph is None:
5376
return g

0 commit comments

Comments
 (0)