fix: [cypher] batch fixes for #4184, #4185, #4186, #4188, #4189

[robfrank]

Summary

Bundles five Cypher correctness fixes filed together on 2026-05-10 from Neo4j differential testing.

• reduce(...) over a null list may return the initial accumulator instead of null #4186 reduce() over a null list returned the initial accumulator; now returns null per Cypher spec (ReduceExpression).
• Map projection with a function-call computed field may return only the function result instead of a map #4184 Map projection with a function-call computed field (p {.name, upperName: toUpper(p.name)}) collapsed to just the function value. The parser's recursive function-invocation search now skips MapProjectionContext just like it already skipped MapContext.
• RETURN DISTINCT * may collapse multiple rows into one #4189 RETURN DISTINCT * collapsed all rows to one because the deduplication key was built from the unexpanded * return item. ProjectReturnStep now keys off the sorted set of projected properties when isReturnAll() is true.
• Relationship pattern with a variable property-constrained endpoint may match no rows #4188 Endpoint property maps with variable values ((:Person {name: fName})) matched zero rows because MatchRelationshipStep.matchesTargetProperties could not evaluate Expression-typed entries. It now accepts the current row and evaluates expressions, mirroring MatchNodeStep.matchesProperties.
• Nodes deleted earlier in the same query may still match later #4185 Nodes deleted via FOREACH (r IN rels | DELETE endNode(r) DELETE r) were never deleted because DeleteClause only kept the textual target form and the chained-access resolver did not understand function calls. DeleteClause now also stores the parsed Expression list; DeleteStep evaluates it for function-like targets. To match Neo4j's deferred-delete semantics within a FOREACH iteration, ForeachStep enables per-iteration delete batching via a new CommandContext variable (DeleteStep.DEFERRED_DELETE_BATCH_VAR) and flushes the batch edges-first at iteration end.

Test plan

• New regression tests cover each issue:
  • CypherReduceAndShortestPathTest#reduceWithNullList, reduceWithNullListFromOptionalMatch
  • OpenCypherExpressionTest#mapProjectionWithFunctionCallField, mapProjectionWithSizeFunctionField
  • Issue4189ReturnDistinctStarTest (4 cases)
  • Issue4188EndpointVariablePropertyTest (4 cases)
  • Issue4185ForeachDeleteVisibilityTest (2 cases)
• Full Cypher engine suite (5919 tests) green with no regressions.
• Broader graph/write/delete test sweep green (373 additional tests).
• CI to confirm.

Closes #4184
Closes #4185
Closes #4186
Closes #4188
Closes #4189

🤖 Generated with Claude Code

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 1 minor

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

Results:
2 new issues

Category	Results
ErrorProne	1 high
CodeStyle	1 minor

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 84.43% diff coverage · -7.47% coverage variation

