fix(gfql): OPTIONAL MATCH + WITH pipeline row semantics (#1896) by lmeyerov · Pull Request #1897 · graphistry/pygraphistry

lmeyerov · 2026-08-14T10:33:08Z

Stacked on #1895. Fixes all four silent-wrong shapes from the #1896 re-probe umbrella:

CRITICAL guard bypass — terminal WITH after OM rode the single-alias row pipeline (pandas: nulled+collapsed; polars: fabricated seed ids into the optional column). New flatten_terminal_with_over_optional routes pure bare-alias carries onto the connected left-join lowering with the stage WHERE as a post-join binding-row filter — exact openCypher row semantics, which also makes the canonical WITH p, x WHERE x IS NULL anti-join answer correctly on both engines.
Zero-count groups — passthrough WITH-aggregates fold to direct RETURN aggregates (null-keeping group keys; count(b)=0 preserved).
3+4. Anonymous null fill — apply_optional_reentry_null_fill count-arithmetic replaced by identity-carrying anti-join of unmatched prefix rows (rename + LIMIT carries fixed); no-identity shapes decline typed, never anonymous rows.
ORDER BY decline messages now hint the working output-alias spelling.

21 new green pins (section F, hand-computed oracles, both engines) + 2 decline-shape negative controls. Gates: OM 81P, divergences 11P/3xf, hop 42P, flatten 25P, lowering 1420P (0 new failures), polars 587P, TCK 4145P/0F, ruff+guards green (surface baseline regen for intentional lowering growth 9454→9503).

🤖 Generated with Claude Code

https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

Four silent-wrong shapes from the round-004-precursor adversarial re-probe: 1 (CRITICAL): terminal WITH after OPTIONAL MATCH rode the single-alias row-column pipeline -- pandas nulled matched bindings and collapsed multiplicity, polars fabricated bid=seed-ids. flatten_terminal_with_over_ optional now rewrites pure bare-alias carries onto the connected left-join lowering, with the stage WHERE applied as a post-join binding-ROW filter (openCypher WITH..WHERE semantics; serves seed AND optional-alias predicates -- the canonical `WHERE x IS NULL` anti-join now answers). Terminal projection/aggregate stages passed through by RETURN fold into a direct RETURN (2: zero-count groups now kept -- count(b)=0 / count(*)=1 for unmatched seeds). Whole-row-next-to-aggregate stages keep their typed decline; any other unflattenable OM WITH stage now declines typed instead of riding the pipeline into silent-wrong. 3+4 (one root): the reentry null-fill replaced `prefix_rows - result_rows` anonymous null templates with an identity anti-join over projected carried-alias columns (bare props, renamed WITH scalars incl. their __cypher_reentry_ markers) -- rename (`WITH a AS p`) and LIMIT carries now null-extend with their seed identity; a multiplicity surplus no longer masks missing null rows. Shapes that project no identifying carried column decline typed (prefix_rows==1 count logic stays, it is sound). 5: the ORDER BY optional-property decline now hints the output-alias spelling that sorts with openCypher null placement. Pins: section F in test_optional_match_semantics.py -- pandas+polars hand-computed oracles for every finding, plus decline-shape negative controls; the anti-join message-audit pin flips to a served-answer pin. Surface-guard baseline regenerated: lowering.py 9454 -> 9503 (flatten hook + typed guard + post-join filter). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-14T10:40:29Z

GPU sweep receipt (dgx GB10, RAPIDS 26.02, --gpus-enforced, @ d502dea): full graphistry/tests/compute/gfql tree — 8820 passed, 53 skipped, 22 xfailed, 0 failed (4m40s). First zero-failure GPU sweep of the release campaign; prior tip was 1 failed / 8773 passed. Sanity probes (cudf sum + string to_pandas, polars GPUEngine raise_on_fail collect) green. Minor follow-up noted, not a failure: pandas FutureWarning on all-NA concat at reentry/execution.py:180 in the null-fill path.

Same coverage-floor playbook as flatten_pure_carry_optional: the new #1896 flatten shipped with only end-to-end coverage; 20 direct AST pins cover the admit paths and every decline branch (flatten.py floor 93.60% -> above 94.05%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-15T15:59:05Z

+    if fill_rows is None and carried_output_map:
+        # #1896: never synthesize ANONYMOUS null rows by count arithmetic --
+        # anti-join unmatched prefix rows so the null-extension keeps its
+        # carried seed identity, or decline typed below.


Violates comment rules too

lmeyerov · 2026-08-15T15:59:35Z

+    *,
+    prefix_df: DataFrameT,
+    result_df: Optional[DataFrameT],
+    null_row: Dict[str, Any],  # hygiene-ok: explicit-any -- heterogeneous Cypher fill values, mirrors _optional_reentry_carried_null_rows


Avoid Any when narrow available

lmeyerov · 2026-08-15T16:00:11Z

+
+    Disqualified shapes return None and stay on the (typed-decline) pipeline:
+    renames/DISTINCT/ORDER/SKIP/LIMIT on the stage, multiple stages, UNWIND/
+    CALL, references to non-carried aliases after a subset carry.


More comment issues

…ping (#1897 review) Owner inline review flagged three sites (reentry/execution.py:145 comment, reentry/execution.py:189 `Any` + hygiene-ok pin, reentry/flatten.py:311 docstring). The review skill's self-review gate was then run over the whole PR diff and every hit given a verdict. Names and structure instead of prose: reentry/flatten.py flatten_terminal_with_over_optional keeps a two-line docstring; its 20-line narration is now the helper names _is_terminal_with_over_optional_match, _stage_reshapes_rows, _match_clause_aliases, _query_without_pure_carry_stage, _query_with_terminal_stage_folded_into_return, _stage_has_aggregates. _pure_carry_aliases_ignoring_where was a copy of _pure_carry_aliases minus one guard; both now share _bare_carry_aliases + _stage_reshapes_rows. reentry/execution.py the carried_output_map tri-state (None declines / {} nothing to do / map fills) is a frozen CarriedOutputSources plus CARRIED_OUTPUTS_NOT_REPRODUCIBLE; the "never synthesize anonymous null rows" comment is the helper name _optional_reentry_unmatched_identity_null_rows. gfql_unified.py _carried_output_sources / _carried_output_source_column / _output_reads_carried_alias replace the inline branch cascade and its trailing comments. cypher/lowering.py both added comment blocks and the post_join_row_filter docstring paragraph deleted; the parameter name states the rule. Typing (aliases byte-identical to the #1894 review so the branches merge cleanly): cypher/ast.py CypherScalar = Union[None, bool, int, float, str]; CypherLiteral re-expressed over it agg_types.py CypherEmptyGroupValue / CypherEmptyGroupFills execution.py CypherFillValue / CypherFillRow Both `# hygiene-ok: explicit-any` pins this PR had introduced are removed, not suppressed. Ratchets locked down: explicit-any reentry/execution.py 9 -> 0, gfql_unified.py 21 -> 20 (4496 -> 4486 total); cypher surface lowering_py_max_lines 9503 -> 9493. Pins added where a deleted comment was the only record of a contract: test_flatten_terminal_with.py::test_admitted_query_never_retains_a_with_stage_so_recompiling_it_terminates test_optional_reentry_carried_outputs.py (all seven source-column branches, including the reentry-marker branch that had only a trailing comment) test_optional_match_semantics.py::test_optional_match_anti_join_with_where_x_is_null_keeps_only_unmatched_rows Parametrized decline cases carry pytest ids instead of `#` labels. No behavior change: graphistry/tests/compute/gfql failure set byte-identical to the PR base (93 failed, md5 d68f3d7cddf5bab5d11588e04727d362), test_compute_chain/test_compute_hops identical (2 pre-existing cudf-lane failures). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-15T16:41:07Z

Review addressed, with the self-review gate run over the whole PR diff rather than just the flagged lines.

The three sites

execution.py:145 — the 3-line # #1896: never synthesize ANONYMOUS null rows… was narration and why-a-fix-was-made, so a BLOCKER twice over. Deleted. The rule now lives in the helper name _optional_reentry_unmatched_identity_null_rows and in the type: the Optional[Mapping[str,str]] tri-state (None = decline / {} = nothing to do / map = fill) became a frozen CarriedOutputSources plus CARRIED_OUTPUTS_NOT_REPRODUCIBLE, which also retired a 4-line parameter docstring.
execution.py:189 — Dict[str, Any] with two hygiene-ok pins on brand-new code. Both pins removed, not suppressed; real aliases added (CypherScalar, CypherEmptyGroupValue, CypherFillValue, CypherFillRow), and five sibling helpers in the file narrowed with them. fix(gfql)!: OPTIONAL MATCH null-extension semantics (#1891) #1894's commit is not on this branch, so its aliases were copied byte-identically so the two merge cleanly.
flatten.py:311 — a 20-line docstring narrating the algorithm and re-listing guards the code already states. Now 2 lines plus six named helpers (_is_terminal_with_over_optional_match, _stage_reshapes_rows, _query_without_pure_carry_stage, _query_with_terminal_stage_folded_into_return, …). Also DRY'd a copy-paste: _pure_carry_aliases_ignoring_where was _pure_carry_aliases minus one guard; both now share _bare_carry_aliases.

Self-review gate, whole diff

18 sites given verdicts — including 12 # labels inside parametrize converted to pytest.param(id=…) and 3 per-test explanations folded into test names (e.g. test_declines_pure_carry_when_order_by_references_an_uncarried_alias). Afterwards grep '^\+\s*#' returns only the deliberate keeps, and grep '^\+.*(Any|type: ignore|hygiene-ok|cast\()' returns nothing.

Kept, with reasons: #: Sphinx doc-comments on the six type aliases (each states an openCypher spec fact no signature can express — the form accepted on #1894); the TYPE_CHECKING-only import-cycle note in agg_types.py, kept byte-identical to #1894's so the branches merge without conflict; and the test-file section banner matching the existing A–E convention.

Pins added where a comment was the only record

Deleting a comment without an equivalent pin is information loss, so three went in:

test_admitted_query_never_retains_a_with_stage_so_recompiling_it_terminates — 4 admitted shapes, asserts with_stages == () and that re-flattening declines (the recursion-termination claim the deleted comment made).
New test_optional_reentry_carried_outputs.py — 7 named params covering every branch of the carried-output mapping, including p.__cypher_reentry_av__ and the uncarried-marker fallthrough, which had no pin at all before.
test_optional_match_anti_join_with_where_x_is_null_keeps_only_unmatched_rows, with the hand-computed oracle in its docstring.

Gates

Failure list md5 d68f3d7cddf5bab5d11588e04727d362 — identical across base, PR head, and post-fix. Targeted suites 1538 passed; cypher dir 7 failed (all cudf on a GPU-less box, inside the identical 93); chain+hops byte-identical to base. ruff clean; mypy 328 files clean.

Hygiene improved and ratcheted down, not held flat: explicit-any in reentry/execution.py 9 → 0 (baseline entry deleted) and gfql_unified.py 21 → 20; total grandfathered 4496 → 4486. The cypher-surface baseline also ratcheted 9503 → 9493 as lowering.py shrank.

One judgement call flagged for you: that surface ratchet tightens within the bump this PR itself made (9454 → 9503 → 9493) rather than leaving 10 lines of slack.

lmeyerov · 2026-08-15T17:00:22Z

-        fill_rows = [dict(null_row) for _ in range(missing_count)]
-    elif not fill_rows:
+        if not carried_outputs.every_output_reproducible:
+            raise reentry_validation_error(


tck-gfql obligations likely, audit & handle as needed

lmeyerov · 2026-08-15T17:01:56Z

@@ -0,0 +1,150 @@
+"""Direct tests for terminal-WITH-over-OPTIONAL flattening (#1896).


shouldn't these be more like cypher/reentry/test_flatten.py instead of toplevel?

lmeyerov · 2026-08-15T17:02:25Z

+    return replace(query, with_stages=()), stage.where
+
+
+def _query_with_terminal_stage_folded_into_return(


seems significant positive/negative test obligations

lmeyerov · 2026-08-15T17:02:47Z


-CypherLiteral = Union[None, bool, int, float, str, ParameterRef]
+#: Every value openCypher can write literally: the four primitive types plus null.
+CypherScalar = Union[None, bool, int, float, str]


is this from lower on our pr stack?

lmeyerov · 2026-08-15T17:03:09Z

        )
        if compiled_connected_optional is not None:
            return _attach_graph_context(compiled_connected_optional)
+    if query.with_stages and any(m.optional for m in query.matches):


more significant pos/neg test amplifications obligations

lmeyerov · 2026-08-15T17:03:57Z


+    # TYPE_CHECKING only: `gfql.cypher.__init__` eagerly imports the whole compiler, and
+    # this module is imported from inside that import (row.pipeline), so a runtime import
+    # would be circular. The aliases below only need the name at type-check time.


avoid unnecessary and overly verbote comments... root cause why this happened, and both fix here, but all the root cause of why authoring & reviewing let this through

lmeyerov · 2026-08-15T17:04:51Z


-def _optional_reentry_aggregate_fill_values(compiled_query: CompiledCypherQuery) -> Dict[str, Any]:
+_IDENTIFIER_TOKEN = re.compile(r"[A-Za-z_][A-Za-z0-9_]*")
+_REENTRY_MARKER_COLUMN = re.compile(r"__cypher_reentry_(\w+)__")


strange location for these symbols, shouldn't they be externed for reuse by others?

lmeyerov · 2026-08-15T17:05:16Z

+    return None
+
+
+def _carried_output_sources(compiled_query: CompiledCypherQuery) -> CarriedOutputSources:


gfql_unified.py is already overly large..

lmeyerov · 2026-08-15T17:05:43Z

+    ops = list(compiled_query.chain.chain) if compiled_query.chain is not None else []
+    for op in ops:
+        function = getattr(op, "function", None)
+        op_params = getattr(op, "params", None) or {}


try to replace dynamic typing pattersn like getattr with more structured & static programming

lmeyerov · 2026-08-15T17:06:29Z

+    return None
+
+
+def _carried_output_sources(compiled_query: CompiledCypherQuery) -> CarriedOutputSources:


test amplification concerns?

lmeyerov · 2026-08-15T17:07:45Z

Answering the factual question first, then the rest is dispatched.

"is this from lower on our pr stack?" (ast.py:30) — Yes, and deliberately byte-identical. CypherScalar and the CypherLiteral re-expression are the same lines #1894 introduced (verified: git show on both branches returns identical text). #1894's commit is not an ancestor of this branch, so rather than invent a parallel alias the lane copied it verbatim so the two merge without conflict. Same for the TYPE_CHECKING note in agg_types.py. That is also why you are seeing the same agg_types.py:55 comment on both #1894 and here — one duplication, two PRs. It is being deleted in both, not just here.

"gfql_unified.py is already overly large" — confirmed and now tracked. It is 2,526 lines; lowering.py is 9,493 (#1921). Both are the same problem, and I have added gfql_unified.py to that refactor issue rather than opening a competing one. Note the immediate consequence for this PR: your point about the fill-value symbols being in a "strange location" and the file being oversized are the same observation — those symbols belong in an importable home (gfql/agg_types.py already exists for exactly this), not inline here.

Dispatched for fixing, grouped:

Encoding: agg_types.py:55 verbose comment (with the root-cause you asked for — see docs(review): encode the rejection categories the GFQL stack kept hitting #1924, which now carries seven causes and the corrected keep bar: "a justified keep is still a keep; the test is whether a name or a test can carry it").
Structure: extern the fill-value symbols for reuse; test file moved to mirror the source tree (cypher/reentry/test_flatten.py rather than top-level) — you are right that a test's location should follow the module it pins.
Typing: the remaining getattr probes replaced with static dispatch on the AST types.
Testing obligations (three separate comments — flatten.py:367, lowering.py:9155, gfql_unified.py:1508): a positive/negative matrix across both engines for the flatten admit/decline branches and the terminal-WITH folding, with hand-computed oracles and an anti-vacuity count at this PR's base.
tck-gfql obligations (execution.py:171): audited — this PR's branch has a companion tck-gfql branch carrying the promotion cascade, and I will report explicitly whether any scenario's outcome changes and whether the contract/manifest needs regenerating, rather than assuming it does not.

Also worth flagging from your earlier note, since it applies here: the surface-guard ratchet on this PR tightened within its own bump (9454 → 9503 → 9493), leaving no slack. Say the word if you would rather it left headroom.

lmeyerov · 2026-08-15T17:20:21Z

+    """Prefix-frame column behind each result output that reads the carried alias."""
+
+    columns: Mapping[str, str]
+    every_output_reproducible: bool


are any of these exernally needed types?

lmeyerov · 2026-08-15T17:20:56Z

+_IDENT_TOKEN = re.compile(r"[A-Za-z_][A-Za-z0-9_]*")
+_AGGREGATE_CALL = re.compile(
+    r"\b(count|sum|avg|min|max|collect|stdev|percentile\w*)\s*\(", re.IGNORECASE
+)


should these be externed and/or swapped to predicates?

lmeyerov · 2026-08-15T17:21:56Z

+#: What an aggregate answers for an EMPTY group, per the Cypher contract documented above:
+#: ``count``/``sum`` -> ``0``, ``count(*)`` over a synthesized null-extended row -> ``1``,
+#: ``collect`` -> ``[]``. Every other aggregate's empty-group answer is ``null`` and is
+#: therefore absent from a fill map rather than present as a value.


overly verbose..

Cascading base update after #1895 landed its review remediation. Resolutions: - graphistry/compute/gfql/agg_types.py: BOTH sides independently introduced an empty-group-aggregate contract (diff3 merge base empty). They are the SAME concept, so reconciled rather than unioned blindly: kept the base's shorter `#:` line for CypherEmptyGroupValue / CypherEmptyGroupFills and the base's CYPHER_ZERO_EMPTY_GROUP_AGGREGATIONS / CYPHER_EMPTY_LIST_EMPTY_GROUP_AGGREGATIONS frozensets, which the (cleanly merged) body of _optional_reentry_aggregate_fill_values already reads. Dropped this branch's 3-line TYPE_CHECKING circular-import narration; the import position already states it. - graphistry/compute/gfql_unified.py: import block took the base (superset: the two frozensets plus CypherParams, both used in the merged file). The _optional_reentry_aggregate_fill_values hunk is NOT a rival implementation -- the base only renamed its return type Dict[str, Any] -> Dict[str, CypherEmptyGroupValue], which this branch did too. Kept the base's signature line and this branch's genuinely-new carried-output helpers above it (_IDENTIFIER_TOKEN, _REENTRY_MARKER_COLUMN, _output_reads_carried_alias, _carried_output_source_column, _carried_output_sources), which are a different feature (#1896 carried-output reproducibility) that only shares the file. - graphistry/compute/gfql/cypher/reentry/execution.py: unioned the imports (this branch's CypherEmptyGroupValue/CypherScalar for CypherFillValue, the base's REENTRY_HIDDEN_COLUMN_PREFIX used at lines 572/576). Kept this branch's carried_outputs parameter on apply_optional_reentry_null_fill. Kept the base's deletion of the empty_result_row prose comment while keeping this branch's `null_row: CypherFillRow` annotation. - bin/ci_type_hygiene_baseline.json: took the base's numbers, then --update-baseline; reentry/execution.py now has zero findings and its entry is removed rather than left at 8. Gates: no conflict markers, ruff clean, type-hygiene guard clean, cypher surface guard pass, mypy shows only the 4 known polars-skew errors. graphistry/tests/compute/gfql = 8595 passed / 93 failed, and the same 93 fail on ghhttps/fix/gfql-1888-endpoint-closure alone (local cudf + polars version skew), so the merge adds no failures. Touched tests (test_flatten_terminal_with, test_optional_reentry_carried_outputs, test_optional_match_semantics, plus agg/flatten neighbours): 431 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

Cascading base update after #1897 took its review remediation. Resolutions: - graphistry/compute/gfql/cypher/reentry/flatten.py: the base refactored flatten_terminal_with_over_optional into named helpers and DELETED _pure_carry_aliases_ignoring_where (verified: absent from ghhttps/fix/gfql-1896-om-with-pipeline:flatten.py, its only caller replaced by _is_terminal_with_over_optional_match / _stage_reshapes_rows / _bare_carry_aliases). Took the base's deletion plus its new _stage_has_aggregates, which the cleanly-merged _query_with_terminal_stage_folded_into_return already calls. Kept this branch's genuinely-new flatten_pure_carry_terminal_with_nonoptional (#1899), which reads _pure_carry_aliases, not the deleted helper. `git grep _pure_carry_aliases_ignoring_where` is now empty. - bin/test-polars.sh: kept BOTH lane entries -- test_row_multiplicity_semantics.py (this branch) and test_hop_boundary_matrix.py (base) are different files, so picking a side would silently drop a polars lane. - bin/ci_cypher_surface_guard_baseline.json: took the base's 9493, then --write-baseline. lowering.py is 9635 lines after the merge, which is the union of both sides' additions and BELOW this branch's own previous cap of 9674; the ratchet is tightened, not loosened. Gates: no conflict markers, ruff clean, type-hygiene guard clean, cypher surface guard pass, mypy shows only the 4 known polars-skew errors. test_row_multiplicity_semantics + test_fast_path_engagement + tests/compute/gfql/cypher + test_optional_match_semantics = 3671 passed, 8 failed -- all 8 are [cudf] parametrizations that fail identically on the base branch alone (no GPU in this environment). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

#1897 went CONFLICTING when #1895's remediation landed, so GitHub could build no merge ref and stopped creating workflow runs entirely. Both conflicts were "each side appended a different new section", merge base empty on both: - test_optional_match_semantics.py: #1895 added section E (optional-reentry aggregate fill values, #1891, compile-level so the end-to-end sweep cannot discharge it vacuously via a decline); #1897 added section F (#1896 OM -> WITH pipeline row semantics). Different features, so BOTH kept. - ci_cypher_surface_guard_baseline.json: `lowering_py_max_lines` was 9493 ours / 9425 theirs. Neither is right for the merged tree -- taking either side would have left the ratchet loose or falsely tight. Set to the MEASURED count of the merged file, 9464. 90 passed in test_optional_match_semantics.py; cypher surface guard passes; ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

…ith-pipeline # Conflicts: # graphistry/compute/gfql/lazy/engine/polars/chain.py

… into fix/gfql-1896-om-with-pipeline

… mirror reentry tests Externs the optional-reentry carried-output symbols out of the oversized gfql_unified.py into graphistry/compute/gfql/cypher/reentry/carried_outputs.py, replaces the getattr AST probes with isinstance dispatch on ASTCall, and shares one identifier predicate across flatten/carry/carried_outputs via graphistry.compute.gfql.identifiers. Tests pinning cypher/reentry/* now mirror the source tree under graphistry/tests/compute/gfql/cypher/reentry/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

#1901 went CONFLICTING when #1897's merge landed, so GitHub could build no merge ref and created no workflow runs at all. One conflict, in ci_cypher_surface_guard_baseline.json: `lowering_py_max_lines` was 9635 ours / 9464 theirs / 9493 at the merge base. Neither side's number describes the merged tree, so it is set to the MEASURED count of the merged lowering.py, 9635. The cypher-surface guard passes at that value. gfql_fast_paths.py auto-merged. ruff clean; cypher suite 3517 passed with 7 failures, all `[cudf]` lanes already present in the recorded baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

lmeyerov · 2026-08-15T21:55:48Z

Two things I promised in the dispatch comment above were not actually delivered at fcd46ace5, and one risk had never been checked. Both are closed now, and I re-audited all 16 threads against the current head rather than assuming the earlier write-up.

1. The test relocation was one level short

You asked for cypher/reentry/test_flatten.py rather than top-level. The files had only reached graphistry/tests/compute/gfql/cypher/ — there was no reentry/ directory at all. git mv'd, so history follows:

old	new	tests
`tests/compute/gfql/cypher/test_flatten_carried_endpoint_rebind.py`	`tests/compute/gfql/cypher/reentry/test_flatten_carried_endpoint_rebind.py`	23
`tests/compute/gfql/cypher/test_flatten_pure_carry_optional.py`	`tests/compute/gfql/cypher/reentry/test_flatten_pure_carry_optional.py`	26
`tests/compute/gfql/cypher/test_flatten_terminal_with.py`	`tests/compute/gfql/cypher/reentry/test_flatten_terminal_with.py`	24
`tests/compute/gfql/cypher/test_optional_reentry_carried_outputs.py`	`tests/compute/gfql/cypher/reentry/test_carried_outputs.py`	7

80 before, 80 after (23/26/24/7 each side, checked per file, not just the total).

The fourth one is the interesting case, and it is why I checked what each file pins instead of moving on filename: test_optional_reentry_carried_outputs.py did not pin a cypher/reentry/* module at all — it imported graphistry.compute.gfql_unified._carried_output_source_column. Moving it under reentry/ would have been the wrong location for the code as it stood. Your #1508/#1515 comments are the same observation from the other end, so I moved the code instead (below) and then the test followed it honestly, renamed to mirror its module.

No __init__.py — checked first: the sibling cypher/ and row/ test dirs have none (index/, layouts/, same_path/ do), so the new dir matches its immediate parent.

References swept (grep -rn over CI configs, bin/*.sh, docker/*.sh, pyproject.toml, DEVELOP.md, the guard baselines and test_polars_lane_completeness.py): none of the four files was registered anywhere. Specifically —

test_polars_lane_completeness.py keys on modules that mention polars; all four files contain zero occurrences of the string, so none was in the lane list or the NOT_POLARS_GATED dict.
.github/workflows/ci.yml path filters use the prefix regex ^graphistry/tests/compute/gfql/cypher/, which cypher/reentry/... still matches — no CI edit needed.
The polars lane still carries the #1896 execution tests via test_optional_match_semantics.py, which did not move.

2. The comment-density guard: it did fail, and the fix was not a cap

You were right that this was live, not hypothetical. Merging master (526976e91) — which brings #1927, #1924, #1926, #1894 — the guard failed on two files:

comment-block:    graphistry/compute/hop.py                8 findings, baseline 6
issue-rationale:  .../cypher/reentry/flatten.py            2 findings, baseline 1

hop.py was inherited, not this PR's: the base branch had already fixed it in a7c9d6fd5 ("merge: master into fix(gfql)!: endpoint-closure contract on every surface (#1888, absorbs #1808) #1895, and clear the comment guard it now trips"), which fcd46ace5 predates. Merging the current base head cleared it — no duplicate fix, no divergence with fix(gfql)!: endpoint-closure contract on every surface (#1888, absorbs #1808) #1895.
flatten.py was ours: flatten_terminal_with_over_optional's docstring cited (#1896) as its rationale. Deleted the citation; the rationale is carried by the pin names in reentry/test_flatten_terminal_with.py. The grandfathered 1 is the pre-existing #1341 module docstring.

No baseline cap was raised. The only baseline edit is a tightening: gfql_unified.py comment-block 32 → 21 and issue-rationale 15 → 11, locked because the externing below made the improvement real. I deliberately did not run a blanket --update-baseline: 26 other files are now below their caps, but those improvements belong to master/#1895 and locking them here would just manufacture a conflict on a shared file.

3. Merge resolution

One conflict, in lazy/engine/polars/chain.py. diff3 showed it was the trap you described: our side had trimmed a 5-line comment to 3, and theirs looked like it had rewritten the same comment — but the real content of their side was a new edge_src, edge_dst = _bound_edge_endpoints(self) line that the code below now depends on. Kept both: master's extraction plus our trimmed comment. _bound_edge_endpoints is master's new helper (chain.py:855) and is used at the return g0.nodes(...).edges(...) below.

bin/ci_cypher_surface_guard_baseline.json: lowering_py_max_lines resolved to 9464, which is the measured line count of the merged lowering.py (master was 9425 and untouched there; the delta is entirely this PR's). The ratchet you flagged still has zero slack by construction — I left it that way rather than quietly loosening it. Say the word and I'll add headroom.

4. Per-thread disposition (all 16)

#	Thread	Disposition	Evidence
1	`execution.py` "Violates comment rules too"	Addressed in `fcd46ace5`	`grep -n "never synthesize ANONYMOUS" reentry/execution.py` → no match. Rule now in the helper name `_optional_reentry_unmatched_identity_null_rows` (execution.py:197). Guard: file at 4 vs cap 5.
2	`execution.py` "Avoid Any when narrow available"	Addressed in `fcd46ace5`	`reentry/execution.py` is absent from `ci_type_hygiene_baseline.json`'s `explicit-any` map — absent means it must be zero. Aliases `CypherFillValue`/`CypherFillRow` at execution.py:45/48. Self-review gate over the whole product diff returns no added `Any`/`hygiene-ok`/`cast(`.
3	`flatten.py` "More comment issues"	Addressed in `fcd46ace5`, +1 residual fixed here	20-line docstring is now 2 lines + named helpers (`_is_terminal_with_over_optional_match`, `_stage_reshapes_rows`, `_query_without_pure_carry_stage`, `_query_with_terminal_stage_folded_into_return`). The residual `(#1896)` citation the new guard caught is deleted.
4	`execution.py:177` "tck-gfql obligations likely, audit & handle"	Addressed — and here is the explicit answer I owed you	Companion branch exists and CI will run it: `tck-gfql@fix/gfql-1896-om-with-pipeline` = `48ced5951`; `ci.yml:672-703` resolves the tck ref by `head_ref` match. Yes, scenario outcomes change and yes the manifest needed regenerating — `09008dd contract: 4 OPTIONAL MATCH scenarios now succeed with correct rows`, `674f3b6 contract: regen capability-debt manifest for the 4 promoted keys`, `48ced59 fully promote match-where6-2 + match7-29/30/31`.
5	test file location	Was open — fixed here	Section 1.
6	`flatten.py:354` "significant pos/neg test obligations"	Addressed in `fcd46ace5`	`reentry/test_flatten_terminal_with.py`, 24 tests, both admit and decline branches at AST level.
7	`ast.py:30` "is this from lower on our pr stack?"	Addressed — now reconciled, not just answered	`git diff ghhttps/master HEAD -- graphistry/compute/gfql/cypher/ast.py` is empty after the master merge (#1894 landed in `526976e91`). The byte-identical copy merged away exactly as intended. Same for `agg_types.py`.
8	`lowering.py:9152` "pos/neg test amplification obligations"	Addressed in `fcd46ace5`, verified anti-vacuous here	The `test_1896_` family in `test_optional_match_semantics.py`: 22 test instances over pandas+polars, including 3 decline pins. Measured anti-vacuity: replayed against the base `a7c9d6fd5`, 20 of 22 fail*, 2 pass (positive controls).
9	`agg_types.py` verbose comment + root cause	Addressed in `fcd46ace5`	`git diff ghhttps/master HEAD -- graphistry/compute/gfql/agg_types.py` empty; the TYPE_CHECKING note is gone on both. Root cause is #1924, and it is now a machine gate (`bin/ci_comment_density_guard.py`, #1926) rather than a promise — this branch runs it clean.
10	`gfql_unified.py:1515` "strange location, shouldn't they be externed?"	Was PARTIAL — fixed here	The empty-group fill-value symbols did land in `gfql/agg_types.py`. The carried-output resolution did not: two regexes and three functions were still inline. Now `graphistry/compute/gfql/cypher/reentry/carried_outputs.py` (167 lines), which is also where `CarriedOutputSources` / `CARRIED_OUTPUTS_NOT_REPRODUCIBLE` now live — with their producer instead of their consumer.
11	`gfql_unified.py` "already overly large"	Was open — fixed here	2541 → 2422 lines (−119). Ratcheted and locked: `comment-block` 32 → 21, `issue-rationale` 15 → 11.
12	"replace dynamic patterns like getattr with structured & static programming"	Was STILL OPEN at `fcd46ace5` — fixed here	`getattr(op, "function", None)` / `getattr(op, "params", None)` were still at gfql_unified.py:1553-1554. Now `isinstance(op, ASTCall)` in `_projection_items_and_grouping`, matching what the sibling `optional_reentry_aggregate_fill_values` already did in the same file.
13	"test amplification concerns?" on `_carried_output_sources`	Addressed in `fcd46ace5`	`reentry/test_carried_outputs.py`, 7 named params covering every branch (incl. `p.__cypher_reentry_av__` read-through and the uncarried-marker fallthrough), plus the 22-test execution family.
14	`execution.py:56` "are any of these externally needed types?"	Now answered, and acted on	Yes for `CarriedOutputSources` + `CARRIED_OUTPUTS_NOT_REPRODUCIBLE` — `gfql_unified` imports both — so they moved to `carried_outputs.py`. No for `CypherFillValue`/`CypherFillRow`: `grep -rn` shows every use is inside `reentry/execution.py`, so they stay private there.
15	`flatten.py:45` "should these be externed and/or swapped to predicates?"	Was open — fixed here, both halves	Externed and turned into predicates: `is_bare_identifier()` / `identifier_tokens()` in `graphistry/compute/gfql/identifiers.py`. That retired four duplicates of the same regex — `flatten._BARE_IDENT`, `flatten._IDENT_TOKEN`, `gfql_unified._IDENTIFIER_TOKEN`, and two inline copies in `reentry/carry.py`. `_REENTRY_MARKER_COLUMN` became `reentry_hidden_column_output_name()` in `naming.py`, built from the same prefix/suffix constants as `_reentry_hidden_column_name()` so the writer and the parser cannot drift. `_AGGREGATE_CALL` stays local — one call site, flatten-specific.
16	`agg_types.py` "overly verbose"	Addressed in `fcd46ace5`	Same as #9.

Also added the missing CHANGELOG.md entry for #1896 — it had none.

Gates

gate	result
`ruff check graphistry bin`	clean
`ci_comment_density_guard.py`	OK, no growth (was FAILING before the merge)
`ci_type_hygiene_guard.py`	OK, no growth
`ci_cypher_surface_guard.py`	pass; `lowering.py` 9464 = baseline 9464
`mypy graphistry`	4 errors, 329 files — byte-identical to the base's 4 errors / 328 files, zero new
`pytest graphistry/tests/compute`	102 failed / 10243 passed vs base 102 failed / 10190 passed — failure lists md5-identical (`6a75a38ebb30614a35083426514a76ef`), `diff` empty. The 102 are pre-existing cuDF-lane failures on this box, unchanged. +53 passed = the 53 tests this PR adds (22 `#1896` execution + 24 terminal-WITH + 7 carried-output).
self-review gate on the product diff	only `#:` Sphinx alias doc-comments (the form accepted on #1894); no added `Any` / `type: ignore` / `hygiene-ok` / `cast(`

Failure lists captured whole (… 2>&1 \| grep '^FAILED' \| sort > file, never through tail) and compared as sets against a baseline generated from the merged base a7c9d6fd5 in a separate worktree.

Pushed as e08e0dbad (fcd46ace5 -> merge master 526976e91 -> merge base a7c9d6fd5 -> the three commits above). mergeable: MERGEABLE, base unchanged, CI running. Nothing was resolved, force-pushed, rebased, or re-based.

`test-gfql-core (3.12)` was the only red lane on #1897, and no test failed in it -- 4756 passed. The coverage audit rejected the run: graphistry/compute/gfql/cypher/reentry/carried_outputs.py: actual=94.17% floor=0.00% reason=resolved target file is missing from the baseline That file is new: externing the carried-output resolution out of gfql_unified.py (review threads #10/#11) created it, and a newly added source file has to be registered or the audit fails closed. Fixing the omission, not the gate. Floors follow each lane's measured reality rather than one invented number: pandas gets the 94.17% CI measured, and both rapids lanes get 0.0, matching every sibling in `cypher/reentry/` (carry, compiletime, flatten, lowering_support, rewrite all sit at 0.0 there -- the GPU lane does not exercise them). No floor is asserted that was not measured on that lane. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

This was referenced Aug 14, 2026

CI lane-coverage audit: ~1,600 test ids with zero CI execution + neutral-skip gating (round-004 umbrella) #1898

Open

fix(gfql): row-pipeline multiplicity + aggregate identities (#1899) #1901

Open

lmeyerov commented Aug 15, 2026

View reviewed changes

lmeyerov mentioned this pull request Aug 15, 2026

docs(review): encode the rejection categories the GFQL stack kept hitting #1924

Merged

lmeyerov commented Aug 15, 2026

View reviewed changes

lmeyerov mentioned this pull request Aug 15, 2026

Refactor lowering.py: 9,260 lines / 217 top-level defs in one module #1921

Open

lmeyerov commented Aug 15, 2026

View reviewed changes

lmeyerov mentioned this pull request Aug 15, 2026

fix(gfql)!: OPTIONAL MATCH null-extension semantics (#1891) #1894

Merged

lmeyerov and others added 5 commits August 15, 2026 14:24

Merge remote-tracking branch 'ghhttps/master' into fix/gfql-1896-om-w…

69217d2

…ith-pipeline # Conflicts: # graphistry/compute/gfql/lazy/engine/polars/chain.py

Merge remote-tracking branch 'ghhttps/fix/gfql-1888-endpoint-closure'…

325d53f

… into fix/gfql-1896-om-with-pipeline

ci(gfql): lock the gfql_unified comment-density improvement

72892fc

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

docs: changelog entry for the #1896 OPTIONAL MATCH + WITH fix

e08e0db

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi

		@@ -0,0 +1,150 @@
		"""Direct tests for terminal-WITH-over-OPTIONAL flattening (#1896).

		return replace(query, with_stages=()), stage.where


		def _query_with_terminal_stage_folded_into_return(

		return None


		def _carried_output_sources(compiled_query: CompiledCypherQuery) -> CarriedOutputSources:

Uh oh!

Conversation

lmeyerov commented Aug 14, 2026

Uh oh!

lmeyerov commented Aug 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmeyerov commented Aug 15, 2026

The three sites

Self-review gate, whole diff

Pins added where a comment was the only record

Gates

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmeyerov commented Aug 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmeyerov commented Aug 15, 2026

1. The test relocation was one level short

2. The comment-density guard: it did fail, and the fix was not a cap

3. Merge resolution

4. Per-thread disposition (all 16)

Gates

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant