fix(opencypher): MATCH WHERE ID(n) = <expr> falls back to full scan when expr is dynamic

[ExtReMLapin]

Fixes #3864

Long story short, in

UNWIND $batch AS BatchEntry
MATCH (b:CHUNK) WHERE ID(b) = BatchEntry.destRID
CREATE (p:CHUNK_EMBEDDING {vector: BatchEntry.vector})
CREATE (p)-[:embb]->(b)  

MATCH (b:CHUNK) WHERE ID(b) = BatchEntry.destRID is doing a full scan on CHUNK

[ExtReMLapin]

Gemini wrote that, claude checked it

[gemini-code-assist]

Code Review

This pull request implements dynamic ID filtering within MatchNodeStep to optimize OpenCypher queries where the node ID is specified in a WHERE clause. It introduces logic to extract ID filters from boolean expressions and refactors the ExpressionEvaluator into a class member for reuse. Review feedback highlights a potential NullPointerException when a vertex is not found by its RID and suggests optimizing the AST traversal by pre-analyzing the filter expression.

[gemini-code-assist]

The List.of(vertex) call will throw a NullPointerException if the RID is valid but the vertex does not exist in the database (i.e., lookupByRID returns null). Since List.of does not allow null elements, you should check for null and return an empty iterator instead.

        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();

[gemini-code-assist]

The extractDynamicIdFilter method is called for every input row and performs a recursive traversal of the whereFilter AST. Since the structure of the whereFilter is constant for the duration of this execution step, this traversal is redundant.

It would be more efficient to pre-analyze the whereFilter once (e.g., in the constructor) to identify the Expression that provides the ID value, and then simply evaluate that expression here. Additionally, consider supporting elementId() in addition to id() for broader Cypher compatibility.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[ExtReMLapin]

According to claude (but I don't have any tokens left) There might be other cases where it's not fixed, for example

UNWIND $batch AS BatchEntry
MATCH (b:CHUNK {someKey: BatchEntry.value})  -- inline props, not WHERE clause

2. MergeStep.findNode() / findAllNodes() - always full scan
   MERGE (n:CHUNK {name: $value})

[lvca]

It actually makes sense! Merged, thanks!! I'm going to write some test cases to avoid future regressions.

[codecov]

Codecov Report

❌ Patch coverage is 38.82353% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.94%. Comparing base (973cb52) to head (801337d).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...edb/query/opencypher/executor/steps/MergeStep.java	16.66%	32 Missing and 8 partials ⚠️
...query/opencypher/executor/steps/MatchNodeStep.java	67.56%	5 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3865      +/-   ##
==========================================
+ Coverage   64.66%   64.94%   +0.28%     
==========================================
  Files        1579     1579              
  Lines      116503   116618     +115     
  Branches    24707    24749      +42     
==========================================
+ Hits        75335    75742     +407     
+ Misses      30871    30504     -367     
- Partials    10297    10372      +75     

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