Skip to content

fix(gfql): decline undirected varlen + node alias on polars chain (honest NIE, #1741)#1742

Closed
lmeyerov wants to merge 1 commit into
masterfrom
fix/gfql-1741-undirected-varlen-alias-nie
Closed

fix(gfql): decline undirected varlen + node alias on polars chain (honest NIE, #1741)#1742
lmeyerov wants to merge 1 commit into
masterfrom
fix/gfql-1741-undirected-varlen-alias-nie

Conversation

@lmeyerov

Copy link
Copy Markdown
Contributor

Summary

Fixes the silent-wrong-results half of #1741: on engine='polars'/'polars-gpu', MATCH (a {..})-[:T*1..2]-(f) RETURN f.id returned rows pandas (the parity oracle) correctly excludes.

Tests

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL

…nest NIE)

issue #1741: undirected multi-hop lacks pandas' backtrack avoidance —
node/edge SETS agree, but the alias flag on nodes after the undirected
varlen edge tags nodes whose only path bounces back over a single edge
(the seed at *1..2), which pandas' trail semantics leaves untagged.
Cypher RETURN projects those flags, so MATCH (a)-[:T*1..2]-(f) RETURN f
silently returned wrong rows on engine='polars'. Decline the affected
shape (undirected non-single-hop + any node alias) with a NotImplemented
pointing at #1741; unnamed chains keep native parity (pinned by test).

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

Copy link
Copy Markdown
Contributor Author

Reviewer pushback accepted — my first diagnosis was wrong, and the real fix is smaller than I claimed

@lmeyerov asked why we raise instead of implementing it. Re-investigated, and the framing in this PR's description ("missing backtrack avoidance") is incorrect. Direct A/B:

  • hop(undirected, hops=2) and hop(undirected, min_hops=1, max_hops=2) from seed p0 return identical node sets on pandas and polars: [p0, p1, p2, p4]. Both engines include the seed reached by bouncing back over one edge. Neither implements trail semantics in the traversal.
  • The final chain _nodes also match on both engines.
  • The only divergence is the node-alias flag: pandas flags f on [p1, p2, p4], polars on [p0, p1, p2, p4]. RETURN f.id projects that flag → the one bogus row.

What pandas actually does (compute/chain.py ~452-503): for a named ASTNode whose previous op is an ASTEdge, it narrows the alias with a hop-distance gatelabel_mask &= hop_col.notna() & (hop_col >= min_hop) & (hop_col <= max_hop), with min/max from the edge's output_min_hops/min_hops/hops. The seed carries hop 0, fails >= 1, and loses the alias. It is not backtrack avoidance at all. _apply_node_names in the polars chain has no hop gating whatsoever — that is the entire bug.

Why it isn't a one-liner today: the gate needs per-node hop distances (label_node_hops), which the polars chain currently NIEs for undirected varlen. So the real fix is:

  1. Emit per-node hop distances for undirected bounded varlen in the polars hop — the layered BFS already knows each layer's distance, it just doesn't emit the column. This also removes an existing NIE.
  2. Port pandas' gate into _apply_node_names (~20 lines).
  3. Drop this PR's decline; its differential test flips from "raises" to "matches the pandas oracle" and becomes the parity gate.

Proposed disposition: keep this open only as stop-the-bleeding (master currently returns silently wrong rows, which is worse than an error). If (1)+(2) land promptly, close this unmerged and ship the real fix instead. Happy to go straight to the real fix if you'd rather not carry the interim decline at all — say the word.

Full diagnosis with the A/B evidence is recorded in plans/gfql-benchmark-numbers/plan.md so it survives context loss.

@lmeyerov

Copy link
Copy Markdown
Contributor Author

Superseded by the real fix: #1746 (native label_node_hops in the polars hop) + #1747 (the hop-distance alias gate, stacked on it). The repro now matches pandas exactly through the Cypher surface — {p1, p2} on both engines — instead of declining.

Leaving this open only until #1747 merges, since master currently returns silently wrong rows for this shape and a decline is strictly better than that. Once #1747 lands I'll close this unmerged and flip its test from "raises" to "matches the pandas oracle".

@lmeyerov

Copy link
Copy Markdown
Contributor Author

Superseded by #1747 — will close once that lands

This PR declines (NotImplementedError) the undirected varlen + node-alias shape. #1747 now implements the pandas hop-distance alias gate instead of declining it, so that exact shape returns the correct rows (MATCH (a)-[*1..2]-(b) no longer over-includes the backtracked-to seed) — verified 4-engine against the pandas oracle. Implementing beats raising here.

Keeping this open only as a safety net until #1747 merges; it will be closed at that point. No further work planned on it.

@lmeyerov

Copy link
Copy Markdown
Contributor Author

Closing now (not waiting for #1747 to merge): this PR touches the same two files (chain.py + test_engine_polars_chain.py) and the same undirected-varlen-alias shape as #1747, with the opposite approach — declining (NIE) what #1747 correctly implements. Two open PRs conflicting on the same lines is confusing and offers no benefit: #1747 is reviewed, cross-engine-verified (0 regressions), and CI-green. Reopen if #1747 is ever rejected. Superseded by #1747 (refs #1741).

@lmeyerov lmeyerov closed this Jul 20, 2026
lmeyerov added a commit that referenced this pull request Jul 20, 2026
…ong (#1748)

Adapts the retired #1742 decline pattern to the one shape #1747's hop-gate can't yet
cover: a node named after a forward/reverse variable-length edge with min_hops>1. Those
labels come from pandas' layered backward walk (not ported), so the alias runs ungated
and tags nodes outside the [min_hop, max_hop] window — silently wrong rows. Now an honest
NotImplementedError pointing at #1748.

The decline is precise (differential vs pandas, 15 graphs x fwd/rev x named/unnamed):
30/30 named shapes NIE, 30/30 unnamed shapes run and match pandas, 0 silent-wrong,
0 over-decline. With this the whole #1741 stack leaves ZERO silent-wrong shape on the
polars chain. xfail pins flipped to positive assert-raises tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
lmeyerov added a commit that referenced this pull request Jul 20, 2026
…ong (#1748)

Adapts the retired #1742 decline pattern to the one shape #1747's hop-gate can't yet
cover: a node named after a forward/reverse variable-length edge with min_hops>1. Those
labels come from pandas' layered backward walk (not ported), so the alias runs ungated
and tags nodes outside the [min_hop, max_hop] window — silently wrong rows. Now an honest
NotImplementedError pointing at #1748.

The decline is precise (differential vs pandas, 15 graphs x fwd/rev x named/unnamed):
30/30 named shapes NIE, 30/30 unnamed shapes run and match pandas, 0 silent-wrong,
0 over-decline. With this the whole #1741 stack leaves ZERO silent-wrong shape on the
polars chain. xfail pins flipped to positive assert-raises tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y6dQEcjdazEnzuvuwf73ZL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant