#4095 fix(cypher): anonymous directed rel patterns must not reuse the same edge

[robfrank]

Summary

• Fixes Consecutive directed relationship patterns may reuse the same relationship #4095 (companion to Consecutive undirected relationship patterns may reuse the same relationship #4096): consecutive anonymous relationship patterns in the same MATCH clause could reuse the same physical edge, producing spurious rows like (Alice, Bob, Alice) for (s)<-[:KNOWS]-(f)-[:KNOWS]->(d).
• The Consecutive undirected relationship patterns may reuse the same relationship #4096 fix tracked named relationship variables in relVarsPerClause. Anonymous rels (rel.getVariable() == null) were never added, so sameClausePrecedingRelVars stayed empty and the per-row isomorphism check in ExpandAll.collectUsedEdgeRids had nothing to compare against. Same root pattern as the legacy MatchRelationshipStep handles via computeHopEdgeTrackingNeeds + synthetic " relN" names.
• ExpandAll now has an edgeTrackingVar field that stashes the edge in the row under a synthetic property name even when edgeVariable is null. CypherOptimizer.computeNeedsEdgeTracking flags anonymous hops whose edge types overlap with a sibling hop in the same MATCH clause, and buildExpansionChain assigns each one a synthetic " anon_e_N" name, sets it on the operator before addTargetLabelFilter wraps it, and registers it in relVarsPerClause for subsequent hops.

Test plan

• New Issue4095DirectedRelReuseIsomorphismTest (4 tests): diverging + converging row count + aggregate count
• Issue4096UndirectedRelReuseIsomorphismTest still passes (named-variable path unchanged)
• Issue4006BoundRelVarPathIsomorphismTest still passes
• All 6572 Cypher tests pass with no regressions

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 17 complexity

Metric	Results
Complexity	17

View in Codacy

🟢 Coverage 90.00% diff coverage · -7.76% coverage variation

Metric	Results
Coverage variation	✅ -7.76% coverage variation
Diff coverage	✅ 90.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (c1814a7)	123556	90737	73.44%
Head commit (fb92b57)	154945 (+31389)	101770 (+11033)	65.68% (-7.76%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4115)	40	36	90.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[gemini-code-assist]

Code Review

This pull request addresses GitHub issue #4095 by enforcing Cypher relationship uniqueness for anonymous relationship variables within the same MATCH clause. It introduces a mechanism in CypherOptimizer to identify relationships requiring tracking and implements synthetic variable generation to store these edges in the ExpandAll operator. A comprehensive regression test suite is also added. Feedback was provided regarding the efficiency of the relationship type overlap detection logic in CypherOptimizer, suggesting a Map-based approach to replace the current nested loops for better performance and readability.

[gemini-code-assist]

The logic to find relationships with overlapping types can be implemented more efficiently and readably. The current implementation uses nested loops with a complexity that is roughly quadratic in the number of relationships per clause, which could be slow for complex queries with many relationships.

A more efficient and declarative approach would be to use a map to group relationships by their types. This avoids the nested loops and manual index management, improving both performance and readability.

      final Map<String, List<LogicalRelationship>> relationshipsByType = new HashMap<>();
      for (final LogicalRelationship rel : clauseRels) {
        for (final String type : rel.getTypes()) {
          relationshipsByType.computeIfAbsent(type, k -> new ArrayList<>()).add(rel);
        }
      }

      for (final List<LogicalRelationship> rels : relationshipsByType.values()) {
        if (rels.size() > 1) {
          needs.addAll(rels);
        }
      }

[robfrank]

Applied. Replaced the nested per-clause loop with a Map<String, List<LogicalRelationship>> type-bucket pass: each rel is added to the bucket of every type it declares, then any bucket with size > 1 contributes its rels to the tracking set. Linear in (rels x types) and clearly intent-revealing. The hasUntyped short-circuit stays — an empty getTypes() would otherwise never enter any bucket, but semantically an untyped hop overlaps with everything.

[claude]

Code Review: #4095 fix - Anonymous directed rel patterns isomorphism

Overall: Solid, well-scoped fix with good test coverage. A few edge cases worth discussing.

---

What this PR does

Fixes Cypher relationship isomorphism enforcement for anonymous (unnamed) relationship variables in ExpandAll. Previously, (s)<-[:KNOWS]-(f)-[:KNOWS]->(d) could return rows where both hops bound the same physical edge (e.g. (Alice, Bob, Alice)) because anonymous rels were never registered in relVarsPerClause and the per-row uniqueness set was always empty.

