-
-
Notifications
You must be signed in to change notification settings - Fork 116
fix(opencypher): MATCH WHERE ID(n) = <expr> falls back to full scan when expr is dynamic #3865
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
Merged
+147
−9
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| import com.arcadedb.query.opencypher.ast.BooleanExpression; | ||
| import com.arcadedb.query.opencypher.ast.ComparisonExpression; | ||
| import com.arcadedb.query.opencypher.ast.Expression; | ||
| import com.arcadedb.query.opencypher.ast.FunctionCallExpression; | ||
| import com.arcadedb.query.opencypher.ast.LogicalExpression; | ||
| import com.arcadedb.query.opencypher.ast.NodePattern; | ||
| import com.arcadedb.query.opencypher.ast.PropertyAccessExpression; | ||
|
|
@@ -65,11 +66,12 @@ | |
| * predicates during scanning rather than in a separate FilterPropertiesStep. | ||
| */ | ||
| public class MatchNodeStep extends AbstractExecutionStep { | ||
| private final String variable; | ||
| private final NodePattern pattern; | ||
| private final String idFilter; // Optional ID filter to apply (e.g., "#1:0") | ||
| private final BooleanExpression whereFilter; // Optional inline WHERE predicate (pushdown) | ||
| private String usedIndexName; // Track which index was used (if any) | ||
| private final String variable; | ||
| private final NodePattern pattern; | ||
| private final String idFilter; // Optional ID filter to apply (e.g., "#1:0") | ||
| private final BooleanExpression whereFilter; // Optional inline WHERE predicate (pushdown) | ||
| private final ExpressionEvaluator evaluator; // Shared evaluator for WHERE/ID expression resolution | ||
| private String usedIndexName; // Track which index was used (if any) | ||
|
|
||
| /** | ||
| * Creates a match node step. | ||
|
|
@@ -111,6 +113,7 @@ public MatchNodeStep(final String variable, final NodePattern pattern, final Com | |
| this.pattern = pattern; | ||
| this.idFilter = idFilter; | ||
| this.whereFilter = whereFilter; | ||
| this.evaluator = new ExpressionEvaluator(new CypherFunctionFactory(DefaultSQLFunctionFactory.getInstance())); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -291,13 +294,19 @@ private Iterator<Identifiable> getVertexIterator() { | |
| } | ||
|
|
||
| private Iterator<Identifiable> getVertexIterator(final Result currentInputResult) { | ||
| // Check for dynamic ID filter from WHERE clause if static idFilter is not present | ||
| String effectiveIdFilter = this.idFilter; | ||
| if ((effectiveIdFilter == null || effectiveIdFilter.isEmpty()) && whereFilter != null) { | ||
| effectiveIdFilter = extractDynamicIdFilter(whereFilter, currentInputResult); | ||
| } | ||
|
|
||
| // OPTIMIZATION: If ID filter is present, look up the specific vertex by ID | ||
| // This is critical for performance when matching by ID (e.g., WHERE ID(a) = "#1:0") | ||
| // Without this optimization, MATCH (a),(b) WHERE ID(a) = x AND ID(b) = y | ||
| // would create a Cartesian product of ALL vertices before filtering | ||
| if (idFilter != null && !idFilter.isEmpty()) { | ||
| if (effectiveIdFilter != null && !effectiveIdFilter.isEmpty()) { | ||
| try { | ||
| final RID rid = new RID(context.getDatabase(), idFilter); | ||
| final RID rid = new RID(context.getDatabase(), effectiveIdFilter); | ||
| final Identifiable vertex = context.getDatabase().lookupByRID(rid, true); | ||
| // Return single-element iterator for the matched vertex | ||
| return List.of(vertex).iterator(); | ||
|
Comment on lines
+309
to
312
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 final RID rid = new RID(context.getDatabase(), effectiveIdFilter);
final Identifiable vertex = context.getDatabase().lookupByRID(rid, true);
if (vertex == null)
return Collections.emptyIterator();
// Return single-element iterator for the matched vertex
return List.of(vertex).iterator(); |
||
|
|
@@ -570,6 +579,58 @@ private Iterator<Identifiable> tryFindAndUseIndexFromWhere(final DocumentType ty | |
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Extracts ID filter from a boolean expression, resolving dynamic values against the current input result. | ||
| * This handles UNWIND...MATCH...WHERE patterns where ID is filtered dynamically. | ||
| */ | ||
| private String extractDynamicIdFilter(final BooleanExpression expr, final Result currentInputResult) { | ||
| if (expr instanceof ComparisonExpression) { | ||
| final ComparisonExpression comp = (ComparisonExpression) expr; | ||
| if (comp.getOperator() != ComparisonExpression.Operator.EQUALS) | ||
| return null; | ||
|
|
||
| // Check for pattern: ID(variable) = <expression> | ||
| if (comp.getLeft() instanceof FunctionCallExpression) { | ||
| final FunctionCallExpression func = (FunctionCallExpression) comp.getLeft(); | ||
| if ("id".equalsIgnoreCase(func.getFunctionName()) && func.getArguments().size() == 1) { | ||
| final Expression arg = func.getArguments().get(0); | ||
| if (arg instanceof VariableExpression && variable.equals(((VariableExpression) arg).getVariableName())) { | ||
| return evaluateToId(comp.getRight(), currentInputResult); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check for reversed: <expression> = ID(variable) | ||
| if (comp.getRight() instanceof FunctionCallExpression) { | ||
| final FunctionCallExpression func = (FunctionCallExpression) comp.getRight(); | ||
| if ("id".equalsIgnoreCase(func.getFunctionName()) && func.getArguments().size() == 1) { | ||
| final Expression arg = func.getArguments().get(0); | ||
| if (arg instanceof VariableExpression && variable.equals(((VariableExpression) arg).getVariableName())) { | ||
| return evaluateToId(comp.getLeft(), currentInputResult); | ||
| } | ||
| } | ||
| } | ||
| } else if (expr instanceof LogicalExpression) { | ||
| final LogicalExpression logical = (LogicalExpression) expr; | ||
| if (logical.getOperator() == LogicalExpression.Operator.AND) { | ||
| final String leftId = extractDynamicIdFilter(logical.getLeft(), currentInputResult); | ||
| if (leftId != null) return leftId; | ||
| return extractDynamicIdFilter(logical.getRight(), currentInputResult); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private String evaluateToId(final Expression expr, final Result currentInputResult) { | ||
| final Object resolvedValue = evaluator.evaluate(expr, currentInputResult, context); | ||
| if (resolvedValue != null) { | ||
| if (resolvedValue instanceof Identifiable) | ||
| return ((Identifiable) resolvedValue).getIdentity().toString(); | ||
| return resolvedValue.toString(); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Extracts equality predicates of the form variable.property = value from a boolean expression. | ||
| * Supports AND conjunctions. Resolves dynamic expressions against the current input result. | ||
|
|
@@ -603,8 +664,6 @@ private void extractEqualityPredicates(final BooleanExpression expr, | |
|
|
||
| if (propertyName != null && valueExpr != null) { | ||
| // Resolve the value expression | ||
| final ExpressionEvaluator evaluator = new ExpressionEvaluator( | ||
| new CypherFunctionFactory(DefaultSQLFunctionFactory.getInstance())); | ||
| final Object resolvedValue = evaluator.evaluate(valueExpr, currentInputResult, context); | ||
| if (resolvedValue != null) | ||
| predicates.put(propertyName, resolvedValue); | ||
|
|
||
Oops, something went wrong.
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.
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.
The
extractDynamicIdFiltermethod is called for every input row and performs a recursive traversal of thewhereFilterAST. Since the structure of thewhereFilteris constant for the duration of this execution step, this traversal is redundant.It would be more efficient to pre-analyze the
whereFilteronce (e.g., in the constructor) to identify theExpressionthat provides the ID value, and then simply evaluate that expression here. Additionally, consider supportingelementId()in addition toid()for broader Cypher compatibility.