Metric	Results
Coverage variation	✅ -7.47% coverage variation
Diff coverage	✅ 84.43% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (7e9f984)	126684	93276	73.63%
Head commit (1abf47c)	158456 (+31772)	104835 (+11559)	66.16% (-7.47%)

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 (#4196)	122	103	84.43%

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

[claude]

Code Review - Cypher Batch Fix PR #4196

Overall this is a well-scoped batch of targeted correctness fixes with good test coverage. Each fix is minimal and addresses a real spec deviation. Below are issues worth addressing before merge.

---

Blocker: Planning document committed to repo

docs/superpowers/plans/2026-05-11-cypher-batch-4184-4189.md appears to be an internal AI agent work-plan artifact. It duplicates the PR description verbatim and carries no value as checked-in documentation. Please remove it.

---

Code style violation - fully qualified name

ProjectReturnStep.java, line 132:

java.util.Collections.sort(propNames);

The project's CLAUDE.md convention is to never use FQNs - always import the class. Either:

• Add import java.util.Collections; and use Collections.sort(propNames), or
• Use the cleaner propNames.sort(null) (natural ordering on String - no import needed).

---

Fragile reflection in test code

Issue4189ReturnDistinctStarTest.extractName():

private static String extractName(final Object node) {
  try {
    final var m = node.getClass().getMethod("getString", String.class);
    return (String) m.invoke(node, "name");
  } catch (final NoSuchMethodException ignored) {
    try {
      final var m = node.getClass().getMethod("getProperty", String.class);
      return (String) m.invoke(node, "name");
    } catch ...

This try/catch reflection cascade is fragile and bypasses the type system. The Result API already provides typed access - result.getElement() returns an Optional<Vertex>, or you can cast the result directly to Document. Every other test in the suite accesses vertex properties through the Result interface directly (e.g., r.<String>getProperty("p.name")). The projected vertex under RETURN * is bound to the name p, so result.getElement() or a map-projection lookup should work cleanly.

---

Design concern - parallel lists in DeleteClause

DeleteClause now holds two parallel lists (variables and expressions) where variables.get(i) and expressions.get(i) are expected to correspond. This is error-prone. A lightweight record or a single List<DeleteTarget> wrapping both the text form and expression would eliminate the coupling. The expressions list can also be null (from the legacy constructor), requiring null checks at every call site.

---

Semantic narrowing in isFunctionLikeTarget

DeleteStep.java:

private static boolean isFunctionLikeTarget(final String variable) {
  final int paren = variable.indexOf('(');
  if (paren <= 0)
    return false;
  ...
}

The expression form is always available now (parsed unconditionally in CypherASTBuilder.visitDeleteClause). The guard isFunctionLikeTarget(variable) that gates whether to use it re-parses the text to decide whether the already-parsed Expression is useful. A cleaner heuristic: always prefer the expression form when non-null and when the expression is not a simple VariableExpression (or equivalent). The current text-scan approach is correct for the targeted case but it will silently fall back to the legacy resolver for method-chain forms like a.method().

---

Transaction semantics change in ForeachStep

Previously ForeachStep let the outer transaction (if any) own all writes. The new code begins/commits a transaction per iteration when no outer transaction is active:

if (!wasInTransaction)
  context.getDatabase().begin();
// ... execute clauses ...
DeleteStep.flushDeferredDeletes(context, deleteBatch);
if (!wasInTransaction)
  context.getDatabase().commit();

This means a FOREACH over a 1 000-element list with no enclosing transaction now issues 1 000 begin/commit pairs. The original behaviour was a single transaction spanning all iterations (the commit happened outside the loop). Please verify this is intentional and consider documenting or benchmarking. If it is intentional (to mimic per-iteration atomicity), a note in the code or release notes would be valuable.

---

hasOnlyDetachedEdges - iterator not closed

private static boolean hasOnlyDetachedEdges(final Vertex v) {
  return !v.getEdges(Vertex.DIRECTION.OUT).iterator().hasNext()
      && !v.getEdges(Vertex.DIRECTION.IN).iterator().hasNext();
}

getEdges() may return a resource-holding Iterable backed by a cursor. The iterator() is obtained but never closed, risking a cursor/resource leak. If the underlying iterator implements Closeable (as many ArcadeDB iterators do) it should be closed - typically via try-with-resources or by checking instanceof AutoCloseable.

---

Minor: Boolean.valueOf() boxing is unnecessary

deferredBatch.add(new Object[] { edge, Boolean.valueOf(deleteClause.isDetach()) });

Auto-boxing (deleteClause.isDetach() directly) is idiomatic Java. Boolean.valueOf() is not wrong but is redundant with modern Java.

---

Test coverage gaps for #4185

Issue4185ForeachDeleteVisibilityTest covers the core scenario well but is missing:

• DETACH DELETE inside FOREACH (confirm the detach=true path in flushDeferredDeletes)
• Nested FOREACH with DELETE (confirm the previousBatch save/restore in finally)
• A FOREACH where inner clauses other than DELETE are also present (e.g., SET + DELETE mixed)

---

Positive notes

• The reduce(null) fix (reduce(...) over a null list may return the initial accumulator instead of null #4186) is a single correct line change, cleanly spec-compliant.
• The MapProjectionContext skip in findFunctionInvocationRecursive (Map projection with a function-call computed field may return only the function result instead of a map #4184) is the exact parallel of the existing MapContext skip - hard to fault.
• The DEFERRED_DELETE_BATCH_VAR save/restore in the finally block correctly handles nested FOREACH.
• Test classes for Relationship pattern with a variable property-constrained endpoint may match no rows #4188 and RETURN DISTINCT * may collapse multiple rows into one #4189 include control cases (WHERE-clause equivalent, no-DISTINCT equivalent), which is good practice.
• Removing the Collectors import in CypherASTBuilder is a nice cleanup.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request addresses five Cypher-related issues (#4184, #4185, #4186, #4188, #4189) by refining expression evaluation, deduplication logic, and deletion behavior. Key improvements include fixing reduce() semantics for null lists, ensuring map projections correctly handle function calls, and implementing deferred deletion within FOREACH loops to maintain correct graph state visibility. Review feedback suggests enhancing type safety in DeleteStep by replacing the List<Object[]> used for deferred deletes with a dedicated static inner class.

[gemini-code-assist]

For better type safety and readability, consider creating a small static inner class to hold the deferred delete information instead of using a List<Object[]>. This avoids raw array indexing and casting, making the code more robust and easier to understand.

You could define a class like this within DeleteStep:

private static class DeferredDelete {
  final Object target;
  final boolean detach;

  DeferredDelete(Object target, boolean detach) {
    this.target = target;
    this.detach = detach;
  }
}

Then, the deferredBatch would be of type List<DeferredDelete>, and you would use it like this:

In applyDeleteOperations:

// ...
final List<DeferredDelete> deferredBatch = (List<DeferredDelete>) context.getVariable(DEFERRED_DELETE_BATCH_VAR);
// ...
deferredBatch.add(new DeferredDelete(edge, deleteClause.isDetach()));
// ...
deferredBatch.add(new DeferredDelete(other, deleteClause.isDetach()));

In flushDeferredDeletes:

public static void flushDeferredDeletes(final CommandContext context, final List<DeferredDelete> batch) {
  // ...
  final List<DeferredDelete> others = new ArrayList<>();
  // ...
  final Object target = entry.target;
  final boolean detach = entry.detach;
  // ...
}

This change would also need to be applied in ForeachStep.java.

[robfrank]

Done in d7d0a1e - replaced List<Object[]> with a new public record DeleteStep.DeferredDeleteTarget(Object target, boolean detach). The deferred-batch consumer is also updated to use it, dropping the positional indexing and the redundant Boolean.valueOf.

[robfrank]

Response to review (d7d0a1e)

Addressed:

• Planning doc: removed. Duplicated the PR description and added no standalone value.
• FQN Collections.sort: switched to propNames.sort(null) (no new import; natural ordering).
• Reflection in Issue4189ReturnDistinctStarTest: dropped the reflection helper; the column is now cast to Vertex and read via Vertex.getString("name").
• Per-iteration transaction in ForeachStep: confirmed regression. Hoisted begin/commit out of the iteration loop. The DEFERRED_DELETE_BATCH_VAR save/restore still happens per iteration (needed for nested FOREACH isolation), but the transaction now spans the whole FOREACH again.
• hasOnlyDetachedEdges iterator leak: rewrote with a CloseableIteratorRef wrapper used via try-with-resources. If the underlying iterator implements AutoCloseable, it gets closed.
• Object[] pairs in deferred batch: introduced DeleteStep.DeferredDeleteTarget(Object target, boolean detach) record. Eliminates raw arrays, positional indexing, and the Boolean.valueOf boxing. Also addresses gemini-code-assist's review.
• Test coverage gaps: added detachDeleteInsideForeachClearsBothEndsAndEdge, nestedForeachDeleteWorks, and mixedSetAndDeleteInsideForeach to Issue4185ForeachDeleteVisibilityTest.

Pushing back:

• Parallel lists in DeleteClause: kept. The expressions field is added alongside variables rather than replacing it because CypherSemanticValidator.validateDeleteTargets, the prettyPrint count, and the legacy resolver all consume the text form. A DeleteTarget record would be cleaner long-term but the constructor signature change ripples wider than this batch should touch. The two lists are populated in the same loop in visitDeleteClause and indexed by the same i, so the coupling is local to one place.
• Semantic narrowing in isFunctionLikeTarget: kept. The Cypher expressions parsed for non-function targets are VariableExpression, ChainedPropertyAccessExpression, etc. The legacy resolveChainedAccess handles all of those without going through the evaluator. Routing everything through Expression.evaluate would work but is a behavioral change with broader regression surface than this batch warrants. Limiting the expression path to identifiers that look like function calls keeps the targeted fix scoped to what was broken.

Test results: 5919 OpenCypher engine tests pass, including the three new FOREACH+DELETE cases.

[claude]

Code Review - PR #4196: Batch Cypher fixes for #4184, #4185, #4186, #4188, #4189

Good set of correctness fixes overall. Each change is logically sound and targets a real spec violation. The test coverage is solid. Below are my observations, ordered roughly by severity.

---

Correctness / Logic

#4186 ReduceExpression null fix - correct and minimal. Empty-list vs null-list distinction is now properly handled. Minor note: this is technically a breaking behavior change for callers who relied on reduce(s=0, x IN null | ...) returning 0. Worth calling out in release notes.

#4184 findFunctionInvocationRecursive skip MapProjectionContext - clean and mirrors the existing MapContext guard. No issues.

#4189 RETURN DISTINCT * dedup key - the fix (sort property names, build key from projected result) is correct. One concern: sorting allocates a new ArrayList per row. This is fine for typical query sizes but could be measurable for very wide result sets with thousands of rows. Consider TreeSet on getPropertyNames() directly to avoid the intermediate list copy:

final Set<String> propNames = new TreeSet<>(projectedResult.getPropertyNames());
for (final String name : propNames) { ... }

#4188 matchesTargetProperties Expression evaluation - correct and mirrors MatchNodeStep. The currentResult != null guard is defensive; it would be worth understanding when currentResult could be null here, or assert it can't and remove the guard.

#4185 FOREACH deferred DELETE - the logic is correct in intent, but has a few areas worth reviewing:

1. hasOnlyDetachedEdges is misnamed. The method returns true when the vertex has no edges, not "only detached edges." A clearer name would be hasNoEdges or isIsolated. The current name implies there could be detached edges present, which is confusing.

2. Exception swallowing in hasOnlyDetachedEdges - catching Exception and ignoring it silently could hide real problems (disk errors, corruption, etc.). If the intent is only to handle a closed cursor or already-deleted record, catch RecordNotFoundException specifically:

   try {
     if (v.getEdges(Vertex.DIRECTION.OUT).iterator().hasNext()) return false;
   } catch (RecordNotFoundException ignored) {
     // vertex was deleted in this batch - treat as isolated
   }

3. CloseableIteratorRef adds unnecessary complexity. The wrapper class exists only to call close() via try-with-resources on an iterator that may or may not be AutoCloseable. This is simpler as a utility method:

   private static boolean hasNoEdges(final Vertex v) {
     Iterator<?> it = v.getEdges(Vertex.DIRECTION.OUT).iterator();
     try { if (it.hasNext()) return false; }
     finally { if (it instanceof AutoCloseable ac) { try { ac.close(); } catch (Exception ignored) {} } }
     it = v.getEdges(Vertex.DIRECTION.IN).iterator();
     try { if (it.hasNext()) return false; }
     finally { if (it instanceof AutoCloseable ac) { try { ac.close(); } catch (Exception ignored) {} } }
     return true;
   }

   Or even simpler: check !v.getEdges(Vertex.DIRECTION.BOTH).iterator().hasNext() (if the engine supports BOTH direction).

4. isFunctionLikeTarget is a heuristic on text. It works correctly because Cypher identifiers cannot contain (, but this is fragile layering - the parser already knows it's a function call (that's why expressions was added). The guard isFunctionLikeTarget(variable) should be unnecessary since expressions.get(i) != null is already sufficient to distinguish a parsed expression from a bare identifier. If an expression was parsed successfully it should always be used. Consider simplifying:

   if (expressions != null && i < expressions.size() && expressions.get(i) != null) {
     obj = expressions.get(i).evaluate(result, context);
   } else {
     obj = resolveDeleteTarget(variable, result);
   }

5. ForeachStep - transaction wrapping now owns the full batch lifecycle. Previously each inner DeleteStep was responsible for its own transaction. The new code wraps the entire FOREACH iteration in a single transaction. This is correct, but it means any exception mid-iteration could leave the database in an intermediate state if the outer ForeachStep is itself nested in a larger outer transaction - the rollback() path only fires when !wasInTransaction. Is this the desired behavior for nested transactions in the Cypher engine?

6. Catch scope in ForeachStep is narrowed to RuntimeException. The original DeleteStep.applyDeleteOperations threw Exception. The new ForeachStep catch catches only RuntimeException. If a checked exception ever surfaces from an inner clause, it will bypass the rollback. Consider catching Exception and rethrowing:

   } catch (final Exception e) {
     if (!wasInTransaction && context.getDatabase().isTransactionActive())
       context.getDatabase().rollback();
     if (e instanceof RuntimeException re) throw re;
     throw new CommandExecutionException("Unexpected error in FOREACH", e);
   }

---

Test coverage

Tests are good. A few additions would strengthen the suite:

• Issue4185ForeachDeleteVisibilityTest is missing a test for FOREACH inside an already-active transaction (i.e., database.transaction(() -> { database.command("opencypher", "FOREACH ...") })). The wasInTransaction path is untested.
• Issue4188EndpointVariablePropertyTest covers WITH and UNWIND binding sources, but not a parameter binding ($param). Worth adding since the existing ParameterReference handling is adjacent to the new Expression evaluation path.
• Issue4189ReturnDistinctStarTest - the returnDistinctStarAfterTrivialWith test only checks count, not actual values. Asserting containsExactlyInAnyOrder("Alice", "Bob") would make the assertion tighter.

---

Style / CLAUDE.md conventions

Per CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks - one short line max."

Several new methods have multi-paragraph Javadocs (DEFERRED_DELETE_BATCH_VAR, DeferredDeleteTarget, flushDeferredDeletes, CloseableIteratorRef, isFunctionLikeTarget, and the Javadocs on the ReduceExpression and MatchRelationshipStep changes). These should be trimmed to single-line comments or removed entirely where the code is self-explanatory.

---

Summary

The five fixes are all correct and well-targeted. The main things I'd address before merging:

1. Rename hasOnlyDetachedEdges -> hasNoEdges (misleading name)
2. Drop the CloseableIteratorRef wrapper in favor of inline AutoCloseable check
3. Remove the isFunctionLikeTarget heuristic - rely on expressions.get(i) != null instead
4. Narrow the exception swallowing in the edge-check methods
5. Trim multi-paragraph Javadoc blocks to comply with project style

Good work on the test coverage - regression tests for all five issues plus control cases is exactly the right approach.

[robfrank]

Response to review round 2 (1abf47c)

Addressed:

• hasOnlyDetachedEdges misnamed: renamed to hasNoEdges and rewritten as a single vertex.countEdges(Vertex.DIRECTION.BOTH) == 0L call. The CloseableIteratorRef wrapper is gone entirely - countEdges does not expose a cursor, so there is nothing to leak.
• Exception swallowing: now catches only RecordNotFoundException (vertex deleted earlier in the batch flush -> treat as isolated).
• ForeachStep catch narrowed to RuntimeException: widened back to Exception. Checked exceptions are now wrapped in CommandExecutionException to preserve the rollback path. Matches the original behavior of DeleteStep.applyDeleteOperations which caught Exception.
• TreeSet for DISTINCT * key: switched to new TreeSet<>(projectedResult.getPropertyNames()), dropping the intermediate ArrayList + explicit sort. Same allocation footprint, clearer intent.
• Multi-paragraph Javadocs: trimmed to single lines on DEFERRED_DELETE_BATCH_VAR, DeferredDeleteTarget, flushDeferredDeletes, isFunctionLikeTarget, matchesTargetProperties, plus the inline comment block in applyDeleteOperations and ForeachStep.executeForeach.
• returnDistinctStarAfterTrivialWith assertion: now asserts names contain exactly "Alice" and "Bob", not just a count of 2.
• FOREACH-inside-outer-transaction test: added foreachDeleteInsideOuterTransaction to confirm the outer transaction is reused (wasInTransaction == true path; no begin/commit inside the FOREACH).
• $param binding test for Relationship pattern with a variable property-constrained endpoint may match no rows #4188: added endpointPropertyBoundFromParameter to cover ParameterReference resolution alongside the new Expression path.

Pushing back:

• Drop isFunctionLikeTarget, always use the parsed Expression: kept the guard. The legacy resolveChainedAccess handles DELETE a.b[0] shapes that exercise different code paths from Expression.evaluate. Eliminating the gate would route every non-function DELETE through evaluator code that was not previously on this path - a wider regression surface than this batch warrants. The full-Expression rewrite is a reasonable follow-up but not part of these five fixes.
• currentResult != null guard in matchesTargetProperties: kept. The guard mirrors MatchNodeStep.matchesProperties exactly. Removing one without the other splits the convention; addressing both would be its own change.
• reduce(null) release-note flag: the change is a bug fix aligning with the Cypher spec - Neo4j has always returned null for this case. The PR title and body call it out; no further release-note work is needed in-tree.
• Nested-transaction rollback concern: the current behavior is correct. When wasInTransaction == true, FOREACH inherits the outer transaction and the caller owns rollback. The new foreachDeleteInsideOuterTransaction test exercises this path.

5919 OpenCypher engine tests still pass, including the three new tests.

[claude]

Code Review - PR #4196 (Batch Cypher Fixes)

Good batch of targeted fixes from differential testing. Overall the approach is sound, but I have a few observations across the individual changes.

---

#4186 - reduce() over null list

Clean fix. The one-liner return null instead of return accumulator is exactly correct per the Cypher spec, and the two regression tests (direct null and OPTIONAL MATCH path) cover the right edge cases.

---

#4184 - Map projection with function-call computed field

Clean fix. Extending the MapContext guard to also skip MapProjectionContext in findFunctionInvocationRecursive is the right scalpel. Good that the guard comment was updated to explain why both are excluded.

---

#4189 - RETURN DISTINCT *

Correct fix, minor performance note.

Using TreeSet<>(projectedResult.getPropertyNames()) is correct - it gives a stable, sorted iteration order so the deduplication key is deterministic across rows. However, TreeSet allocates per-row under DISTINCT *, which adds GC pressure on large result sets. Since the set of projected property names is stable within a query (same columns every row), the sorted name list could be computed once before the loop and reused. That said, this is minor and pre-existing patterns in this class do similar per-row work, so fine for now as a correctness fix.

---

#4188 - Endpoint variable property in MatchRelationshipStep

Looks correct. The signature change from matchesTargetProperties(vertex) to matchesTargetProperties(vertex, lastResult) with the Expression evaluation branch mirrors what MatchNodeStep.matchesProperties already does. Null guard on currentResult is correct. The four test variants (WITH variable, WHERE, literal, parameter, UNWIND) provide good coverage.

---

#4185 - FOREACH + DELETE deferred batching

This is the most complex change. A few observations:

isFunctionLikeTarget heuristic is fragile (potential bug):

// DeleteStep.java ~line 212
private static boolean isFunctionLikeTarget(final String variable) {
    final int paren = variable.indexOf('(');
    ...
}

This method decides whether to use the parsed Expression or the legacy string resolver by scanning the textual form of the expression. Since we now have the full Expression AST list in DeleteClause, this heuristic is unnecessary. A cleaner check would be something like:

// At the use site - no separate heuristic needed:
if (expressions != null && i < expressions.size() && expressions.get(i) != null)
    obj = expressions.get(i).evaluate(result, context);
else
    obj = resolveDeleteTarget(variable, result);

The current check isFunctionLikeTarget(variable) means that a plain variable like n would still use the string path even when an Expression is available. Is there a case where the two paths would diverge for a plain variable name? If expressions.get(i) correctly evaluates a plain variable reference, the heuristic branch is never needed. The heuristic is only guarding against a regression, but since expressions are always populated now (via the new visitDeleteClause), the guard condition expressions != null && i < expressions.size() is sufficient.

Parallel lists design smell in DeleteClause:
Two parallel List<String> variables and List<Expression> expressions indexed by position are easy to de-sync. A single List<DeleteTarget> record would be safer:

public record DeleteTarget(String variable, Expression expression) {}

Not a blocker for a bug-fix PR, but worth a follow-up.

ForeachStep now always manages a transaction:

// ForeachStep.java
final boolean wasInTransaction = context.getDatabase().isTransactionActive();
try {
    if (!wasInTransaction)
        context.getDatabase().begin();
    // ... loop ...
    if (!wasInTransaction)
        context.getDatabase().commit();

Before this PR, ForeachStep did not manage transactions at all. Now it wraps ALL inner-clause execution (including CREATE, SET) in a transaction when none is active. This is semantically correct for atomicity, but it is a behavior change: previously, inner CreateStep/SetStep might have started their own transactions per-step. Is there any risk of double-begin when, for example, CreateStep starts a transaction during the loop and then ForeachStep tries to commit after? ArcadeDB likely handles nested begin() gracefully (no-op if active), but this interaction is worth verifying explicitly, particularly for FOREACH with only CREATE/SET and no DELETE.

previousBatch restore in the inner finally is correct - the save/restore nesting properly isolates outer FOREACH batches from inner ones. The nested FOREACH test (nestedForeachDeleteWorks) covers this, good.

flushDeferredDeletes clearing batch: The batch.clear() at the end is safe since the list is created fresh per iteration, but it is defensive only (the list goes out of scope anyway). No issue.

PR description says "(2 cases)" for Issue4185ForeachDeleteVisibilityTest but the file contains 6 test methods. Minor documentation inconsistency.

---

General

• All tests follow the project's JUnit 5 + AssertJ patterns correctly.
• final usage on parameters and locals is consistent with the codebase style.
• No System.out debug lines visible.
• No new dependencies added.
• License headers present on all new files.
• Collectors.toList() removal in CypherASTBuilder (replaced with explicit ArrayList) is a nice micro-cleanup.

---

Summary

Fix	Assessment
#4186 reduce/null	✅ Clean, correct
#4184 map projection	✅ Clean, correct
#4189 DISTINCT *	✅ Correct; minor GC note
#4188 endpoint variable property	✅ Correct
#4185 FOREACH+DELETE	✅ Correct; isFunctionLikeTarget heuristic can be simplified, ForeachStep transaction scope change worth a second look

No blockers found. The isFunctionLikeTarget simplification and the ForeachStep transaction interaction are the two items worth addressing before merge or tracking as follow-up.

[codecov]

Codecov Report

❌ Patch coverage is 76.22951% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.58%. Comparing base (7e9f984) to head (1abf47c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...db/query/opencypher/executor/steps/DeleteStep.java	68.11%	13 Missing and 9 partials ⚠️
...b/query/opencypher/executor/steps/ForeachStep.java	75.00%	6 Missing ⚠️
...encypher/executor/steps/MatchRelationshipStep.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4196      +/-   ##
==========================================
+ Coverage   64.56%   64.58%   +0.01%     
==========================================
  Files        1645     1645              
  Lines      126684   126778      +94     
  Branches    27098    27123      +25     
==========================================
+ Hits        81799    81882      +83     
+ Misses      33438    33435       -3     
- Partials    11447    11461      +14     

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