The fix follows the same pattern as the existing named-variable fix (#4096): assign synthetic " anon_e_N" names to anonymous hops that need tracking, store the edge under that name in the result row, and register it in relVarsPerClause so subsequent hops compare against it in collectUsedEdgeRids.

---

Correctness

computeNeedsEdgeTracking logic is sound. Grouping by clauseIndex, short-circuiting on untyped, and the outer: label for early exit are all correct. The conservative "all rels in clause when any is untyped" approach avoids false negatives.

One potential gap - ExpandInto path not handled:

In buildExpansionChain, the new tracking block is inside the else branch (ExpandAll-only). If an anonymous rel is promoted to ExpandInto (both endpoints already bound), the edge is not stored in the row for downstream hops to check. ExpandInto has isSameClauseEdgeReuse but only guards against its own reuse, not tracking the edge for subsequent hops. This is a pre-existing limitation (not introduced here), but a follow-up issue is warranted - a 3+ hop pattern like (a)-[:T]->(b)<-[:T]-(c)-[:T]->(d) could hit this if b and c are already bound.

---

Minor code observations

Synthetic vars in RETURN *: The " anon_e_N" tracking vars are stored as row properties and never cleaned up. If a query uses RETURN *, those synthetic properties (two leading spaces) could appear in the output. Worth confirming the projection step strips them or documenting the behaviour.

computeNeedsEdgeTracking adds named rels to needs when any is untyped. Named rels in needs are harmless (the rel.getVariable() == null guard prevents acting on them), but filtering to anonymous-only would make the intent clearer without changing behaviour.

O(N^2) type intersection in computeNeedsEdgeTracking: Fine at plan-build time for typical patterns. A brief comment noting this is planning-time cost only would help future readers.

---

Test coverage

Coverage is good for the primary cases (diverging/converging with row count and aggregate assertions).

Missing scenarios worth considering before merge:

1. 3+ hop same-type chain: (a)-[:T]->(b)-[:T]->(c)-[:T]->(d) - all three hops anonymous, same type. Exercises syntheticEdgeVarCounter > 1 and verifies all three edges are distinct.

2. Mixed named + anonymous in same clause: MATCH (a)-[r:T]->(b)-[:T]->(c) - confirms the two tracking paths (existing named-var + new synthetic) do not interfere.

3. Cross-clause reuse still permitted: MATCH (a)-[:T]->(b) MATCH (c)-[:T]->(d) should still allow the same physical edge in both clauses. Valid Cypher cross-clause reuse must not be blocked.

---

Summary

The fix is correct for the primary case, uses the established synthetic-variable convention consistently, and the tests clearly document the bug and expected behaviour. The ExpandInto gap for anonymous rels is pre-existing and should get a tracked follow-up issue. Adding the 3-hop and mixed-variable test cases would meaningfully harden coverage before merge.

[robfrank]

Addressed in fb92b57:

Test additions (all 3 suggested scenarios):

• threeHopAnonymousSameTypeChainHasDistinctEdges - 4-vertex linear chain with 3 anonymous KNOWS hops; verifies multiple synthetic counters yield distinct edges
• mixedNamedAndAnonymousSameClauseBindsDistinctEdges - one named (r) and one anonymous KNOWS slot in the same clause; both tracking paths coexist correctly
• crossClauseAnonymousEdgeReuseIsAllowed - two separate MATCH clauses; same physical edge legitimately fills both, count returns 4 (not blocked)

computeNeedsEdgeTracking complexity: Refactored per Gemini's suggestion to a Map<String, List<LogicalRelationship>> type-bucket pass. Now O(rels × types) instead of O(rels²).

Named rels in needs (cosmetic): With the bucket refactor this is moot — buckets now produce the actual overlapping pairs, and the optimizer's (rel.getVariable() == null || rel.getVariable().isEmpty()) guard at the call site already filters down to anonymous-only. Named rels in the set never act on it.

ExpandInto 3+ hop gap: Acknowledged as pre-existing and out of this PR's scope. The 3-hop test only hits ExpandAll because no endpoint is bound from a separate clause. Worth a follow-up issue, agreed.

RETURN * synthetic-var bleed: Confirmed pre-existing — the legacy MatchRelationshipStep path uses the same " relN" two-leading-spaces convention with the same propagation behavior. FinalProjectionStep doesn't strip them. Worth a separate cleanup (filter prop.startsWith(" ") in the returnAll block) but unrelated to this fix's correctness.

[claude]

Code Review: #4095 fix - Anonymous directed rel patterns isomorphism

Overall: Well-implemented fix, correct approach, test coverage is stronger than the PR description suggests. A few follow-up items worth tracking.

---

What this PR does

Fixes Cypher relationship isomorphism enforcement for anonymous (unnamed) relationship variables in ExpandAll. Previously, a pattern like (s)<-[:KNOWS]-(f)-[:KNOWS]->(d) could return rows where both hops bound the same physical edge (e.g. (Alice, Bob, Alice)) because anonymous rels were never registered in relVarsPerClause, leaving sameClausePrecedingRelVars empty for every hop and making the collectUsedEdgeRids isomorphism check a no-op.

The fix follows the same approach as the named-variable fix (#4096): compute which hops need tracking at plan-build time (computeNeedsEdgeTracking), assign synthetic " anon_e_N" names to qualifying anonymous hops, store the edge under that name in the result row, and register the synthetic name in relVarsPerClause so subsequent hops include it in collectUsedEdgeRids.

---

Correctness

The core fix is correct. Specific points:

• computeNeedsEdgeTracking logic is sound. Grouping by clauseIndex, short-circuiting when any hop is untyped (conservative but correct - untyped matches any edge type so all hops in the clause may collide), and falling back to per-type overlap detection are all appropriate.

• Ordering in buildExpansionChain is correct. The synthetic var block runs before addTargetLabelFilter wraps currentOp, which is required since addTargetLabelFilter may wrap the ExpandAll in another operator. The comment // Must run before addTargetLabelFilter wraps currentOp. makes this clear.

• Synthetic vars don't leak to query results. The " anon_e_N" prefix (two leading spaces) is not a valid Cypher identifier and cannot appear in any RETURN clause. FinalProjectionStep filters result rows to only explicitly returned variables, so synthetic tracking properties are automatically dropped before results reach the caller. The RETURN * concern raised in earlier reviews is not an issue - RETURN * expands to explicitly declared query variables, not to all row properties.

---

One gap: ExpandInto path not covered

In buildExpansionChain, the new tracking block is inside the else branch (ExpandAll-only):

} else {
    currentOp = createExpandAllOperator(rel, currentOp, boundVariables, sameClausePreceding);

    // new block - only runs when currentOp is ExpandAll
    if ((rel.getVariable() == null || rel.getVariable().isEmpty())
        && needsEdgeTracking.contains(rel)
        && currentOp instanceof ExpandAll expand) { ... }
}

If an anonymous hop is promoted to ExpandInto (both endpoints already bound at plan time), the edge is not stashed in the row, so subsequent hops in the same clause cannot detect the reuse. This is a pre-existing limitation not introduced by this PR, and the fix correctly handles the common unbounded-hop case. However, a 3+ hop pattern where one anonymous hop resolves to ExpandInto could still produce spurious rows. A follow-up issue to address ExpandInto tracking parity is recommended to keep the work visible.

---

Minor observations

computeNeedsEdgeTracking adds named rels to needs when untyped: When hasUntyped == true, needs.addAll(clauseRels) includes both named and anonymous rels. Named rels are harmless (the rel.getVariable() == null guard prevents acting on them in buildExpansionChain), but filtering to anonymous-only would make intent clearer:

// Current
needs.addAll(clauseRels);

// Clearer (same behaviour)
for (final LogicalRelationship r : clauseRels)
    if (r.getVariable() == null || r.getVariable().isEmpty())
        needs.add(r);

This is a very minor readability point - not a bug.

PR description says "4 tests" but the test class has 7. The description lists "diverging + converging row count + aggregate count" (4 tests), but Issue4095DirectedRelReuseIsomorphismTest actually contains:

1. divergingAnonymousDirectedDoesNotReuseRelationship
2. convergingAnonymousDirectedDoesNotReuseRelationship
3. divergingAnonymousDirectedCountIsCorrect
4. convergingAnonymousDirectedCountIsCorrect
5. threeHopAnonymousSameTypeChainHasDistinctEdges - exercises syntheticEdgeVarCounter > 1
6. mixedNamedAndAnonymousSameClauseBindsDistinctEdges - verifies named + anonymous tracking coexist
7. crossClauseAnonymousEdgeReuseIsAllowed - confirms cross-clause reuse remains valid

Tests 5-7 cover the cases that are most likely to regress from future optimizer changes. The PR description underrepresents the coverage.

---

Test coverage

Coverage is comprehensive and well-structured. The cross-clause test (crossClauseAnonymousEdgeReuseIsAllowed) is particularly valuable - it locks in the expected count (4 rows) with a comment explaining the arithmetic, which makes future regressions immediately diagnosable.

The @BeforeEach/@AfterEach lifecycle management with database.drop() is correct and consistent with the project test pattern.

---

Summary

The fix is correct, complete for the unbounded-hop case, and well-tested. The ExpandInto gap is the only remaining correctness risk, and it is pre-existing. I'd suggest filing a follow-up issue for ExpandInto parity and then this is ready to merge.

[codecov]

Codecov Report

❌ Patch coverage is 82.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.48%. Comparing base (c1814a7) to head (fb92b57).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...db/query/opencypher/optimizer/CypherOptimizer.java	80.55%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4115      +/-   ##
==========================================
+ Coverage   64.46%   64.48%   +0.02%     
==========================================
  Files        1623     1623              
  Lines      123556   123596      +40     
  Branches    26303    26316      +13     
==========================================
+ Hits        79652    79704      +52     
+ Misses      32850    32826      -24     
- Partials    11054    11066      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.