#4096 fix(cypher): undirected multi-hop patterns must not reuse the same edge

[robfrank]

Summary

• The optimizer's ExpandAll physical operator did not enforce Cypher's path isomorphism constraint, so (a)-[r1:KNOWS]-(b)-[r2:KNOWS]-(c) could bind the same edge to both r1 and r2, producing rows like Alice, Bob, Alice, sameRel=true.
• Fix: skip a candidate edge whose RID already appears under another Edge-typed property in the row, using a new isEdgeAlreadyUsed() helper. The same check already lived in MatchRelationshipStep.isEdgeAlreadyUsed() for the non-optimizer path; the optimizer path bypassed it entirely.
• Added 3 regression tests in Issue4096UndirectedRelReuseIsomorphismTest.

Closes #4096

Test plan

• mvn test -pl engine -Dtest=Issue4096UndirectedRelReuseIsomorphismTest - 3/3 pass
• mvn test -pl engine -Dtest="*Cypher*,*OpenCypher*,*opencypher*" - 5896 tests, 0 failures
• Confirmed pre-fix the test failed with same edge reused as r1=r2

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
Complexity	1 medium

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 62.79% diff coverage · -7.79% coverage variation

Metric	Results
Coverage variation	✅ -7.79% coverage variation
Diff coverage	✅ 62.79% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (34d8898)	123485	90693	73.44%
Head commit (cd6314f)	154905 (+31420)	101701 (+11008)	65.65% (-7.79%)

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 (#4114)	86	54	62.79%

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 implements Cypher path isomorphism in the ExpandAll operator, ensuring that relationship variables within a MATCH pattern are bound to distinct edges. It includes a new regression test suite for issue #4096. A review comment suggests optimizing the edge reuse check by pre-calculating a set of used edge RIDs per input row to avoid redundant property iterations in the expansion loop.

[gemini-code-assist]

This method is called for every edge of a source vertex, causing the properties of the result row to be iterated repeatedly. This could lead to performance issues when expanding from vertices with a high degree.

To optimize this, you could compute the set of used edge RIDs just once per input row, before iterating over the edges. This would involve:

1. Adding a private Set<RID> usedEdgeRids field to the anonymous ResultSet implementation in execute().
2. When a new currentInputResult is fetched in fetchMore(), populate this set by iterating over its properties once.
3. In the edge expansion loop, replace the call to isEdgeAlreadyUsed with a more efficient usedEdgeRids.contains(edge.getIdentity()).

This refactoring would make the isEdgeAlreadyUsed method redundant and improve performance by avoiding redundant computations in a hot loop.

[robfrank]

Implemented in f27207a. The check now precomputes a RidHashSet of used edge RIDs once per input row in ExpandAll and queries it per edge with O(1) contains(). The set is also scoped to same-MATCH-clause preceding rel vars only, so it is null/empty in the common single-hop case and the lookup is free.

[claude]

Code Review

Overview

This PR fixes a correctness bug in the Cypher optimizer path: the ExpandAll physical operator was not enforcing Cypher's path isomorphism constraint, allowing the same edge to bind to two different named relationship variables in an undirected multi-hop pattern. The fix mirrors the check already present in MatchRelationshipStep.isEdgeAlreadyUsed() for the legacy path.

The core fix is correct and the approach is sound. A few things are worth calling out.

---

Issues

1. Missing previousStepVariables guard (correctness risk)

File: ExpandAll.java:213-222

The MatchRelationshipStep.isEdgeAlreadyUsed() counterpart skips variables that originate from previous MATCH clauses (tracked via previousStepVariables), because Cypher's relationship uniqueness constraint only applies within a single MATCH clause:

// MatchRelationshipStep - existing code
if (previousStepVariables != null && previousStepVariables.contains(prop))
    continue;

The new ExpandAll.isEdgeAlreadyUsed() omits this guard entirely. This means a query like:

MATCH (a)-[r1:KNOWS]-(b)
WITH a, b, r1
MATCH (b)-[r2:KNOWS]-(c)

...would incorrectly suppress rows where r1 and r2 happen to be the same edge, because when the second MATCH's ExpandAll runs, it sees r1 in the row and flags it as already used. Cross-clause edge reuse is valid Cypher and must not be blocked.

This should be addressed either by passing a previousStepVariables set into ExpandAll from the planner, or by documenting that the optimizer currently only emits ExpandAll for single-clause patterns (if that is actually guaranteed today).

---

2. ExpandInto is not covered

File: ExpandInto.java

ExpandInto also has an edgeVariable field and is used by the optimizer when both endpoints are already bound. If two hops in a pattern both resolve to ExpandInto (e.g., after index seeks bind all three nodes), the isomorphism check is also absent there. The scenario is narrower than ExpandAll but the same root cause applies.

---

3. Unnamed relationships are still unguarded

The fix is gated on edgeVariable != null. Cypher's relationship uniqueness constraint also applies to unnamed relationships: (a)--(b)--(c) must not traverse the same edge twice. For the optimizer path, unnamed relationships with ExpandAll remain unguarded. This is a pre-existing gap (the legacy MatchRelationshipStep always calls isEdgeAlreadyUsed unconditionally), but worth noting since this PR is touching exactly this area.

---

Observations

Test: undirectedMultiHopTotalCountMatchesExplicitFilter is redundant

File: Issue4096UndirectedRelReuseIsomorphismTest.java:101-119

This third test asserts that the unfiltered count equals the count with an explicit WHERE r1 <> r2. But undirectedMultiHopDistinctRelationshipCount already checks the count is 2, and undirectedMultiHopDoesNotReuseRelationship already confirms sameRel = false for both rows. The third test adds no independent signal. Consider replacing it with a test for a cross-clause pattern (the previousStepVariables scenario above) to both document the expected behavior and guard against the gap in item 1.

No coverage for the cross-clause case

There is no test verifying that an edge bound in a previous MATCH clause is still eligible for traversal in a subsequent MATCH clause using the optimizer path. Adding one would both document intent and catch a regression if the previousStepVariables guard is later added.

---

Positives

• The fix correctly identifies the optimizer code path that bypassed the isomorphism check.
• Passing a RID rather than an Edge to isEdgeAlreadyUsed() is a small but nice improvement over the MatchRelationshipStep signature.
• Existing Cypher test suite (5896 tests) passes with the change.
• The regression test class is well-structured and uses the project's standard assertion style (assertThat(...).isEqualTo(...)).
• @BeforeEach/@AfterEach correctly drop the test database to avoid cross-test contamination.

[robfrank]

Follow-up in f27207a addresses the review.

Issue 1 (cross-clause scoping): Confirmed real - ran a probe query (MATCH (a)-[r1]-(b) MATCH (b)-[r2]-(c) RETURN count(*)) which returned 2 instead of the expected 6 with the original fix, because the optimizer merges all MATCH clauses into a single LogicalPlan. Fixed by:

1. Adding a clauseIndex field to LogicalRelationship (set in LogicalPlan.extractPatterns).
2. CypherOptimizer.buildExpansionChain now tracks rel vars per clause index in a Map<Integer, Set<String>> and passes the same-clause preceding set to each operator via setSameClausePrecedingRelVars.
3. ExpandAll/ExpandInto only check edges bound to variables in that scoped set.

Issue 2 (ExpandInto): Same scoping mechanism added. ExpandInto already resolves the connecting edge when an edgeVariable is set; when it isn't but the row has a same-clause edge bound, the check loads it via findEdgeForCheck (cheap getFirstEdgeConnectedToVertex).

Issue 3 (anonymous relationships): Out of scope for this PR. The legacy MatchRelationshipStep already synthesises internal anonymous variables via computeHopEdgeTrackingNeeds for multi-hop anonymous patterns with overlapping types; the optimizer has no equivalent. Worth a separate issue.

Observations: Replaced the redundant undirectedMultiHopTotalCountMatchesExplicitFilter test with crossClauseEdgeReuseIsAllowed (covers issue 1) and added expandIntoPathDoesNotReuseRelationship (covers issue 2).

Gemini's perf suggestion: Implemented. ExpandAll builds a RidHashSet of used edge RIDs once per input row instead of scanning result properties on every edge.

Full Cypher suite (5896 tests) still green.

[claude]

Code Review - PR #4114: Fix Cypher Undirected Multi-Hop Relationship Isomorphism

Overview

This PR correctly identifies and fixes a real Cypher compliance bug: the optimizer's ExpandAll and ExpandInto physical operators did not enforce relationship uniqueness within a MATCH clause, allowing the same edge to be bound to two different relationship variables. The fix is targeted and the clause-scoping logic (same-clause vs. cross-clause) reflects the correct Cypher semantics.

---

Correctness Issues

1. collectUsedEdgeRids / isSameClauseEdgeReuse miss TraversalPath and List<Edge> bindings

The reference implementation in MatchRelationshipStep.isEdgeAlreadyUsed() (lines 920-948) handles three kinds of edge bindings in a row:

if (val instanceof Edge && ...)                return true;
if (val instanceof TraversalPath)   { ... }   // variable-length pattern edges
if (val instanceof List)            { ... }   // VLP relationship variable edge lists

The new ExpandAll.collectUsedEdgeRids and ExpandInto.isSameClauseEdgeReuse check only instanceof Edge. A variable-length hop earlier in the same MATCH clause (e.g. (a)-[r1*1..2]-(b)-[r2]-(c)) can put a TraversalPath or a List<Edge> into the row, and those edges will not be extracted. This means isomorphism enforcement silently falls through for mixed fixed/variable-length patterns in the same clause.

2. ExpandInto.findEdgeForCheck returns at most one edge, causing false positives on multi-edge graphs

GraphEngine.getFirstEdgeConnectedToVertex returns only the first matching edge. When there are multiple edges of the same type between two vertices, findEdgeForCheck may return an edge that happens to conflict with a same-clause variable, even though a different valid edge exists between the same pair. The row gets dropped even though a non-conflicting traversal was possible. This is a correctness hole for multigraph schemas.

3. Anonymous relationship patterns bypass the check

The optimizer only tracks named relationship variables:

if (rel.getVariable() != null && !rel.getVariable().isEmpty()) {
    relVarsPerClause.computeIfAbsent(...).add(rel.getVariable());
}

Cypher's relationship uniqueness constraint applies to all relationships in a pattern, named or not. For (a)-->(b)-->(c) the two traversed edges must also be distinct. Because anonymous edges are never stored under a row property, they cannot be checked post-hoc. This matches the same limitation as MatchRelationshipStep, so it is not a regression, but a comment noting the known gap would help future contributors.

---

Minor Issues

4. Inconsistent isEmpty() guard in ExpandInto

isSameClauseEdgeReuse guards with isEmpty() but hasUsedRels does not:

// hasUsedRels - missing isEmpty() check
if (sameClausePrecedingRelVars == null) return false;

// isSameClauseEdgeReuse - has the guard
if (sameClausePrecedingRelVars == null || sameClausePrecedingRelVars.isEmpty()) return false;

Iterating an empty set is harmless, but the inconsistency is noticeable.

5. ExpandInto performs two edge lookups when edgeVariable is null

When no edge binding is required (edgeVariable == null), the existing code already found connectivity. The new isomorphism check then calls findEdgeForCheck which does another full getFirstEdgeConnectedToVertex traversal. For the common single-hop case the sameClausePrecedingRelVars set is empty so hasUsedRels returns false quickly - but for longer chains this adds an extra edge scan per row. This is acceptable given the correctness requirement, just worth noting in a comment.

6. Default clauseIndex = 0 in legacy constructors

The 8- and 9-argument LogicalRelationship constructors (kept for backwards compatibility) default clauseIndex to 0. These constructors are used directly in ExpandIntoRuleTest. If any non-test caller creates relationships across multiple MATCH clauses using these constructors, the clause index will be wrong and isomorphism will be incorrectly scoped to clause 0. This is low risk today but could silently misfire if new callers are added.

---

Test Coverage

The four regression tests in Issue4096UndirectedRelReuseIsomorphismTest cover the stated scenarios well. A few gaps worth adding in follow-up:

• A directed multi-hop pattern ((a)-[r1]->(b)-[r2]->(c)) - verify the fix does not regress directed paths
• A mixed fixed+variable-length pattern in the same clause ((a)-[r1*1..2]-(b)-[r2]-(c)) to demonstrate the known gap (issue 1 above) or confirm it is handled
• A multigraph case (two parallel edges between the same vertex pair) to document expected behavior under issue 2

The cross-clause count assertion (crossClauseEdgeReuseIsAllowed expecting 6) is correct and the comment explaining the breakdown is appreciated.

---

Code Quality

• The RidHashSet usage and the null-short-circuit pattern (return null from collectUsedEdgeRids vs. an empty set) are good performance choices consistent with the project's GC-pressure guidelines.
• The clause-scoping design (relVarsPerClause keyed by clauseIndex) is clean and the rationale is well-commented.
• The Javadoc on setSameClausePrecedingRelVars explains the Cypher semantics correctly.
• LogicalRelationship.getClauseIndex() Javadoc is accurate.

---

Summary

The core fix is correct and the approach is sound. Before merging I would suggest:

1. (Required) Extend collectUsedEdgeRids and isSameClauseEdgeReuse to handle TraversalPath and List<Edge> bindings, mirroring MatchRelationshipStep.isEdgeAlreadyUsed.
2. (Recommended) Add a comment on findEdgeForCheck noting the first-edge limitation and its implication for multigraph schemas.
3. (Optional) Align hasUsedRels to also guard with isEmpty() for consistency.

[claude]

Code Review - #4096 fix(cypher): undirected multi-hop patterns must not reuse the same edge

Overview

This PR enforces Cypher path isomorphism for the optimizer's physical operators (ExpandAll / ExpandInto), which previously bypassed the same check that already existed in MatchRelationshipStep.isEdgeAlreadyUsed(). The per-clause scoping approach (via clauseIndex on LogicalRelationship and relVarsPerClause in buildExpansionChain) is the correct way to implement Cypher's rule that relationship uniqueness applies only within a single MATCH clause.

---

Bug: GAVExpandAll bypasses the isomorphism check

In createExpandAllOperator, when a GraphTraversalProvider is registered for the edge type, the optimizer returns a GAVExpandAll early - before setSameClausePrecedingRelVars is ever called:

if (provider != null) {
    return new GAVExpandAll(input, provider, sourceVariable, targetVariable, direction, edgeTypes,
        gavCost, outputCardinality);   // <-- returned here, no isomorphism wiring
}

final ExpandAll expandAll = new ExpandAll(...);
expandAll.setSameClausePrecedingRelVars(sameClausePrecedingRelVars);  // never reached for GAV
return expandAll;

GAVExpandAll has no setSameClausePrecedingRelVars method and no corresponding check in its expansion loop, so queries that trigger the GAV fast path will still reuse the same edge across hops. A regression test should cover this path, or GAVExpandAll should gain the same guard.

---

Potential Issue: ExpandInto.findEdgeForCheck with parallel edges

findEdgeForCheck calls getFirstEdgeConnectedToVertex, which returns the first edge between two vertices. In a graph with multiple parallel edges of the same type between the same pair of vertices, the method might return an edge that is different from the one actually selected for the traversal. This could produce a false positive (incorrectly skipping a valid path where the first edge happens to match a bound variable, but another parallel edge would not) or a false negative (the traversal picks the first edge which does match a bound variable, but findEdgeForCheck is never called because connectedEdge was already set).

This is a pre-existing limitation of ExpandInto's existence-check semantics, but the new code amplifies it.

---

Design concern: setter instead of constructor parameter

sameClausePrecedingRelVars is a non-final field set via setSameClausePrecedingRelVars() in both ExpandAll and ExpandInto. The GAV bug above exists precisely because the object can be returned before the setter is called. Passing it through the constructor (or at minimum making the field final after a mandatory setter call) would make the dependency explicit. Given that ExpandAll already has many constructor parameters, a builder or an optional config wrapper might be cleaner long-term.

---

Minor: anonymous relationship variables are not covered

buildExpansionChain only tracks named relationship variables in relVarsPerClause:

if (rel.getVariable() != null && !rel.getVariable().isEmpty()) {
    relVarsPerClause.computeIfAbsent(rel.getClauseIndex(), k -> new HashSet<>())
        .add(rel.getVariable());
}

Cypher's path isomorphism rule applies to all relationships, named or not. A pattern like MATCH (a)--(b)--(c) should not reuse the same edge. Because anonymous edges are never stored in the result row in the optimizer path (the if (edgeVariable != null && !edgeVariable.isEmpty()) guard), the check has no information to work with for anonymous hops. This is a pre-existing limitation rather than a regression from this PR, but worth tracking.

---

Code quality notes

• The whitespace alignment changes throughout CypherOptimizer.java (field declarations, constructor parameter lists, Javadoc @param tags) add substantial diff noise. They are not harmful, but make it harder to see the semantically meaningful changes.
• ExpandAll uses RidHashSet for O(1) lookups while ExpandInto uses a linear scan over sameClausePrecedingRelVars with direct equals comparisons. Both are correct; the inconsistency is minor for typical query sizes (2-4 hops).
• The hasUsedRels gate in ExpandInto before calling findEdgeForCheck is a good performance guard.

---

Test coverage

The three regression tests are well-chosen:

Test	Purpose
undirectedMultiHopDoesNotReuseRelationship	Named vars, verifies sameRel = false
undirectedMultiHopDistinctRelationshipCount	Count-based sanity check
crossClauseEdgeReuseIsAllowed	Cross-MATCH allowance (critical for scoping correctness)
expandIntoPathDoesNotReuseRelationship	ExpandInto path

Missing: a test that uses a GAV-backed edge type to confirm the check also works there (currently it does not, per the bug above).

A note on assertion style: the CLAUDE.md convention prefers the assertThat(x).isEqualTo(y) form throughout - the tests use it correctly.

---

Summary

The core logic - per-clause variable tracking in the optimizer, delegating the check into ExpandAll/ExpandInto at the row level - is correct and matches the semantics of the existing MatchRelationshipStep path. The primary concern before merging is the GAVExpandAll gap, which leaves the original bug active for queries that go through that fast path.

[codecov]

Codecov Report

❌ Patch coverage is 54.65116% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.45%. Comparing base (34d8898) to head (cd6314f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...uery/opencypher/executor/operators/ExpandInto.java	10.25%	30 Missing and 5 partials ⚠️
...opencypher/optimizer/plan/LogicalRelationship.java	71.42%	2 Missing ⚠️
...query/opencypher/executor/operators/ExpandAll.java	94.11%	0 Missing and 1 partial ⚠️
...db/query/opencypher/optimizer/CypherOptimizer.java	94.73%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4114      +/-   ##
==========================================
- Coverage   64.46%   64.45%   -0.01%     
==========================================
  Files        1623     1623              
  Lines      123485   123556      +71     
  Branches    26288    26303      +15     
==========================================
+ Hits        79610    79644      +34     
- Misses      32823    32856      +33     
- Partials    11052    11056       +4     

☔ 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.