feat(#5000): populate Bolt ResultSummary counters for write queries

[robfrank]

Closes #5000. Part of #4890 (Group C protocol/type-fidelity gaps), epic #4882 (Bolt Driver Compatibility Certification).

Problem

ResultSummary counters (nodesCreated, relationshipsCreated, propertiesSet, ...) were never populated for Bolt write queries. Two layers were missing: (1) BoltNetworkExecutor built no stats map in the SUCCESS metadata, and (2) the native Cypher engine had no CRUD-counter tracking to source it from.

Approach

A reusable, allocation-light engine capability, consumed by the Bolt layer:

• QueryStatistics - a mutable accumulator of primitive int counters (nodes/relationships created/deleted, properties-set, labels added/removed, indexes/constraints added/removed) with containsUpdates().
• CommandContext.getStatistics() - lazily allocated, so pure read queries (MATCH/RETURN) never create one and take no new hot-path branch.
• Mutation steps (CreateStep, SetStep, DeleteStep, MergeStep, RemoveStep) increment the accumulator with Neo4j-compatible semantics; the DDL path (OpenCypherQueryEngine.executeDDL) counts index/constraint add/remove, guarded so IF [NOT] EXISTS no-ops don't over-count.
• ResultSet.getStatistics() - a new default method mirroring getExecutionPlan() (zero blast radius on other implementations); CypherExecutionPlan.execute() and executeDDL() attach the accumulator to the returned result set.
• BoltNetworkExecutor - handlePull/handleDiscard read it and emit a Neo4j hyphenated stats map (non-zero counters + contains-updates) on the terminal SUCCESS, only for writes.

Counters reflect the statement's work regardless of a later transaction rollback (Neo4j-correct).

Conformance

Flips RESULT-004 in bolt/conformance/spec.yaml from expected-fail to passing, and unwraps the expected-fail harness in all five per-language e2e suites (Java, JS, Python, Go, C#) so each asserts the real counters.

Testing

• Engine unit tests (QueryStatisticsTest, ResultSetStatisticsTest, CypherQueryStatisticsTest): 21 cases covering CREATE/SET/DELETE/MERGE/REMOVE, SET/REMOVE label, CREATE/DROP INDEX/CONSTRAINT, distinct-label dedup, SET n = {map} replace semantics, absent-property removal (counts 0), and a read-query-allocates-nothing regression.
• Bolt IT (Bolt5000ResultCountersIT) via the real neo4j-java-driver: asserts summary.counters() for a write and no updates for a read.
• Cypher mutation + DDL regression subsets and the full com.arcadedb.query.opencypher package: green.
• Spec validator (bolt/conformance/validate_spec.py): 39 scenarios OK.

Scope / follow-up

Scoped to the engine accumulator + Bolt consumption. Surfacing the same counters over HTTP /command and gRPC, aggregating counts across top-level UNION writes and CALL { ... } write subqueries (currently documented in-code as returning zero to the caller), and constraint-kind test coverage are tracked in #5015.

🤖 Generated with Claude Code

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[gemini-code-assist]

Code Review

This pull request implements Bolt ResultSummary counters for write queries by introducing a QueryStatistics accumulator on CommandContext and exposing it via ResultSet.getStatistics(). It instruments Cypher mutation steps and the DDL path to track CRUD and schema changes, mapping them to the Neo4j Bolt SUCCESS metadata map. The review feedback highlights a potential NullPointerException in OpenCypherQueryEngine if a type name does not exist in the schema, as well as potential overcounting of propertiesSet in SetStep and MergeStep when handling maps with null values for non-existent properties.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

If typeName does not exist in the schema, schema.getType(typeName) will return null. Calling getIndexByProperties or getPolymorphicIndexByProperties on a null reference will throw a NullPointerException. Add a defensive null check for type before accessing its methods.

  private static boolean indexExistsOnProperties(final Schema schema, final String typeName, final String[] propertyNames) {\n    final DocumentType type = schema.getType(typeName);\n    if (type == null)\n      return false;\n    return type.getIndexByProperties(propertyNames) != null || type.getPolymorphicIndexByProperties(propertyNames) != null;\n  }

[robfrank]

Guarded with an existsType check in 46fef7d. Two clarifications on the mechanism: in this codebase LocalSchema.getType(...) throws SchemaException on a missing type rather than returning null, so a type == null check would be dead code; and both callers (executeCreateIndex, executeCreateConstraint) auto-create the type via getOrCreateVertexType/EdgeType before this helper runs, so the path was not reachable. The existsType guard hardens it against a future caller regardless - a missing type now correctly reports "no index".

[gemini-code-assist]

Using map.size() unconditionally can lead to overcounting if the map contains null values for properties that did not previously exist on the document (since removing a non-existent property is a no-op and should not increment the propertiesSet counter). Consider only counting non-null entries and null entries for properties that actually existed before the merge, similar to how it is handled in the replace-map branch (applyReplaceMap).

[robfrank]

Correct - fixed in 46fef7d. The += branch now counts each non-null assignment and each null-removal only when the property actually existed (mutableDoc.has(...) before remove), matching applyReplaceMap. Added a regression test (SET n += {c:3, b:null, z:null} on a node with b but not z asserts propertiesSet == 2).

[gemini-code-assist]

Using map.size() unconditionally can lead to overcounting if the map contains null values for properties that did not previously exist on the document (since removing a non-existent property is a no-op and should not increment the propertiesSet counter). Consider only counting non-null entries and null entries for properties that actually existed before the merge, similar to how it is handled in the replace-map branch.

[robfrank]

Same fix applied to the MERGE_MAP branch in 46fef7d, with a regression test through MERGE ... ON MATCH SET n += {a:null, absent:null, b:2} asserting propertiesSet == 2 (removal of the absent key is not counted).

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 54 complexity

Metric	Results
Complexity	54

View in Codacy

_{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]

Review: feat(#5000): populate Bolt ResultSummary counters for write queries

Nicely scoped, well-tested change. The lazy QueryStatistics on CommandContext keeps the read hot-path allocation-free, the default method on ResultSet gives it zero blast radius on other implementations, and the Neo4j-hyphenated stats map is emitted only for writes. Coverage is genuinely good: 21 engine unit cases plus a real neo4j-java-driver IT covering PULL and DISCARD, and the conformance/e2e wiring is unwrapped consistently across all five languages. Below are observations, ordered by importance.

Correctness

1. Silent under-reporting for UNION / CALL {} writes (documented, but user-visible). executeUnion() and executeWithSeedRow() each spin up their own BasicCommandContext, so a write performed inside a branch/subquery never reaches the outer ResultSet's statistics. The consequence over Bolt is not just a zero counter but contains-updates: false for a statement that did mutate the graph - a driver-visible incorrectness, not merely a missing detail. The in-code comments and follow-up Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015 are the right call for scope, but consider whether the terminal SUCCESS should at least force contains-updates: true when the statement is known to be a write, even if the granular counters are 0, to avoid actively-wrong summaries. Worth calling out explicitly in Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015.

2. propertiesSet derives from getPropertyNames().size() on CREATE. For CreateStep/MergeStep node+edge creation this assumes every persisted property name is a user property. That holds today and the IT confirms propertiesSet == 2, but it couples the counter to the serializer's property surface - if internal/system properties ever leak into getPropertyNames(), the count silently inflates. A brief comment noting the assumption (as already done for the label-dedup) would harden it.

3. DETACH DELETE double-count edge case. deleteAllEdges() and deleteVertex() don't consult the deleted set that the batch deleteObjectStatic() path uses. For a query that both DETACH DELETEs a node and explicitly matches/deletes one of its relationships, the same edge could be counted twice (Neo4j dedups internally). Likely rare and not a regression, but a candidate test case for Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015.

Thread-safety

4. QueryStatistics uses plain int fields and getStatistics() on BasicCommandContext is an unsynchronized check-then-set. This is fine because Cypher mutation steps execute single-threaded per query and the context is per-query-thread - but given the engine's query-parallelism pool (see CLAUDE.md concurrency section), a one-line class Javadoc stating "not thread-safe; single writer per command context" would prevent a future parallel-mutation step from silently racing the counters.

Style / minor

5. BoltResultStats.putIfNonZero widens to long for the wire while QueryStatistics stores int - correct for the Neo4j protocol, just confirming it is intentional (it is; Bolt integers are 64-bit).
6. The two large design/plan docs under docs/superpowers/ (~1000 lines) are committed alongside the code. Fine if that is the repo convention, but they will drift from the implementation over time - worth confirming they belong in the tree vs. the issue.

Verification

Could not run the full build in this review environment, but the diff compiles cleanly against the interfaces it touches, no System.out debug is left behind, new test files carry license headers, and the default-method approach means no other ResultSet implementation needed changes. The conformance flip (RESULT-004 -> passing) and the five e2e unwraps are consistent with the engine change.

Overall this is a clean, well-tested addition. The main thing I would want tracked before closing the epic is the UNION/CALL contains-updates: false behavior (item 1), since that is the one place the summary can be actively wrong rather than just incomplete.

🤖 Generated with Claude Code

[robfrank]

Responses to the review, by item:

1. UNION / CALL contains-updates: false (actively wrong). Agreed this is the sharpest of the gaps: type: "w" but no stats map means a mutating statement reports containsUpdates() == false, which is wrong rather than merely incomplete. I considered forcing contains-updates: true for known-writes with zero granular counters, but that produces an internally inconsistent summary (updates happened, yet every counter is 0) and still misreports the actual work. The correct fix is propagating the child-context counters up through executeUnion()/executeWithSeedRow(). I have expanded follow-up #5015 to call this out explicitly as active incorrectness (not just missing counts), so it is tracked rather than silently shipped.

2. propertiesSet from getPropertyNames().size() on CREATE. Added a comment at the create sites noting the invariant that a freshly created record`\s property names are exactly the user-set properties (labels/type are not properties), so the coupling to the serializer surface is documented (46fef7d).

3. DETACH DELETE double-count. Real edge case - deleteAllEdges()/deleteVertex() do not consult the batch deleted set, so a query that both DETACH DELETEs a node and explicitly deletes one of its edges could count that edge twice where Neo4j dedups. Not a regression and niche; added it to #5015 with a note to add the repro test there.

4. Thread-safety. Added a class Javadoc line to QueryStatistics: not thread-safe, single writer per command context, plain ints with no synchronization (46fef7d). Correct that it is safe today because mutation steps run single-threaded per query; the note guards against a future parallel-mutation step.

5. putIfNonZero long widening. Intentional - Bolt integers are 64-bit; confirmed.

6. docs/superpowers/ spec/plan in-tree. This is the established repo convention (spec/plan docs are committed alongside feature PRs here), so keeping them. Point taken on drift.

Also addressed Gemini's inline findings in 46fef7d (the SET n += {map} null-for-absent over-count in both SetStep and MergeStep, plus a defensive existsType guard on the DDL type lookup). Two new regression tests; CypherQueryStatisticsTest 21/21 and the mutation/DDL regression subset 170/170 green.

[claude]

Code Review: Bolt ResultSummary counters for write queries (issue 5000)

Reviewed the full diff (27 files, engine accumulator + Bolt consumption + conformance/e2e flips). Overall this is a high-quality, well-tested change: clean layering (QueryStatistics is engine-neutral, Bolt owns the wire-key mapping), a genuinely allocation-light hot path, TDD discipline with 21 unit cases plus a real neo4j-java-driver IT, and honest in-code documentation of the known gaps. The counting semantics are carefully worked out (distinct-label dedup, absent-property removal counting 0, replace-map counting removed-plus-written). Nice work.

A few points worth considering before merge.

Correctness / potential bugs

1. propertiesSet from getPropertyNames().size() on CREATE may diverge from Neo4j (CreateStep.createVertex/createEdge; same in MergeStep.createVertex). Counting all property names of the freshly-saved record is correct for the common case, but two edge cases are worth a quick check:

   • CREATE (n:Foo {a: 1, b: null}) - Neo4j does not set a null-valued property, so propertiesSet == 1. If ArcadeDB materializes b (or drops it), the count could be off by one versus Neo4j.
   • A type carrying a default-valued or otherwise auto-populated property would inflate getPropertyNames() beyond the user-supplied keys.
     Both are outside the current test matrix. A one-line test for the null-inline-property case would lock the contract down.

2. UNION and CALL write subqueries report zero counters (documented at CypherExecutionPlan.executeUnion/executeWithSeedRow, tracked in issue 5015). This is the most consequential caveat: since RESULT-004 is now flipped to passing across all five e2e suites, a user issuing a ... UNION ... write through a Neo4j driver gets a silently wrong (zero) summary.counters() rather than an error. Acceptable as a documented limitation, but please confirm no conformance scenario exercises a UNION/CALL write, otherwise the certification has a hole. Worth a sentence on why zero-but-wrong is preferable to not emitting a stats map at all for those shapes.

3. BasicCommandContext.getStatistics() does not consult the parent context. This is the underlying mechanism behind limitation 2: any child context created mid-plan accumulates into its own instance and the counts are dropped. If the intent is a top-level-only accumulator, that is fine, but delegating to the parent when present (as several other BasicCommandContext accessors already do) would make it robust to future nested-context refactors and could close the UNION/CALL gap cheaply.

Maintainability

4. The Neo4j replace-map counting rule is duplicated in 4 places. The idiom !prop.startsWith("@") && map.get(prop) == null appears in SetStep.applyReplaceMap (line ~281), SetStep.applyMergeMap, and the MergeStep REPLACE_MAP (line ~1361) and MERGE_MAP branches. The tests cover each path today, but four independent copies of the same subtle semantics will drift. Extracting a small static helper (e.g. in QueryStatistics or an opencypher util) would single-source the rule and shrink each step.

Minor

5. QueryStatistics is documented as not thread-safe (correct for the single-query-thread assumption). Worth confirming no write path dispatches mutation steps onto the async/parallel pools - if a future parallel write ever shares one CommandContext, the plain-int increments would race. The class Javadoc naming that invariant would help the next reader.

6. containsUpdates() including index/constraint counters correctly maps to Neo4j SummaryCounters.containsUpdates(); flagging it as intentional since Neo4j also has a separate containsSystemUpdates() that this deliberately does not model (noted in the design doc non-goals - good).

Tests

Coverage is strong: unit (QueryStatisticsTest, ResultSetStatisticsTest), engine integration (CypherQueryStatisticsTest covering CRUD/DDL/merge/replace-map/absent-property/dedup), and a real-driver Bolt IT including a read-reports-no-updates regression. The main gaps are the two edge cases in point 1 and constraint-kind coverage (already tracked in issue 5015).

Net: solid to merge once the null-inline-property counting behavior is confirmed (point 1) and ideally the UNION-write conformance question (point 2) is resolved. Points 3-6 are follow-up-worthy but not blockers.

Generated with Claude Code

[robfrank]

Second-pass responses:

1. Null-inline property on CREATE. Verified this is already Neo4j-correct: CreateStep.setProperties skips null values (// In Cypher, null property values are not stored; if (value != null) document.set(...)), so CREATE (n:Foo {a:1, b:null}) stores only a and getPropertyNames().size() == 1. Added a lock-down test asserting propertiesSet == 1 for that shape (fdb6940). The auto-populated/default-property case does not arise for Cypher-created types (Cypher auto-creates types with no declared defaults); the coupling is documented at the create sites.

2. UNION / CALL conformance hole. Confirmed no conformance scenario in spec.yaml exercises a UNION or CALL write, so flipping RESULT-004 to passing does not certify a shape that misreports. On the "zero-but-wrong vs not-emitting" point: the current behavior is not emitting a stats map at all for these shapes (the guard is containsUpdates(), which is false when the outer context has no counts), so the driver reports containsUpdates() == false with no counters - there is no zero-valued map being sent. #5015 tracks propagating the real counts.

3. Parent-context delegation in getStatistics(). Good lead - noted in #5015 as a candidate mechanism. It would help the CALL {} case if executeWithSeedRow sets the outer context as parent, but UNION branches run as independent execute() calls with fresh root contexts, so they would still need explicit aggregation. Captured both in the follow-up rather than half-solving here.

4. Replace-map rule duplicated. After the += fix in 46fef7d the !prop.startsWith("@") && map.get(prop) == null idiom remains in the two REPLACE_MAP branches (SetStep.applyReplaceMap, MergeStep); single-sourcing it into a small helper is a clean maintainability win. Added to #5015 to avoid a refactor mid-review-cycle.

5. Thread-safety Javadoc. Added in 46fef7d: "Not thread-safe: a single command context is written by one query thread at a time; counters are plain ints with no synchronization." Confirmed no write path dispatches mutation steps onto the async/parallel pools.

6. containsUpdates() incl. index/constraint. Intentional, matching Neo4j SummaryCounters.containsUpdates(); containsSystemUpdates() is deliberately not modeled (design non-goal).

[claude]

Review: Bolt ResultSummary write counters (issue 5000)

Reviewed the engine accumulator, the five mutation steps, the DDL path, the Bolt consumption layer, and the tests. Clean, well-scoped, and unusually well-tested. Threading a lazily-allocated QueryStatistics through the already-shared CommandContext and surfacing it via a default method on ResultSet (mirroring getExecutionPlan()) keeps the blast radius near zero and the read hot-path allocation-free. Nice work.

Strengths

• Allocation discipline matches the CLAUDE.md mantra: primitive int counters, lazy getStatistics() so pure MATCH/RETURN never allocates, plus a regression test locking that in.
• Neo4j semantics carefully handled: distinct-label dedup, SET n = {map} replace counting both writes and removals, absent-property removal counted as 0, and IF [NOT] EXISTS DDL no-ops guarded against over-counting. The commit history shows these were found and fixed iteratively.
• The default method on ResultSet is the right call: zero impact on the many implementors, only the Cypher paths override.
• Strong coverage: 21 engine unit cases plus a real neo4j-java-driver IT, and the conformance flip mirrored across all five e2e suites.

Observations / minor concerns

1. Possible relationship undercount on DETACH DELETE (please verify). In the DeleteStep deferred-batch path (~line 252), entry.detach() triggers v.delete() + stats.incNodesDeleted(), but edges removed by the vertex-delete cascade are not counted there. Neo4j DETACH DELETE reports relationshipsDeleted for those cascaded edges. If this path (rather than deleteVertexAndEdges, which does count edges) is what runs for DETACH DELETE on a connected node, the counter reports relationships-deleted: 0. Worth confirming which path executes and, either way, adding a DETACH DELETE test asserting relationshipsDeleted: the one deletion shape CypherQueryStatisticsTest does not cover.

2. SET vs MERGE ON MATCH SET diverge on unchanged-value writes. SetStep.applyPropertySet always counts +1 for a non-null assignment, but MergeStep PROPERTY branch is wrapped in a valuesEqual(...) guard (an MVCC version-bump optimization), so MERGE (n) ON MATCH SET n.p = same-value reports properties-set: 0. Neo4j counts the assignment regardless of whether the value changed, so the two paths now report differently for equivalent writes. Pre-existing behavioral difference the counters inherit, not a blocker, but worth a code comment noting the intentional divergence or aligning the two.

3. handleDiscard reads getStatistics() before draining the result set. Correct today because writes are fully materialized in CypherExecutionPlan.execute() before the ResultSet is returned (and the Optional wraps the live mutable instance). Slightly fragile if a lazy-write path is ever added; a one-line comment reaffirming the writes-materialized-eagerly invariant at that read site would help.

4. Docs footprint: the PR adds ~1000 lines of planning/spec docs under docs/superpowers/. Fine if intended as durable design records; flagging in case they were meant to stay session-local.

Nits

• BoltResultStats.putIfNonZero widening int to long is correct for the packstream integer encoding drivers expect.
• No stray System.out, license headers present on new files, final used consistently, one-child if style respected.

Overall: solid, closes a real conformance gap with genuine end-to-end verification. My only substantive ask is item 1 (verify/cover the DETACH DELETE relationship count); the rest are minor.

[robfrank]

Third-pass responses:

1. DETACH DELETE relationship undercount - confirmed and fixed (0333322). You are right: the immediate path (deleteVertex -> deleteAllEdges) counts cascaded edges, but the deferred/FOREACH path (flushDeferredDeletes, used when DEFERRED_DELETE_BATCH_VAR is set) called v.delete() without counting them, so FOREACH (x IN xs | DETACH DELETE x) on a connected node reported relationshipsDeleted: 0. Fixed by counting each connected edge via the existing deleteObjectStatic before v.delete() in the detach branch - which also respects the deleted set, so an edge explicitly deleted earlier in the same batch is not double-counted (closing the double-count concern from the first pass in the same spot). Added two tests: plain MATCH ... DETACH DELETE (immediate path, was already correct) and FOREACH ... DETACH DELETE (deferred path, was red at relationshipsDeleted 0, now 1).

2. SET vs MERGE ON MATCH SET on unchanged values - aligned (0333322). The MergeStep PROPERTY branch counted inside the valuesEqual MVCC guard, so MERGE (n) ON MATCH SET n.p = <same> reported properties-set: 0 while SetStep counts unconditionally. Neo4j counts the assignment regardless of whether the value changed, so I moved the addPropertiesSet(1) out of the guard - the counter now matches Neo4j and SetStep, while the write-skip optimization (and its ConcurrentModificationException avoidance) is preserved. Added a regression test.

3. handleDiscard read-before-drain. Added a one-line comment at the read site reaffirming the invariant: statistics are computed eagerly when the write is materialized in the query plan, so they are valid to read before draining.

4. Docs footprint. Intentional and per repo convention (spec/plan docs are committed with feature PRs here).

CypherQueryStatisticsTest now 25/25; Delete/Foreach/Merge/Set regression 121/121 green.

[claude]

Code Review: PR #5016 - Bolt ResultSummary counters

Reviewed the engine accumulator, the Cypher mutation/DDL counting, the ResultSet/CommandContext plumbing, the Bolt emission, and the tests. Overall this is a clean, well-scoped, and well-tested change. The lazy-allocation design keeps read queries allocation-free, the ResultSet.getStatistics() default mirrors getExecutionPlan() with zero blast radius, and the Neo4j "emit only non-zero + contains-updates" semantics are faithful. Nice work, and thanks for the extensive tests and the in-code documentation of the known gaps.

A few things worth a look:

1. Correctness - counters can double-count on a transaction retry (CreateStep, likely MergeStep)

In CreateStep, the counter increments happen inside the database.transaction(() -> { ... }, true) block (createVertex -> stats.incNodesCreated(), createEdge -> stats.incRelationshipsCreated(), etc.). LocalDatabase.transaction(txBlock, joinCurrentTx) retries the block up to TX_RETRIES times on NeedRetryException/DuplicatedKeyException (LocalDatabase.java:1297-1327). The QueryStatistics lives on the CommandContext and is not reset between retries, so a standalone CREATE (no active outer transaction) that hits an MVCC conflict or duplicate-key retry will report inflated nodes-created/relationships-created/properties-set.

This is narrow (only when the CREATE opens its own tx and actually retries), but it produces wrong counters exactly in the contended case. SetStep/DeleteStep use a manual begin()/commit() with no retry loop, so they aren't affected; MergeStep.createVertex/createEdge increment the same way and should be checked. Consider counting outside the retried block, or snapshotting/resetting the relevant counters on retry.

2. CALL { } and UNION write subqueries report zero (documented, but worth confirming the direction)

BasicCommandContext.getStatistics() lazily allocates on this, while getDatabase() delegates to the parent. A write executed under a child context (CALL subquery / UNION branch) increments the child's accumulator, but CypherExecutionPlan.execute() reads the root context, so those counts are lost. The PR explicitly documents this and tracks it in #5015, so it's a conscious scope decision - just flagging that having getStatistics() walk to the root (like getDatabase() does) would be the natural fix and might be cheaper than a separate follow-up.

3. Minor - constraint/index "existed before" edge cases

In OpenCypherQueryEngine.executeCreateConstraint (UNIQUE/KEY) and executeCreateIndex, existedBefore is derived from indexExistsOnProperties(...). If a plain (non-unique) index already covers those properties and you then add a UNIQUE/KEY constraint on the same properties, existedBefore is true and constraints-added won't increment even though a constraint was genuinely added. Likely rare in practice, but the counter semantics differ from Neo4j there. Constraint-kind coverage is already noted as a follow-up in #5015.

Performance

Good. Primitive int counters, lazy allocation on write-only, no new hot-path branch for reads, and the stats map is only built when containsUpdates() is true. No concerns.

Security

No new surface. No sensitive data logged. Fine.

Test coverage

Strong - 21 engine unit cases plus the real-driver Bolt5000ResultCountersIT covering write/read/DISCARD paths, and the read-allocates-nothing regression is a nice touch. Two gaps that map to the risks above: (a) no test exercising a CREATE that retries (item 1), and (b) no CALL/UNION write-counter assertion (item 2, though it's deferred).

Nit

The two docs/superpowers/{plans,specs}/*.md design artifacts (~700 lines) are committed to the repo - confirm that's intended versus being a working artifact.

Nothing here is a blocker except item 1, which is a real (if narrow) correctness bug worth addressing before merge.

[robfrank]

Fourth-pass responses:

1. Retry double-count in CreateStep - confirmed and fixed (7886b29). Verified: CreateStep.createPatterns/createPatternsBatch wrap creation in database.transaction(block, true), which retries on MVCC/duplicate-key conflict, and the counter increments run inside the block, so a standalone CREATE that actually retries inflated the counts. Fixed by snapshotting the counters before the transaction (QueryStatistics.copy()) and restoring them at the top of the transaction lambda (stats.restore(snapshot)), so each re-attempt starts from the pre-attempt baseline and only the successful attempt\s deltas survive. Also clears the batch createdResultslist at the top of each attempt (a retry would otherwise duplicate result rows). Checked the other steps as you suggested:MergeStep, SetStep, and DeleteStepall use manualbegin()/commit()with no retry loop, so they are not affected - no change there. Added acopy()/restore()unit test; deterministically forcing adatabase.transaction` retry needs a concurrent MVCC/duplicate-key conflict (impractical as a stable unit test), so the mechanism is covered by that test plus the reset-at-attempt-start placement.

2. CALL / UNION zero counters + parent-walk suggestion. Deferred to #5015, where I already captured exactly your suggestion: have BasicCommandContext.getStatistics() walk to the root context (as getDatabase() does). Noted there that it closes CALL if the child sets the outer as parent, but UNION branches run as independent top-level execute() calls and still need per-branch aggregation.

3. Constraint "existed before" edge case (plain index present, then UNIQUE/KEY added -> constraints-added not incremented). Real edge case in the counter semantics; added it to #5015 alongside the NOT_NULL/KEY/TYPED coverage gap.

Nit (docs in-tree). Intentional, per repo convention for feature PRs.

QueryStatisticsTest 4/4 (incl. the new copy/restore case), CypherQueryStatisticsTest 25/25, Create/Merge regression green.

[claude]

Review: Bolt ResultSummary write counters (#5016)

Overall this is a well-designed, carefully-scoped change. The engine-side accumulator is allocation-light and lazily created, the Bolt surfacing is clean, and the Neo4j-fidelity semantics have clearly been thought through (label dedup, absent-property removals not counted, null inline properties skipped, retry-safe counters). Documentation and test coverage are excellent. Comments below, mostly minor.

Strengths

• Zero blast radius surfacing. ResultSet.getStatistics() as a default returning Optional.empty() mirrors getExecutionPlan() and leaves all other ResultSet implementations untouched.
• No read-path cost. CommandContext.getStatistics() is lazily allocated, so pure MATCH/RETURN never creates a QueryStatistics and adds no hot-path branch. The readQueryReportsNoUpdates IT locks this in.
• Retry correctness. The copy()/restore() snapshot around CreateStep's database.transaction(..., true) retry block is a subtle but real bug that was caught and fixed - a retried standalone CREATE would otherwise inflate counts. The comment explains why MergeStep/SetStep/DeleteStep (manual begin/commit, no retry) are unaffected.
• Neo4j semantic fidelity. Distinct-label counting, SET n.absent = null / REMOVE n.missing counting 0, SET n += {k:null} only counting when the key existed, and SET n = {map} replace-removal counting all match Neo4j and are individually tested. The 21 unit cases + real neo4j-java-driver IT (including the DISCARD-after-write path) are strong.
• Honest scope. UNION-branch and CALL {}-subquery non-aggregation is documented in-code and tracked in follow-up Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015 rather than silently returning wrong numbers.

Questions / possible issues

1. Self-loop under non-deferred DETACH DELETE may double-count (or throw). In DeleteStep.deleteAllEdges (the non-FOREACH path, ~lines 500-522) edges are collected from DIRECTION.OUT then DIRECTION.IN into one list with no dedup and no deleted set, then each is edge.delete() + incRelationshipsDeleted(). For a self-loop MATCH (a)-[r]->(a) DETACH DELETE a, r is collected from both OUT and IN, so it appears twice. Depending on whether edge.delete() on an already-removed edge is idempotent or throws RecordNotFoundException, this either reports relationships-deleted: 2 for one relationship, or fails the statement (there is no try/catch here, unlike the deferred flushDeferredDeletes path which dedups via a deleted set + deleteObjectStatic). Worth a targeted test for DETACH DELETE of a self-loop and of two endpoints sharing an edge. If the pre-existing behavior is already correct the counter just inherits it - but the counter makes the double-collection observable.

2. CommandContext.getStatistics() has no read-only counterpart. Every caller that only wants to read the accumulator (e.g. CypherExecutionPlan.execute()) goes through getStatistics(), which allocates if absent. Fine in the write path (always populated by then), but a small footgun for any future read-side caller - a peek-only accessor returning null/Optional would avoid an accidental allocation on a read query. Minor.

3. QueryStatistics lives in com.arcadedb.query.sql.executor but is produced/consumed almost entirely by the OpenCypher engine and Bolt. Reasonable shared home (same package as CommandContext), just flagging in case a more neutral package was intended.

Nits

• BoltResultStats.toStatsMap unconditionally puts contains-updates: true; correct only because the two call sites already guard on containsUpdates(). A one-line comment at the method (not just the call sites) would prevent a future caller from emitting contains-updates: true with an all-zero map.
• The two Bolt call sites read getStatistics() at slightly different points relative to draining (handlePull after the drain, handleDiscard before it). Safe today because writes are materialized eagerly in CypherExecutionPlan.execute(), and handleDiscard documents it - but it couples the Bolt layer to that eager-materialization invariant. If a lazy write path is ever introduced, handleDiscard would read an empty Optional. A shared helper reading stats after close would be more robust.

Verification

Read-only review of the diff and surrounding code; I did not run the build/tests. Per CLAUDE.md guidance to compile and run affected tests after backend changes, please confirm the engine com.arcadedb.query.opencypher package tests, QueryStatisticsTest/ResultSetStatisticsTest/CypherQueryStatisticsTest, and Bolt5000ResultCountersIT are green in CI - the PR description says they are.

Nice work overall; the fidelity detail here is impressive.

Reviewed by Claude Code.

[robfrank]

Fifth-pass responses:

1. Self-loop DETACH DELETE double-count - confirmed and fixed (8b26b0b). Good catch. deleteAllEdges (the immediate path) collected a self-loop from both OUT and IN with no dedup, and a new test reproduced it: MATCH (a:SL) DETACH DELETE a on (a)-[:LOOP]->(a) reported relationshipsDeleted: 2. Fixed by de-duplicating collected edges by identity (a Set<RID>) so a self-loop is counted once, and wrapping edge.delete() in a RecordNotFoundException guard so an already-removed edge (e.g. a shared edge deleted when the other endpoint was detached) is never counted twice or throws - matching the robustness of the deferred path. Test now asserts relationshipsDeleted: 1.

Nit (contains-updates). Added a note at BoltResultStats.toStatsMap that contains-updates is emitted unconditionally, so callers must only invoke it when containsUpdates() is true (8b26b0b).

2. Read-only getStatistics() accessor. No current read-side caller allocates through it (execute() only calls it on the write branch), so this is a future footgun rather than a live issue; adding a peek-only accessor now would be speculative. Noted for the #5015 follow-up if a read-side consumer (HTTP/gRPC) needs it.

Nit (Bolt read-after-close helper) + item 3 (QueryStatistics package). The handleDiscard read site documents the eager-materialization invariant; a shared read-after-close helper is a reasonable robustness refactor but only matters if a lazy write path is ever introduced (none exists), so I have left it. QueryStatistics sits with CommandContext/ResultSet in com.arcadedb.query.sql.executor, which is the shared executor home both the Cypher engine and Bolt already depend on - intentional.

CypherQueryStatisticsTest 26/26, OpenCypherDeleteTest 9/9 green.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 54 complexity

Metric	Results
Complexity	54

View in Codacy

🟢 Coverage 92.97% diff coverage

Metric	Results
Coverage variation	Report missing for 5ec2aa61
Diff coverage	✅ 92.97% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (5ec2aa6)	Report Missing	Report Missing	Report Missing
Head commit (9976879)	166590	111958	67.21%

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 (#5016)	256	238	92.97%

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%

¹ Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

_{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: Bolt ResultSummary counters (PR 5016)

Reviewed the full diff. This is a well-structured, carefully thought-out change. The design (a lazy, primitive-int accumulator on CommandContext, surfaced through a zero-blast-radius default method on ResultSet, consumed by the Bolt layer) is clean and matches the repo performance-first conventions. The inline comments documenting Neo4j semantics on each counting site are excellent and make the diff easy to audit. Test coverage is strong: engine unit tests, a real-driver Bolt IT, e2e across five languages, and the conformance spec flip.

Findings ranked by importance.

1. Latent fragility: getStatistics() does not follow the parent chain, and copy() drops it (medium)

BasicCommandContext.getStatistics() lazily allocates a per-context accumulator and, unlike getVariable() (which walks up to the parent), does NOT delegate to a parent context. In addition, BasicCommandContext.copy() (line ~401) copies variables, parent, child, profiling, etc. but NOT statistics.

Today this is safe because every mutation step in a single plan runs against the same context instance passed to syncPull, and the top-level read in CypherExecutionPlan.execute() reads that same instance - the tests confirm it. But it is a silent trap: the moment any future step runs against a child or a context.copy() (a common pattern elsewhere in the executor), its increments land on a different accumulator and vanish from the reported totals, with no error. This is exactly the mechanism behind the documented UNION / CALL subquery zero-counting limitation.

Suggestion: have getStatistics() resolve to the root context accumulator (mirroring how getVariable walks parent), and copy the reference in copy(). That hardens the invariant and would let UNION/CALL sub-plans that share a parent aggregate for free. (Standalone UNION sub-plans build brand-new BasicCommandContext with no parent, so they would still need the follow-up in issue 5015 - but the general case gets safer.)

2. Retry double-count protection is only in CreateStep (low/question)

CreateStep correctly snapshots/restores the accumulator inside its database.transaction() retry block so a retried attempt does not double-count - nicely done, including the createdResults.clear(). However MergeStep also creates nodes/edges and increments the same counters, but does not use a database.transaction() retry wrapper and has no snapshot/restore. Could you confirm there is no path where a MERGE that creates entities can have its create block re-executed (ambient-tx conflict/retry) and thus over-count? If the ambient transaction can retry the step body, MERGE would need the same snapshot/restore guard as CREATE.

3. Test coverage gaps (low)

• No regression test exercises the retry/restore path in CreateStep (the double-count-on-conflict scenario the snapshot logic exists to prevent). Hard to trigger deterministically, but even a unit test that calls restore(copy()) then re-increments would lock the contract.
• Constraint-kind counting (NOT NULL / KEY / TYPED into constraints-added) has substantial new branching in OpenCypherQueryEngine (the typedChanged/changed/existedBefore guards for idempotent IF NOT EXISTS) but I do not see a Cypher test asserting those counters. Issue 5015 tracks this - worth calling out since the no-op-guard logic is the subtle part and is currently unverified.

Things done well

• putIfNonZero emitting only non-zero counters plus contains-updates, casting to long, and the guard comment that callers must check containsUpdates() first - matches Neo4j server output faithfully.
• Reading stats only on the terminal (not-hasMore) SUCCESS in both handlePull and handleDiscard, and only for result sets that actually carry an accumulator - reads emit nothing.
• The DELETE de-dup work: self-loop dedup via a seen RID set in deleteAllEdges, and DETACH counting connected edges through the shared deleted set to avoid double-counting. Careful.
• Neo4j-accurate edge cases: properties-set counting the genuine removal of an existing property and the SET n = {map} replace counting removed-and-rewritten props; distinct-label dedup for labels-added; unchanged-value assignment still counting (MERGE PROPERTY branch).
• The default method on ResultSet keeps every other implementation untouched.

Overall: solid, mergeable work. The main thing I would like addressed before merge is a short answer/guard on point 2 (MERGE retry) and ideally hardening point 1 so the accumulator cannot silently disappear across a context boundary later.

[robfrank]

Sixth-pass responses:

1. copy() dropped statistics - fixed (latest commit). BasicCommandContext.copy() propagated variables/parent/child/profiling but not the accumulator, so a step running against a context.copy() would have its increments silently vanish. Fixed by sharing the same QueryStatistics reference in copy(), so mutations through a copied context aggregate into one place. Added a unit test asserting copy().getStatistics() is the same instance and increments through the copy land on the original total. The broader hardening you suggested - getStatistics() walking to the root parent (mirroring getVariable) - is the natural mechanism to close the UNION/CALL gap and is tracked in #5015; I kept that out of this PR since it changes UNION/CALL behavior, but the copy() fix removes the silent-vanish trap now.

2. MERGE retry over-count - confirmed safe, no guard needed. Verified: MergeStep opens its transaction with manual begin()/commit() (no database.transaction(..., true) retry wrapper and no retry loop), and neither OpenCypherQueryEngine nor CypherExecutionPlan wraps step execution in a retrying transaction - only CreateStep uses database.transaction(..., true), which is why only it carries the snapshot/restore guard. A MERGE create block is never re-executed by a retry, so it cannot over-count; a mid-commit conflict fails the statement rather than re-running the body.

3a. Retry/restore test. Already present as QueryStatisticsTest.copyAndRestoreRollBackIncrements (added with the CreateStep retry fix): it snapshots, re-increments, restores, and asserts the counters roll back - exactly the contract you describe.

3b. Constraint-kind counting tests (NOT NULL / KEY / TYPED, and the no-op IF NOT EXISTS guards). Tracked in #5015 alongside the plain-index-then-constraint existedBefore edge case.

QueryStatisticsTest 5/5 green.

[claude]

Review: Bolt ResultSummary counters for write queries (#5016)

Reviewed the engine accumulator, the five Cypher mutation steps, the DDL path, and the Bolt consumption layer. This is a clean, well-scoped implementation with genuinely good attention to Neo4j semantics (no-op removals not counted, distinct-label dedup, replace-map removed-property counting, retry-safe snapshots). The lazy allocation keeps the read hot-path allocation-free, and the tests cover the important cases. Nice work.

A few observations, roughly in order of importance.

Correctness

1. BasicCommandContext.copy() only shares statistics when already allocated (BasicCommandContext.java:414).
copy.statistics = statistics copies the reference, but if statistics is still null at copy time (the normal case, since allocation is lazy and deferred to the first write), the copied context and the parent will each lazily allocate separate QueryStatistics instances on their first getStatistics() call. Writes performed through one context would then not aggregate into the other, contradicting the comment right above it ("Share the same statistics accumulator ... instead of silently vanishing"). It is not hit by the current Bolt top-level path (I confirmed no context.copy() calls exist in the Cypher executor steps), so this is latent rather than active - but if a future write path routes through a copied context it will silently undercount. Consider forcing shared allocation in copy() (accepting it allocates for copied read contexts too), or a shared holder so both contexts see the same lazily-created instance.

2. MERGE path-create collision may leave stale counts (MergeStep.java:315-322, createNewPath 875-912).
mergePathAll catches DuplicatedKeyException from createNewPath and re-runs the match, but unlike CreateStep (which snapshots via stats.copy() / stats.restore() on each database.transaction retry) MergeStep uses manual begin()/commit() with no snapshot. For a multi-node path where the first vertex saves successfully and a later node/edge collides, that first vertex's incNodesCreated() / addPropertiesSet() increments have already fired and are not rolled back before the retry returns the matched path. (The single-node case at mergeSingleNodeAll is safe because createVertex increments after vertex.save(), so a colliding save never counts.) Edge case tied to concurrent/batch unique-index collisions, but worth a snapshot/restore for parity with CreateStep.

Performance

3. Per-record stream().distinct().count() for label counting (CreateStep.java:405, MergeStep.java:1170).
nodePattern.getLabels().stream().distinct().count() allocates a stream pipeline per created vertex. On the batched bulk-CREATE path this runs once per row and adds avoidable GC pressure, which CLAUDE.md explicitly calls out as a priority. Label lists are tiny, so a manual dedup count (or, since Labels.ensureCompositeType already dedups, deriving the count from the resolved composite type) would be lighter. Same applies to vertex.getPropertyNames().size() / edge.getPropertyNames().size() allocating a collection per record - minor, but on the hot bulk path it adds up.

Minor

4. InternalResultSet.copy() (InternalResultSet.java:117) does not carry statistics over. Not exercised by the Bolt path today, but if a copied result set is ever returned to a stats consumer the counters silently vanish. A one-line copy.statistics = this.statistics would future-proof it.

5. propertiesSet via getPropertyNames().size() after save will include any schema-default/auto-populated properties, which could diverge from Neo4j's "properties the statement assigned" for types that declare defaults. Likely acceptable for the certification scope, but worth noting in the follow-up (#5015).

Tests

Coverage is solid: QueryStatisticsTest, ResultSetStatisticsTest, CypherQueryStatisticsTest (21 cases incl. the read-allocates-nothing regression), plus Bolt5000ResultCountersIT driving the real neo4j-java-driver for PULL, DISCARD, and read paths. The documented follow-ups in #5015 are the right calls to defer.

Overall: approve-worthy pending a look at items 1 and 2, which are the only two with real (if latent/edge-case) correctness impact.

Automated review - please verify findings against the code before acting on them.

[claude]

Review: Populate Bolt ResultSummary counters for write queries (#5016)

Nice, well-structured piece of work. The layering is clean: an engine-neutral QueryStatistics accumulator on CommandContext, surfaced through a zero-blast-radius ResultSet.getStatistics() default method, and consumed by the Bolt layer via a small BoltResultStats mapper. Lazy allocation keeps read queries allocation-free, the retry/snapshot handling in CreateStep shows real care, and the DELETE de-dup + DETACH edge-counting logic is thought through. Test coverage (21 engine unit cases + a real-driver Bolt IT + the 5 e2e suites) is strong. Below are findings, ordered roughly by importance.

Potential correctness issues

1. propertiesSet on CREATE is derived from the final record, not the assignments made (CreateStep.java:407, MergeStep.java:1171)

stats.addPropertiesSet(vertex.getPropertyNames().size());

Neo4j's properties-set counts the property writes performed by the statement. Reading getPropertyNames().size() after save() instead counts whatever ends up on the record. These diverge if the schema type declares a property with a default value (or otherwise auto-populates one): a CREATE (:T {a:1}) on a type where T has a defaulted property b would report properties-set: 2. For today's Cypher-created types this is usually harmless (they carry no schema defaults), but it is a latent semantic gap. Counting the number of properties the step actually assigns (the size of the evaluated, non-null property map you already build) would be exact and slightly cheaper. Same pattern for edges at CreateStep.java:454 / MergeStep.java:1213.

2. MergeStep can over-count when a path create partially succeeds then falls back to match (MergeStep.java:307-323)
createNewPath creates path vertices/edges sequentially, incrementing the shared statistics as it goes. If a later createVertex() throws DuplicatedKeyException, mergePathAll catches it and re-runs findAllMatchingPaths, returning the matched path - but the counters from the already-created earlier vertices/edges in the failed attempt remain. Unlike CreateStep, MergeStep has no snapshot/restore guard around its mutation counting. This is the #4351 collision path, so it's narrow, but worth either applying the same copy()/restore() pattern used in CreateStep.createPatterns, or documenting the divergence.

Minor / nits

3. Mutation steps increment stats but don't restore them on their own rollback path. SetStep, RemoveStep, MergeStep, and the deferred-delete flow increment context.getStatistics() inside a manual begin()/commit() block whose catch rolls back the DB write but leaves the counters bumped. In practice the exception propagates and aborts the whole statement so the stats are never surfaced - so this is not an observable bug today - but it's an implicit invariant ("stats are only read on the success path") that would be good to note in a comment, since it's easy to break later.

4. Unrelated formatting churn in bindings/python/**. The changes to results.py, type_conversion.py, and build.sh are pure formatter reflows (line-wrapping, continuation-style) with no behavioral connection to Bolt counters. They're harmless but add noise to the diff and to git blame; consider splitting them out.

5. BoltResultStats.toStatsMap relies on an undocumented-at-callsite precondition. The method always emits contains-updates: true and comments that callers must only invoke it when containsUpdates() is true. Both call sites in BoltNetworkExecutor do guard correctly (stats.get().containsUpdates()), so this is fine - just flagging that the coupling is by convention, not enforced.

Things I checked that are correct

• Interface addition of CommandContext.getStatistics() - BasicCommandContext is the only implementor; no anonymous impls in tests, so no compile breakage.
• BasicCommandContext.copy() correctly shares the same accumulator reference so mutations through a copied context aggregate in one place.
• Read-query path in CypherExecutionPlan.execute() never calls getStatistics(), so no allocation/branch on the read hot path, as claimed.
• DDL IF [NOT] EXISTS no-op guarding (indexExistsOnProperties, the typedChanged/changed flags) correctly avoids over-counting.
• DELETE de-duplication across rows and DETACH-DELETE shared-edge counting (via the caught RecordNotFoundException) yield Neo4j-correct totals.
• CALL-subquery and UNION-branch aggregation gaps are documented in-code and tracked in the follow-up (Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015).

Overall this looks solid. #1 is the one I'd most want addressed since it changes a headline counter's semantics; #2 is a narrow edge case. Did you verify the CREATE property count against a Cypher type that has a schema-declared property with a default value? That's the case that would expose finding #1.

Automated review by Claude Code.

[claude]

Review: Bolt ResultSummary write counters (#5016)

Reviewed the engine accumulator, the five Cypher mutation steps, the DDL path, and the Bolt consumption layer. This is a careful, well-scoped piece of work: the counter semantics are matched to Neo4j edge-by-edge, the commit history shows the tricky cases (dedup of labels, absent-property removals, += null entries, self-loop double-count, DETACH cascades, transaction-retry double-count) were each found and fixed with a locking test, and the design is genuinely allocation-light for reads. Nice work.

A few observations, mostly minor:

Correctness / semantics

• CreateStep retry restore is sound, but allocates on every write, not just retries. createPatterns/createPatternsBatch call stats.copy() unconditionally at method entry to snapshot before the database.transaction(...) block. For a single-row createPatterns that is one extra small-object allocation per created row, which slightly cuts against the "allocation-light" framing. Since MVCC retries are rare, consider snapshotting lazily (only on the first attempt inside the lambda, or with plain int locals) if this shows up on a CREATE-heavy profile. Not blocking.
• propertiesSet for CREATE relies on getPropertyNames().size(). The inline-null case is correctly handled (verified by the CREATE (:Foo {a:1, b:null}) == 1 test). Residual gap: if the target type has schema-declared default-valued properties, getPropertyNames() on the fresh record would include them and over-count vs. Neo4j (which counts only user-supplied properties). Narrow for typical Cypher usage, but worth a comment or follow-up note.

Scope gaps worth surfacing to users

• UNION / CALL {} write subqueries report zero counters. Documented in-code and tracked in Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015, but it is a user-visible fidelity gap: a MERGE/CREATE inside a CALL { ... } subquery returns a summary with no stats over Bolt even though writes happened. Given the epic is compatibility certification, consider making that limitation visible in the conformance spec (e.g. a documented expected-fail scenario) rather than only an in-code comment, so it is not silently read as "counters supported."
• SQL engine does not populate QueryStatistics. The class lives in query.sql.executor but is only wired by the OpenCypher steps. Fine for this PR's Bolt scope, but anyone calling ResultSet.getStatistics() after a SQL INSERT/UPDATE/CREATE VERTEX gets an empty Optional. A one-line Javadoc on the default method ("populated by the OpenCypher engine only") would prevent a future surprise.

Thread-safety

• QueryStatistics is plain-int and documented single-threaded-per-context, correct today. Flagging for the future: if engine query parallelism (the QueryEngineManager pool) is ever extended to fork mutation-step work, these unsynchronized counters would race and lose increments. The Javadoc contract is the right guard for now.

Minor / style

• BoltResultStats.toStatsMap emits contains-updates: true unconditionally and relies on the two call sites in BoltNetworkExecutor to gate on containsUpdates(). The comment documents this, but since the class is tiny it would be a touch more robust to short-circuit inside toStatsMap itself rather than depend on caller discipline. Optional.
• Nice touch de-duplicating self-loop edges via a RID seen set in deleteAllEdges, and preserving the pre-attempt baseline (not zero) in restore so earlier rows' counts survive a later row's retry. Both are easy to get wrong.

Tests

Coverage is strong: 21 engine unit cases across CREATE/SET/DELETE/MERGE/REMOVE plus label/DDL/replace-map/absent-property paths, a real neo4j-java-driver IT (Bolt5000ResultCountersIT), and the flip of RESULT-004 unwrapped across all five language e2e suites. The read-query-allocates-nothing negative case is covered too. Main untested surface is the documented UNION/CALL gap and constraint-kind DDL variety, both tracked in #5015.

Overall: solid, well-tested, and safe from a correctness standpoint. The items above are refinements and follow-up documentation rather than blockers.

Automated review by Claude. I traced the counting paths in the diff but did not run the full Maven build locally, so I rely on the PR's stated green test runs for compilation/CI.

[codecov]

Codecov Report

❌ Patch coverage is 85.93750% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.43%. Comparing base (86a7562) to head (9976879).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
.../query/opencypher/query/OpenCypherQueryEngine.java	76.19%	3 Missing and 7 partials ⚠️
...db/query/opencypher/executor/steps/DeleteStep.java	75.00%	7 Missing and 2 partials ⚠️
...in/java/com/arcadedb/bolt/BoltNetworkExecutor.java	28.57%	4 Missing and 1 partial ⚠️
...edb/query/opencypher/executor/steps/MergeStep.java	84.84%	1 Missing and 4 partials ⚠️
...db/query/opencypher/executor/steps/CreateStep.java	80.00%	3 Missing ⚠️
...adedb/query/opencypher/executor/steps/SetStep.java	92.59%	0 Missing and 2 partials ⚠️
...query/opencypher/executor/CypherExecutionPlan.java	87.50%	0 Missing and 1 partial ⚠️
...m/arcadedb/query/sql/executor/QueryStatistics.java	97.95%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5016      +/-   ##
============================================
+ Coverage     65.40%   65.43%   +0.02%     
+ Complexity      824      823       -1     
============================================
  Files          1685     1687       +2     
  Lines        134582   134787     +205     
  Branches      28797    28825      +28     
============================================
+ Hits          88026    88200     +174     
- Misses        34493    34509      +16     
- Partials      12063    12078      +15     

☔ View full report in Codecov by Harness.
📢 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.

[claude]

PR Review: Bolt ResultSummary counters for write queries (#5016)

Reviewed the engine accumulator, the Cypher mutation/DDL wiring, and the Bolt consumption layer. This is a well-engineered, carefully-documented change. The design is thoughtful: lazy allocation so read queries pay nothing, primitive int counters, and Neo4j-faithful semantics down to edge cases. Feedback below, ordered by importance.

👍 Strengths

• Allocation-light / zero read-path cost: CommandContext.getStatistics() lazily allocates, and CypherExecutionPlan.execute() only touches it on the write path (hasWriteOps). The read-query-allocates-nothing regression test (CypherQueryStatisticsTest asserting getStatistics() is empty for MATCH) locks this in. Matches the CLAUDE.md performance mantra.
• MVCC-retry correctness: CreateStep wraps mutations in database.transaction() (auto-retry) and correctly snapshots (copy()) / restore()s the counters at the top of each attempt, so a retried transaction never double-counts. A subtle case that's easy to miss and was handled correctly.
• Neo4j-accurate counting: the SET n = {map} replace path (SetStep.applyReplaceMap) counts both newly-written properties and pre-existing properties removed by the replace; absent-property removal counts 0 (RemoveStep/applyPropertySet). DETACH DELETE de-dups self-loops via a seen RID set. All match Neo4j and are backed by tests.
• Clean layering: ResultSet.getStatistics() as a default method mirroring getExecutionPlan() keeps blast radius near zero, and BoltResultStats guards contains-updates behind the caller's containsUpdates() check with a clear contract comment.
• Strong test coverage: 21 engine cases + a real neo4j-java-driver IT (Bolt5000ResultCountersIT), plus flipping RESULT-004 and unwrapping the expected-fail harness across all five e2e language suites.

🔧 Suggestions / concerns

1. Unrelated formatting churn bundled in (recommend splitting out). The entire bindings/python/** set of changes (import reordering in core.py/test_vector_sql.py, line-wrapping in results.py/test_core.py, shfmt reflow in build.sh/build-native.sh) is pure auto-formatter output with no connection to Bolt counters. It inflates the diff and makes review harder. Ideally these belong in a separate chore(format) PR so this one stays scoped to the feature it describes.

2. Large process docs committed to the repo. docs/superpowers/plans/2026-07-06-bolt-result-counters.md (860 lines) and docs/superpowers/specs/...-design.md (167 lines) are added. If docs/superpowers/ is an intentional in-repo design-doc location, fine; if these are agent/process artifacts, consider whether they should live in the repo history long-term.

3. Transaction-model coupling is correct but fragile. QueryStatistics.restore() is needed only because CreateStep uses retrying database.transaction(). MergeStep, SetStep, and DeleteStep use direct begin()/commit() with no retry, so they intentionally don't snapshot. Correct today, but if any of those steps is later wrapped in database.transaction(...) retry it will silently double-count. A one-line comment at those begin() sites ("no retry here, so counters are not snapshot/restored") would protect the invariant.

4. Counters are non-transactional by design - worth an explicit note. Increments happen as each delete()/save() executes, not at commit. For a statement that throws mid-execution the accumulator is left inflated. Benign because Bolt then sends FAILURE (never a SUCCESS carrying stats), and it matches Neo4j's "counters reflect work regardless of later rollback" semantics - but it's an implicit assumption. A short comment on QueryStatistics ("not rolled back on statement failure; safe because failed statements don't surface stats") would help future readers.

5. Documented partial surface. UNION and CALL { ... } write subqueries run with their own BasicCommandContext/QueryStatistics and return zero to the caller; SQL /command (HTTP) and gRPC are not wired even though the ResultSet.getStatistics() plumbing now exists engine-wide. All clearly documented in-code and tracked in Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015 - just flagging that a driver issuing a top-level UNION write will see empty counters until Extend Cypher write-counter (QueryStatistics) surfacing: HTTP/gRPC + UNION/CALL aggregation #5015 lands.

Minor

• BoltNetworkExecutor.isWriteQuery(query) is a string-based heuristic for the type: "w"/"r" metadata, but stats emission itself is independent of it (driven by containsUpdates() on the result set), so a misclassification there won't drop counters. Good separation.
• CreateStep/MergeStep count stats.addPropertiesSet(vertex.getPropertyNames().size()) on the freshly-created record - correct since type/labels aren't stored as properties, and the comment says so.

Overall: solid, production-ready feature work. My only actionable ask is #1 (split the unrelated Python/format churn); the rest are small comments-for-clarity nits. Nice attention to the MVCC-retry double-counting trap in particular.

🤖 Generated with Claude Code

[claude]

Review: Bolt ResultSummary write counters (#5016)

Reviewed the engine accumulator, the five Cypher mutation steps, the DDL path, and the Bolt consumption layer against the repo conventions. Overall this is high-quality, carefully-thought-out work - the Neo4j-semantics handling (absent-property removals count 0, replace-map removals counted, distinct-label dedup, self-loop dedup in DETACH DELETE, transaction-retry counter snapshot/restore) is exactly the kind of edge-case coverage that usually gets missed, and the tests reflect it. Comments are excellent, and the allocation-light / lazy-allocation design respects the performance-first mantra (read queries never allocate a QueryStatistics).

A few observations, none blocking:

1. InternalResultSet.copy() silently drops the new statistics field (low). InternalResultSet.java:117-121 copies only content, not statistics (and, pre-existing, not plan). The DDL path returns an InternalResultSet carrying stats, so any future code path that calls copy() between the engine and the Bolt reader would lose the counters with no error. Not currently reachable in the Bolt path (verified - no copy() on the write/DDL result sets), so it is latent, but it is inconsistent with the new field and cheap to fix (copy plan and statistics too).

2. Defensive gate in BoltResultStats is correct but fragile by contract (nit). toStatsMap() unconditionally puts contains-updates:true and documents that callers must only invoke it when containsUpdates() is true. Both call sites in BoltNetworkExecutor (handlePull:720, handleDiscard:794) honor that. Just flagging the invariant lives in a comment rather than the type. Nice that stats emission is gated on containsUpdates() rather than the isWriteOperation classification, so a misclassification cannot suppress real counters.

3. Per-row QueryStatistics.copy() allocation in unbatched CREATE (very minor). CreateStep.createPatterns() allocates a snapshot copy per input row for retry-safety. With the default OPENCYPHER_BULK_CREATE_BATCH_SIZE this is one small (11-int) allocation per batch (negligible); only the unbatched legacy path pays it per row. Acceptable, mentioning for completeness.

4. Scope: unrelated Python-bindings reformatting bundled in. The diff includes bindings/python/** prettier/formatting-only changes and the e2e container-log-dump helper that are not part of the counters feature. Harmless, but separating pure-formatting churn from feature PRs makes bisecting and reviewing easier.

Correctness spot-checks that passed

• CREATE retry counter reset via restore(snapshot) at the top of the database.transaction(..., true) lambda is correct; createPatternsBatch also clears partial results on retry. MergeStep/SetStep/DeleteStep use manual begin/commit with no retry loop, so they correctly do not need it.
• CommandContext.copy() now shares the same accumulator instance (BasicCommandContext:414) so mutations through a copied context aggregate - good catch.
• SET n = {map} / n += {map} / property null-removal counting matches Neo4j (only count removals of properties that actually existed); consistent across SetStep and MergeStep.
• DETACH DELETE self-loop dedup via a RID seen-set, plus RecordNotFoundException guards preventing recount of already-deleted edges.
• DDL IF [NOT] EXISTS no-ops guarded by indexExistsOnProperties() so they do not over-count.

Tests. Coverage is strong: 21 engine unit cases, the real-driver Bolt5000ResultCountersIT (write, read-no-updates, DISCARD-path), and the RESULT-004 flip across all five language suites. The read-allocates-nothing regression is a good guard for the design intent.

Nice work overall - documenting the follow-ups (HTTP/gRPC surfacing, UNION/CALL write aggregation) in-code and tracking them in #5015 is the right call.

[claude]

Code Review: Bolt ResultSummary write counters (#5000)

Reviewed the engine accumulator, the five Cypher mutation steps, the DDL counting path, the Bolt wiring, and the test suite. This is a high-quality, carefully-scoped PR. The design (lazy QueryStatistics on CommandContext, default ResultSet.getStatistics() mirroring getExecutionPlan(), non-zero-only Neo4j hyphenated stats map) keeps the read hot-path allocation-free and the blast radius small. Neo4j semantics are handled with real care: distinct-label dedup, absent-property removal counting zero, replace-map pre-existing-property removals, DETACH self-loop dedup, and the retry-reset via copy()/restore(). Test coverage (30+ engine cases + a real neo4j-driver IT) is excellent.

A few observations, none blocking:

1. SET n.absent = null still writes on a no-op (minor)

SetStep.applyPropertySet (around SetStep.java:216-234) calls mutableDoc.remove(...) + save() unconditionally even when propertyExisted is false, whereas the parallel MergeStep.applySetClause PROPERTY branch guards with if (doc.has(property)) before mutating specifically to avoid a needless MVCC version bump (its own comment: "Removing an absent property is a no-op: skip to avoid bumping the MVCC version"). The counter is correct either way, but the two paths are inconsistent and the SetStep one can bump the version / risk ConcurrentModificationException for concurrent readers on a genuine no-op. Consider guarding the write the same way.

2. Shared mutable QueryStatistics across copied contexts + "not thread-safe" (latent footgun)

BasicCommandContext.copy() deliberately shares the same statistics reference (copy.statistics = statistics), while the class Javadoc documents it as single-threaded/no synchronization. That's fine today because mutation steps run single-threaded, but shared-mutable + not-thread-safe is exactly the combination that bites later if a copied context is ever handed to a parallel step. Worth an assertion or a stronger note at the copy() site pointing at the single-threaded contract.

3. UNION / CALL-subquery writes report zero to the caller (acknowledged)

Nicely documented in executeUnion() / executeWithSeedRow() and tracked in #5015. Flagging it for visibility: from a Bolt client's perspective CALL { CREATE ... } silently returning zero counters is a wrong-vs-Neo4j result, not just a missing feature, so it's worth prioritizing in the follow-up.

4. Duplicated counting logic (maintainability)

The REPLACE_MAP / MERGE_MAP "count written + count removed pre-existing" logic is near-identical between SetStep and MergeStep.applySetClause. Two copies of subtle Neo4j-semantics code tend to diverge over time; a small shared helper would reduce that risk.

5. Unrelated formatting churn in bindings/python/** (scope)

The PR bundles a set of prettier/isort reflows in the Python bindings (import reordering, line rewrapping) that are unrelated to Bolt counters. They're harmless but add noise and make the diff harder to review; ideally these land in a separate formatting commit/PR.

6. Test-coverage gaps

• No dedicated test for the PR's stated "counters reflect the statement's work regardless of a later transaction rollback" invariant - worth locking down since it's an explicit design claim.
• The IT exercises the PULL path; the handleDiscard stats emission and the chunked-PULL "stats only on the terminal has_more=false chunk" behavior aren't directly asserted.

Correctness spot-checks that passed

• DDL routing: query() rejects non-idempotent statements, so DDL is forced through command() (isWriteQuery -> command -> executeDDL), and executeDDL attaches its own QueryStatistics - so index/constraint counters do reach Bolt. Good.
• Read-only queries never allocate a QueryStatistics and getStatistics() returns Optional.empty(), so no stats map is emitted - correct.
• CreateStep retry reset (snapshot before database.transaction, restore at the top of the retried lambda) correctly prevents double-counting; manual-tx steps that don't retry are unaffected.

Nice work overall - the semantics fidelity and test discipline here are notably strong.

Reviewed with Claude Code.