-
-
Notifications
You must be signed in to change notification settings - Fork 114
#4006 fix(cypher): VLP segments must not re-traverse a relationship bound in a prior MATCH #4008
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1664,7 +1664,7 @@ private AbstractExecutionStep buildMatchStep(final MatchClause matchClause, Abst | |||||
| AbstractExecutionStep nextStep; | ||||||
| if (relPattern.isVariableLength()) { | ||||||
| nextStep = new ExpandPathStep(effectiveSourceVar, pathVariable, relVar, effectiveTargetVar, relPattern, | ||||||
| true, effectiveTargetNode, pathPattern.getEffectivePathMode(), new HashSet<>(boundVariables), context); | ||||||
| true, effectiveTargetNode, pathPattern.getEffectivePathMode(), computePrevVarsForVlp(pathPattern, i, boundVariables), context); | ||||||
| } else { | ||||||
| // Check if this hop requires IN traversal on a unidirectional edge. | ||||||
| // Unidirectional edges don't store incoming links, so we must restructure: | ||||||
|
|
@@ -2028,7 +2028,7 @@ public String prettyPrint(final int depth, final int indent) { | |||||
| // Variable-length path - pass path variable, relationship variable, and target node for label | ||||||
| // filtering. Snapshot previously bound variables for relationship-uniqueness scoping. | ||||||
| nextStep = new ExpandPathStep(currentSourceVar, pathVariable, relVar, targetVar, relPattern, true, | ||||||
| targetNode, pathPattern.getEffectivePathMode(), new HashSet<>(legacyBoundVariables), context); | ||||||
| targetNode, pathPattern.getEffectivePathMode(), computePrevVarsForVlp(pathPattern, i, legacyBoundVariables), context); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the call to
Suggested change
|
||||||
| } else { | ||||||
| // Fixed-length relationship - pass path variable, target node pattern, and bound variables | ||||||
| nextStep = new MatchRelationshipStep(currentSourceVar, relVar, targetVar, relPattern, pathVariable, | ||||||
|
|
@@ -3511,6 +3511,29 @@ private boolean[] computeHopEdgeTrackingNeeds(final PathPattern pathPattern) { | |||||
| return needs; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Computes the set of "previously bound" variables to pass to {@link ExpandPathStep} for a | ||||||
| * variable-length hop at {@code vlpHopIndex} within {@code pathPattern}. | ||||||
| * <p> | ||||||
| * OpenCypher path isomorphism applies within a single <em>path</em>, not within a MATCH clause. | ||||||
| * A relationship variable bound by a prior MATCH that is also explicitly named in the current | ||||||
| * path pattern is a same-path co-participant and must be checked for edge conflicts even though | ||||||
| * it was introduced before this MATCH. We therefore remove those co-participants from the | ||||||
| * exclusion set before handing it to ExpandPathStep. | ||||||
| */ | ||||||
| private static Set<String> computePrevVarsForVlp(final PathPattern pathPattern, final int vlpHopIndex, | ||||||
| final Set<String> boundVariables) { | ||||||
| final Set<String> prevVars = new HashSet<>(boundVariables); | ||||||
| for (int j = 0; j < pathPattern.getRelationshipCount(); j++) { | ||||||
| if (j == vlpHopIndex) | ||||||
| continue; | ||||||
| final RelationshipPattern rel = pathPattern.getRelationship(j); | ||||||
| if (rel.getVariable() != null && !rel.getVariable().isEmpty()) | ||||||
| prevVars.remove(rel.getVariable()); | ||||||
| } | ||||||
| return prevVars; | ||||||
| } | ||||||
|
Comment on lines
+3515
to
+3535
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation of Consider updating this method to iterate over all path patterns in the current /**
* Computes the set of "previously bound" variables to pass to {@link ExpandPathStep} for a
* variable-length hop at {@code vlpHopIndex} within {@code currentPathPattern} of the
* given {@code matchClause}.
* <p>
* OpenCypher relationship isomorphism applies within a single pattern (MATCH clause).
* A relationship variable bound by a prior MATCH that is also explicitly named in the current
* MATCH clause is a co-participant and must be checked for edge conflicts even though
* it was introduced before this MATCH. We therefore remove those co-participants from the
* exclusion set before handing it to ExpandPathStep.
*/
private static Set<String> computePrevVarsForVlp(final MatchClause matchClause, final PathPattern currentPathPattern, final int vlpHopIndex,
final Set<String> boundVariables) {
final Set<String> prevVars = new HashSet<>(boundVariables);
for (final PathPattern pathPattern : matchClause.getPathPatterns()) {
for (int j = 0; j < pathPattern.getRelationshipCount(); j++) {
if (pathPattern == currentPathPattern && j == vlpHopIndex)
continue;
final RelationshipPattern rel = pathPattern.getRelationship(j);
final String var = rel.getVariable();
if (var != null && !var.isEmpty())
prevVars.remove(var);
}
}
return prevVars;
}
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The suggestion extends the fix beyond what the OpenCypher spec requires. Path isomorphism applies within a single path pattern, not across comma-separated patterns within a MATCH clause. The issue description itself states this explicitly:
TCK Match4 [7] (the canonical test for this fix) uses a single path pattern: MATCH p = (n)-[*0..1]-()-[r]-()-[*0..1]-(m)The rule is: if Broadening to the full |
||||||
|
|
||||||
| /** | ||||||
| * Checks if two hops with the same edge type are guaranteed to match different physical edges | ||||||
| * based on the vertex labels at their edge endpoints. | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| /* | ||
| * Copyright © 2021-present Arcade Data Ltd (info@arcadedata.com) | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| * SPDX-FileCopyrightText: 2021-present Arcade Data Ltd (info@arcadedata.com) | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| package com.arcadedb.query.opencypher; | ||
|
|
||
| import com.arcadedb.database.Database; | ||
| import com.arcadedb.database.DatabaseFactory; | ||
| import com.arcadedb.query.sql.executor.Result; | ||
| import com.arcadedb.query.sql.executor.ResultSet; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * Regression test for GitHub issue #4006. | ||
| * <p> | ||
| * When a relationship variable {@code r} is bound in one MATCH clause and then referenced | ||
| * explicitly inside a path pattern containing variable-length segments in a subsequent MATCH, | ||
| * the variable-length segments must not re-traverse {@code r}. OpenCypher path isomorphism | ||
| * applies within a single path, not within a single MATCH clause. | ||
| * <p> | ||
| * This corresponds to TCK scenario {@code Match4 [7]}: | ||
| * "Matching variable length patterns including a bound relationship". | ||
| */ | ||
| class Issue4006BoundRelVarPathIsomorphismTest { | ||
| private Database database; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| database = new DatabaseFactory("./target/databases/testopencypher-4006").create(); | ||
| database.getSchema().createVertexType("Node4006"); | ||
| database.getSchema().createEdgeType("EDGE4006"); | ||
| database.transaction(() -> database.command("opencypher", | ||
| "CREATE (n0:Node4006)-[:EDGE4006]->(n1:Node4006)-[:EDGE4006]->(n2:Node4006)-[:EDGE4006]->(n3:Node4006)")); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void tearDown() { | ||
| if (database != null) { | ||
| database.drop(); | ||
| database = null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * TCK Match4 [7]: variable-length segments flanking a bound relationship must obey path | ||
| * isomorphism - they must not re-traverse the explicitly named {@code r}. | ||
| */ | ||
| @Test | ||
| void vlpSegmentsDoNotReuseExplicitlyNamedBoundRelationship() { | ||
| final ResultSet result = database.query("opencypher", | ||
| "MATCH ()-[r:EDGE4006]-()" | ||
| + " MATCH p = (n)-[*0..1]-()-[r]-()-[*0..1]-(m)" | ||
| + " RETURN count(p) AS c"); | ||
|
|
||
| final List<Result> rows = collect(result); | ||
| assertThat(rows).hasSize(1); | ||
| assertThat(((Number) rows.get(0).getProperty("c")).longValue()).isEqualTo(32L); | ||
| } | ||
|
|
||
| /** | ||
| * Regression guard for issue #3999: a previously bound relationship that is NOT part | ||
| * of the current path pattern must not block the variable-length traversal. | ||
| */ | ||
| @Test | ||
| void unboundedVlpIsNotBlockedByUnrelatedPreviouslyBoundRel() { | ||
| final ResultSet result = database.query("opencypher", | ||
| "MATCH (a:Node4006)-[r:EDGE4006]->(b:Node4006)" | ||
| + " WITH a, b, r" | ||
| + " MATCH path = (a)-[:EDGE4006*1..2]->(b)" | ||
| + " RETURN count(r) AS rc"); | ||
|
|
||
| final List<Result> rows = collect(result); | ||
| assertThat(rows).hasSize(1); | ||
| assertThat(((Number) rows.get(0).getProperty("rc")).longValue()).isGreaterThan(0L); | ||
| } | ||
|
|
||
| private static List<Result> collect(final ResultSet rs) { | ||
| final List<Result> list = new ArrayList<>(); | ||
| while (rs.hasNext()) | ||
| list.add(rs.next()); | ||
| return list; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the call to
computePrevVarsForVlpto pass thematchClauseif the suggested change to the method signature is adopted.