fix(#10588, #10589, #10595, #10596): CREATE INDEX beside an open writer indexes its records, DROP INDEX is all or nothing, list and map equality compares elements as scalars, a named date pattern converts the offset - #10628

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 38.62% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 145 functions across 27 files. (1 skipped…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title accurately summarizes all four primary fixes and identifies the related issues. It is long, but it remains specific and clear.
Description check		The description provides detailed issue context, implementation behavior, related issue references, behavior changes, and regression-test coverage. It omits the template headings for Motivation, Addit…
Linked Issues check		The PR meets the coding requirements for all four active linked issues. For [#10588], transaction registration tracking and reconcileIndexesRegisteredSince queue entries for writes made before index…
Out of Scope Changes check		The changes stay within [#10588], [#10589], [#10595], and [#10596]. Transaction tracking, deletion preflight, equality callbacks, and shared date parsers directly support the linked fixes. The release…

Full details: Description check

Explanation

The description provides detailed issue context, implementation behavior, related issue references, behavior changes, and regression-test coverage. It omits the template headings for Motivation, Additional Notes, and Checklist, but the main required information is present.

Full details: Linked Issues check

Explanation

The PR meets the coding requirements for all four active linked issues. For [#10588], transaction registration tracking and reconcileIndexesRegisteredSince queue entries for writes made before index binding. The commit path reports retryable refusal for conflicting writes, and tests cover other sessions, outer frames, bucket indexes, and propagated indexes. For [#10589], executeDroppingFiles probes and locks files before mutation. dropIndexes preflights multi-index statements, and the regression tests cover refusal points, reopen behavior, and Java API paths. For [#10595], QueryOperatorEquals supplies scalar equality to list and map element comparison. The tests cover temporal representations, =, <>, IN, CONTAINS, CONTAINSALL, and nested values. For [#10596], shared format parsers apply offset conversion, wall-clock date rules, and the supplied zone. SQL functions, date functions, GraphImporter, and source-scan regression tests cover the required paths.

Full details: Out of Scope Changes check

Explanation

The changes stay within [#10588], [#10589], [#10595], and [#10596]. Transaction tracking, deletion preflight, equality callbacks, and shared date parsers directly support the linked fixes. The release-note changes document the four behavior changes. The added tests validate the linked requirements and are supporting implementation work. No unrelated product change is demonstrated by the reviewed diff or supplied change summary.

Full details: Docstring Coverage

Explanation

Docstring coverage is 38.62% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 145 functions across 27 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Up to standards ✅

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

Review

Author is a maintainer and the diff adds no dependencies or security-sensitive code, so I found no security hazards. I reviewed the transaction and index parts (#10588, #10589) and the date and equality parts (#10595, #10596) of the diff. I did not build the code or run the tests. The diff output was truncated, so I did not see the rest of the `LocalSchema`, `IndexBuildCallerChanges` and `TypeIndexBuilder` changes or most of the tests.

What looks good

• The CREATE INDEX silently skips a record another session wrote and has not yet committed: it stays invisible to every index-backed query, and a UNIQUE index then accepts a duplicate of its key #10588 design is sound. The transaction reconciles at `commit1stPhase` while still `BEGUN`, on its own thread and under the read lock. That avoids reading another thread's pages. The common path costs one volatile read and one compare.
• `indexRegistered` runs after the binding, and the Javadoc explains why that ordering matters.
• Each fix has its own regression test, and the release notes document the behaviour change.

Points to consider

1. Silent skip when the transaction is not on top of the stack (`TransactionContext.reconcileIndexesRegisteredSinceBegin`). If `database.getTransactionIfExists() != this`, it only logs a WARNING and the commit goes ahead. The records are then unindexed, which is the same corruption CREATE INDEX silently skips a record another session wrote and has not yet committed: it stays invisible to every index-backed query, and a UNIQUE index then accepts a duplicate of its key #10588 fixes (and a UNIQUE index can accept duplicates). Failing the commit (a retryable exception) would be safer than a log line. If the case is truly unreachable, an assertion or exception documents that. Please also add a test for this branch.
2. `(int)` cast in `noteBucketWritten` of `sequence - indexRegistrationSequenceAtBegin`. This is fine in practice, but `wroteToBucketBefore` could keep the delta as a long, or you could store the absolute sequence in a long-valued map, to avoid the cast.
3. `lastNotedBucket` cache. It is reset in `begin()` and `reset()`. Please confirm that every path that nulls `bucketFirstWriteRegistrations` also resets it (e.g. rollback and retry paths that do not go through `reset()`). A stale cache would skip recording a bucket.
4. `indexRegisteredAt` lifecycle. Entries are removed in `indexUnregistered`. Check that index rename, schema reload and database close paths also clean it, so it does not leak or leave stale names.
5. Breaking semantic changes (SQL = on a list or map still compares a temporal element with Java equals, so [<Date>] is not equal to the stored [<LocalDateTime>] of the same instant, which the scalar =, CONTAINS, CONTAINSALL and openCypher all say it is #10595, asDateTime(format) and date(value, format) keep the wall clock of an offset-bearing string, so the same pattern converts when the schema applies it and does not when the caller names it #10596). `=` on list and map elements now converts (`[1] = ['1']` is true), and date patterns with offsets now store the instant. Both are documented in the release notes. They are visible to existing queries, so consider calling them out as behaviour changes in the PR title or changelog. Verify that the `equals` fast paths for same-class lists did not become noticeably slower, since each element now goes through the generic `equals`. A micro-benchmark on large lists would settle it.
6. `Date` subclass handling. `new Date(date.getTime())` drops the nanos of a `Timestamp` when it is compared against a different class. That seems intended and is commented, but it is worth a test.
7. Style. CLAUDE.md prefers imports over fully qualified names. I did not see violations in the visible part of the diff. Also confirm that no `System.out` debug output remains.

Overall the approach looks solid. Point 1 is the one I'd address before merging.

Review

Author is a maintainer and the diff adds no dependency, no new network or file surface, and no security hazard that I can see, so I did not apply the "do not merge" label. I read the engine changes (10588, 10589) closely. I skimmed the date/temporal changes (10595, 10596) and have not run the tests.

What looks good

• The 10588 design is sound. Each transaction owes the index entries for records it wrote before the index was bound, and it makes them up at commit1stPhase on its own thread, while it is still BEGUN. That avoids reading another thread's pages. The hot-path cost is small: one volatile read per commit and one int compare per write (lastNotedBucket).
• indexRegistered is called after the binding. The Javadoc explains why that order matters.
• indexRegisteredAt entries are removed on drop and moved on rename, so the map does not leak.
• 10589: executeDroppingFiles is now the only way to delete files, and requireFilesCanBeDeleted is private. That prevents the same mistake from being repeated. dropBucket now probes all files before it drops anything.
• Every fix comes with a regression test.

Points to consider

1. Index counters and retries. If a transaction wrote to a bucket on both sides of the registration, it throws ConcurrentModificationException. This only happens when an index is created concurrently with a writer, and the retry works, so I think it is acceptable. Please confirm every caller path (HTTP, embedded database.transaction(..., retries)) actually retries on it. A test showing a second attempt succeeds would document this.
2. Nested transaction refusal. reconcileIndexesRegisteredSince throws a TransactionException when the committing transaction is not on top of the stack. An outer transaction that began before a CREATE INDEX and wrote to the bucket therefore cannot commit while an inner one is still open. This seems correct and safer than skipping, but make sure a test asserts it.
3. Replication. I did not check how indexRegisteredAt behaves on HA followers. There the schema change arrives via the replicated log and not via createBucketIndex. If followers also run commits, confirm the sequence is bumped on that path as well. Otherwise a follower-side transaction would not know it owes entries.
4. Scope. The PR bundles four unrelated fixes: index build, drop atomicity, and two date/temporal issues. Smaller PRs would be easier to review and revert. Not a blocker.
5. Style. IndexBuilder has a Javadoc line well over 160 characters, added in this diff. hasEntriesInFiles builds a throwaway ArrayList of index names; it is only on the commit path of an affected transaction, so it is fine, but iterating the two key sets directly would avoid the allocation.

Overall the changes look correct and well-tested. My only real question is the HA/replica path in point 3.

Review

I read the core of #10588 (TransactionManager, TransactionContext, LocalSchema#reconcileIndexesRegisteredSince) closely. I only skimmed the #10589, #10595 and #10596 changes and the tests, and I did not run the build or the tests.

Security: I found no hazards. There are no new dependencies, no new I/O surface and no new deserialization. The DROP INDEX change makes file deletion safer, because it now runs the existing requireFilesCanBeDeleted probe first.

Design (#10588)

• Making the owning transaction queue what it owes at commit1stPhase, while it is still BEGUN and holds the read lock, avoids reading another thread's pages. That is a better design than refusing CREATE INDEX under any concurrent writer.
• Calling indexRegistered after the binding, under the schema write lock, closes the begin/bind race, and the comment explains why.
• The fast path is one volatile compare per commit. That is fine.

Points to check

1. TransactionManager#getIndexesRegisteredSince allocates a new HashMap and iterates every registered index. It only runs when the sequence moved, so it is cheap. But a long-lived transaction that began before an index was created will pay it on every later commit of that transaction. That is rare, so I would leave it.
2. indexRegisteredAt is keyed by index name. Rename and drop are handled. Please confirm that every other path that removes or replaces an index also calls indexUnregistered or indexRenamed. These include a failed CREATE INDEX that rolls back after indexRegistered, type drop, vector index compaction, and database reload or HA replication on followers. A stale entry would make a later commit run the reconcile for an index that is gone. The lookupIndex == null guard covers that case, so this is robustness only.
3. The ConcurrentModificationException for a transaction that wrote on both sides of the registration is a conservative choice and is retryable. It is documented, and I accept it.
4. The TransactionException for a nested transaction that owes entries is a new commit-time failure mode. It is covered by the docs entry, but it is worth a line in the user-facing release notes if it is not already there.

#10595 and #10596

• Passing the element rule into BinaryComparator#listsEqual and mapsEqual is a good way to keep = and element equality consistent.
• The source-scan test that forbids LocalDateTime.parse(text, pattern) outside DateUtils is a useful guard.
• date(value, format, timezone) changes meaning, because the timezone argument was inert and now takes effect. This is a behaviour change and is listed in the release doc.

Tests and style

• Each of the four fixes has its own regression test, and they use the assertThat style.
• Please make sure the concurrent CREATE INDEX silently skips a record another session wrote and has not yet committed: it stays invisible to every index-backed query, and a UNIQUE index then accepts a duplicate of its key #10588 tests use latches rather than wall-clock assertions. CLAUDE.md asks for StallAwareStopwatch where timing matters, and @Tag("slow") on anything long.
• Please make sure none of the new tests bind hard-coded server ports.

The only things I would ask for are the check in point 2 and a confirmation that CI is green, including the HardcodedTestServerPortsTest scan. I would approve otherwise.

Review

Four independent engine fixes (#10588, #10589, #10595, #10596), each with its own regression test. The PR comes from a maintainer. I did a static read and did not run the tests.

Security: I found no security hazards. The diff adds no new dependencies, no I/O or network surface, no reflection, and no use of the common ForkJoinPool. I also saw no leftover System.out or printStackTrace calls.

Design (#10588): Having the transaction reconcile the indexes bound after its first write, instead of refusing CREATE INDEX while other writers are active, looks like the right trade-off. It adds no cross-thread page inspection, and the cost on the hot path is one volatile read. Points to check:

• The registration counter (indexRegistered) must be incremented strictly after the binding and under the schema write lock. As described, it is.
• The ConcurrentModificationException for a transaction that wrote to the same bucket on both sides of the registration is the conservative choice, and it is retryable. A short note in the docs on when users may see it would help.
• DuplicatedKeyException at commit time for a key the new index gave to another record is correct. Please confirm the rollback leaves no partial index entries behind. A test with a full-text index would cover the double-counting case you mention.

#10589: Probing every bucket sub-index file before dropping any of them is consistent with what #10421 did for DROP TYPE. The change is atomic and the test covers a restart.

#10595 and #10596: The date and offset semantics changes are documented in the release notes, including who will see different answers. Existing stored values are not rewritten, and that is stated clearly.

Tests: Each fix has a dedicated Issue*Test. Two suggestions:

• Add a concurrent-writer stress case for CREATE INDEX silently skips a record another session wrote and has not yet committed: it stays invisible to every index-backed query, and a UNIQUE index then accepts a duplicate of its key #10588 (writer open across the CREATE INDEX, then commit, for UNIQUE and non-unique indexes).
• Make sure none of the new tests assert on raw wall-clock time (see StallAwareStopwatch in CLAUDE.md).

Overall this looks good to me. No blocking issues found.

Codecov Report

❌ Patch coverage is 90.27027% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.93%. Comparing base (98263fb) to head (bf5023d).
⚠️ Report is 1 commits behind head on main.

@@             Coverage Diff              @@
##               main   #10628      +/-   ##
============================================
+ Coverage     78.91%   78.93%   +0.01%     
+ Complexity     4757     4749       -8     
============================================
  Files          2294     2294              
  Lines        250223   250367     +144     
  Branches      54565    54598      +33     
============================================
+ Hits         197474   197626     +152     
+ Misses        32571    32550      -21     
- Partials      20178    20191      +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.