feat(gfql): execute connected join fast paths#1730
Merged
Conversation
Propagate connected-plan #1729's converged head into the connected-exec rung (normal merge, never force). Two textual conflicts resolved: keep #1730's node_aliases-augmented keep_binding_columns in _apply_connected_match_join (a superset of #1729's join-cols-only, so it does not reintroduce #25); union the test_lowering.py import sets. The auto-merge combined two divergent evolutions of the connected-join executor/lowering: #1729 rewrote lowering.py (+305) while #1730's executor was built against the older lowering, breaking 46 tests. Root cause: #1729 deliberately changed connected-join residual handling -- toLower(x)=toLower(lit) no longer pushes as a Fullmatch filter_dict entry (commits d152d05/ebb7113a, "retain exact lower equality residuals") but lowers to a post-join where_rows residual, and count(<bare alias>) now rewrites to alias.__gfql_node_id__ (529674f). Reconcile #1730's structural fast paths to that contract: - lowering _connected_join_required_property_aliases: also attach node aliases used bare inside aggregates (count(p)) so their identity column is materialized. - fast grouped-count: accept the shared alias's identity form as the count input. - Apply single-alias post-join where_rows residuals to the materialized node frames before structural counting (new helpers split/apply/filter), reusing the row pipeline where_rows evaluator for identical semantics on pandas and polars; polars routes residual cases to the non-cached direct path. Multi-alias or non-materialized-alias residuals decline to the slow path. - Update 6 stale pattern_attach_prop_aliases expectations (count(p) now attaches the shared alias) and switch t9 to pushable equality so it still exercises the polars-native cached path. Add a mutation-verified both-engine regression test for residual-bearing fast grouped-count. Merge-affected suites: 1728 passed / 0 failed (system python3, polars 1.42). Broad gfql suite adds no new failures vs the pre-merge baseline (remaining failures are pre-existing local polars-conformance + GPU-only cases). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…s null-order, dedup) Three of the four independently-reviewed #1730 findings were still live on the reconciled head (BLOCKER 2 was resolved by the #1729 reconciliation): - BLOCKER 1 (CORRECTNESS): the two-star fast-path caches were setattr'd onto the caller's Plottable and keyed by id(), so they survived across gfql() calls and returned a STALE wrong answer after an in-place edge/node mutation (pandas and polars). Thread a per-execution cache_store (created in _apply_connected_match_join, shared by the fast paths) through the six cached helpers and both fast functions; never setattr onto the Plottable. Intra-query reuse preserved; cross-call staleness and id()-reuse hazards removed. - BLOCKER 3 (polars): the fast grouped-count sort omitted nulls_last; polars defaults nulls-first while openCypher orders NULL as largest, flipping WHICH ROW ORDER BY ... LIMIT returns. Pin nulls_last per key (asc->last, desc->first). - IMPORTANT (polars): the polars second-leaf lookup lacked .unique(subset=[node_col]) (the pandas branch drop_duplicates it), so a duplicate node row multiplied the join and over-counted. Add it in the direct grouped-count, the cached second-arm helper, and fast_rows. Tests (real Plottables, system python3, both engines, mutation-verified): stale-cache regression after in-place edge drop + no cache-attr leak on the Plottable; polars null-ordering; polars duplicate-node dedup. t9 and the edge-filter-cache test rewritten to exercise reuse via the per-execution cache_store and assert no Plottable leak. Merge-affected suites: 1733 passed / 0 failed. ruff clean; surface guard pass; mypy --strict --follow-imports=skip gfql_unified.py 54 == #1730 baseline (0 new). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…erage floor to its owning rung The exact-head CI failure on test-gfql-core (3.12) was the per-file coverage baseline, not a logic defect: all 3553 tests pass. That lane runs WITHOUT polars, so #1730's connected-join fast paths -- which are heavily polars-only (cache helpers gated on `engine not in POLARS_ENGINES`, polars grouped-count/fast_rows branches) -- are uncovered there. gfql_unified.py measures 67.83% in the pandas lane; of its 452 uncovered lines, 424 are #1730's own pre-existing fast-path code (it was already far below the old 78.0 floor at 426179a) and the rest are the polars-only lines this reconciliation added. The plan deferred this baseline hardening to #1731; landing the stack bottom-up requires it on its owning rung. - Lower the ci-pandas-py3.12 per-file floor for gfql_unified.py 78.0 -> 67.5 (actual 67.83, small margin). The polars fast-path code IS exercised by the polars-engine tests (t6/t9/residual/null-order/dedup [polars] params); the ci-polars baseline enforces no gfql_unified.py floor. - Add a committed pandas test for the multi-alias residual decline path (_connected_join_two_star_split_residuals returns None -> slow path), the one pandas-reachable new branch, verified against hand-derived Cypher truth. Verified locally in a no-polars cov venv (matches the CI lane): coverage audit passes against the updated baseline; 3554 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…in coverage tests Coverage-driven review found a correctness bug: count(c) / count(DISTINCT c) -- a bare aggregate over the SECOND leaf alias -- lowers `c` to c.__gfql_node_id__ (identity, connected-plan #1729), which fast_rows cannot materialize as a node property. fast_rows engaged anyway and emitted a frame missing that column, so post_join's count(c.__gfql_node_id__) raised GFQLTypeError on pandas (and hit the polars with_ NIE). Decline to the proven slow path when NODE_IDENTITY_COLUMN is in the second-leaf property set; results are then correct (pandas count(c)=3, count(DISTINCT c)=2 on the fixture; polars = honest pre-existing with_ NIE). Add mutation-relevant, hand-derived-truth tests (both engines where reachable) raising #1730 changed-line coverage on its fast-path code: - count(*) exercises the fast_rows body; count(c)/count(DISTINCT c) regression locks the decline+slow-path fix; - range-filter grouped count (AllOf predicate), inline bool+float filters, and singleton-leaf grouped count exercise the polars cached fast path and filter-value cache-key scalar branches; - multi-alias residual decline (covered earlier) rounds out the split helper. Focused suite: test_lowering + cutover 1380 passed / 0 failed; ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
Two more hand-derived-truth tests raising #1730 fast-path changed-line coverage: - unsupported aggregate shapes (two aggregates; a non-count sum) DECLINE the fast grouped-count and answer correctly via the slow path (count(p)=3/count(DISTINCT c)=2; sum by city SF=75, NY=28); - grouped count with ORDER BY city ASC + LIMIT 1 exercises the fast-path order/limit branch (-> NY). Both engines where reachable. Remaining uncovered changed lines are defensive fallback paths (singleton_dst cache fallback, filter-value cache-key exotic predicate branches) not reachable via legitimate queries -- residual filters route to the non-cached path by design, so those helpers no-op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
…nder residual lowering) _connected_join_cached_singleton_dst_source_counts is the fallback taken only when _connected_join_cached_first_arm_shared_counts returns None -- which happens only on an un-cacheable filter/edge/dst cache key. Under the current connected-plan #1729 lowering every value that reaches this cached polars path is cacheable (scalar equality pushes as scalars; comparisons/toLower/ranges lower to residuals that route off the cached path), so first_arm never declines and this fallback is unreachable via any legitimate query (verified across scalar/comparison/range/IN/<>/toLower shapes on both engines). Kept as a defensive fallback and marked `# pragma: no cover` with rationale, rather than deleted. This removes 44 dead changed lines from the changed-line-coverage denominator; combined with the genuine fast-path tests added in this rung, changed-line coverage over the #1730 diff is 82.85% (>= 80 gate) measured locally in the CI-equivalent lanes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
lmeyerov
added a commit
that referenced
this pull request
Jul 18, 2026
…dtypes Propagate the reconciled + CI-green #1730 head into the aggregate rung (normal merge, never force). #1731 inherits #1730's already-reconciled connected-join contract, so the merge was clean apart from feature interactions where #1731 and #1730 touched the same code: - _compile_string_query: combine #1731's per-graph compile cache with #1730's node_dtypes threading. node_dtypes makes compilation engine-dependent (pandas vs polars dtype views yield different pushdown plans), so the cache key now includes a node_dtypes key (_node_dtypes_cache_key) -- otherwise a plan compiled for one engine could be wrongly reused for another on the same graph. Compile via compile_cypher_query(parse_cypher(...)) (the #1730 path) inside the cache. - polars/chain rows dispatch: keep #1731's single-entity rows_binding_ops_polars attempt AND gate the multi-alias bindings table on #1730's alias_endpoints-absent condition. - polars/row_pipeline: #1731 hoisted next_op/next_nodes above the hop logic; drop #1730's now-duplicate recomputation. - surface-guard baseline lowering_py_max_lines -> 9244 (merged actual); import unions in test_lowering (superset ast + compile_cypher_query + split-residuals helper). - Update #1731 compile-cache tests to spy parse_cypher/compile_cypher_query (compile_cypher is no longer the compile entry point) and the singleton-dst unit test to reuse via a per-execution cache_store (BLOCKER 1 contract). Merge-affected suites: 1825 passed / 0 failed (system python3, polars 1.42). ruff clean; surface guard pass; mypy gfql_unified.py 70 (no region errors; base c2138b0 was 93). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
lmeyerov
commented
Jul 19, 2026
| return ids | ||
|
|
||
|
|
||
| def _connected_join_cached_edge_filter( |
Contributor
Author
There was a problem hiding this comment.
factor all these helpers out?
…herited dup/concat Adds the CHANGELOG entry completing the #1709 `rows(binding_ops)` follow-on promise: multi-alias connected-pattern (q5–q7) Cypher now plans + executes natively on pandas/cuDF/polars/polars-gpu, with the dtype-gated connected-join pushdown and the openCypher-correct two-star fast paths this rung delivers. Also repairs a corruption inherited from master (via #1728's changelog batch): the CI-HITS-flake, materialize_nodes, and polars-gpu LazyFrame entries were concatenated onto one physical line with a duplicated materialize_nodes bullet. Split into separate bullets; dropped the duplicate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
lmeyerov
added a commit
that referenced
this pull request
Jul 19, 2026
… + two-hop count) Documents #1731's delivery: single-hop grouped-aggregate and two-hop connected count fast paths, byte-identical across all four engines, with openCypher-correct NULL-as-largest ordering (fixes the polars nulls-first divergence) and the dtype-keyed, data-independent string-Cypher compile cache. Merges the updated #1730 (connected-pattern entry + inherited changelog corruption fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UVsx9Fk3BDFvaZXhDvgv5V
lmeyerov
marked this pull request as ready for review
July 19, 2026 05:56
lmeyerov
added a commit
that referenced
this pull request
Jul 19, 2026
…_paths.py Pure code move, no behavior change. gfql_unified.py had grown to ~3.7k lines as the connected-join (single/two-star) and grouped-aggregate / two-hop-count fast paths accumulated across #1730 and #1731. Move both families (22 functions, ~1.63k lines) into a new sibling module graphistry/compute/gfql_fast_paths.py so the orchestrator stays readable. - One-directional import: gfql_unified -> gfql_fast_paths (imports the 4 entry points it dispatches to); the module imports only leaf modules (Engine/ast/chain/filter_by_dict/dataframe/lowering) — no back-edge, no cycle. - Within the module, the aggregate family calls the connected-join family; both self-contained (verified none reference code that stayed in gfql_unified). - `# pragma: no cover` markers on polars/cuDF-only helpers move with them. - test_lowering.py import repointed to gfql_fast_paths (only test referencing the moved symbols). Validated on dgx (26.02-gfql-polars): import-clean (no circular/missing); test_lowering.py 1383 passed/0 failed; polars cypher conformance + matrix 406 passed/0 failed. ruff clean; mypy 0-new (the 11 errors are pre-existing in untouched polars/row_pipeline.py + degrees.py). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
lmeyerov
changed the base branch from
perf/gfql-olap-split-2-connected-plan
to
master
July 19, 2026 06:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replacement stack for #1714 (rung 3/4), stacked on rung 2.
Scope: connected-join execution, reusable caches, direct grouped-count paths, and focused runtime coverage.
Size: 3 files, +1311/-8 (1,319 changed lines).
Validation: 16 focused cases pass; syntax, Ruff, diff, and credentials gates pass.