Up to standards ✅

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

Code Review

Overall this is a clean, minimal fix that correctly addresses two independent test failures.

Fix 1: LSMTreeFullTextIndex.searchMoreLikeThis (LSMTreeFullTextIndex.java)

The fix is correct. The root cause is well-diagnosed: RID.getRecord() internally calls DatabaseContext.INSTANCE.getActiveDatabase() via requireDatabase(), which reads from a thread-local context. When multiple databases are open on the same thread (e.g., TestHelper's DB alongside the test's custom DB), the wrong database gets returned, causing the SchemaException: Bucket not found.

The fix - extracting the database reference directly from underlyingIndex.getComponent().getDatabase() and using db.lookupByRID() - is the right approach. It pins the lookup to the correct database regardless of the thread-local context.

A few observations:

• The db variable is assigned outside the for loop, which is correct since it doesn't change per-iteration. Good.
• lookupByRID(sourceRid, true) passes loadContent = true, which matches the original behavior of sourceRid.getRecord() (which calls getRecord(true)). Consistent.
• The null check after lookupByRID was already there and remains correct - lookupByRID can return null for deleted records.
• The subsequent cast (Document) identifiable is unchecked - if a RID in sourceRids points to a Vertex or Edge (which are also Documents), this is fine since both implement Document. However, if a non-Document RID is passed, this would throw ClassCastException at runtime. This was already a pre-existing issue, not introduced by this PR.

This fix also addresses a potential latent bug in production - not just in tests - since any code path that opens multiple databases on a single thread would be affected.

Fix 2: orientdb-export-customfields.gz binary test resource

The missing test resource is added to enable the OrientDBImporterIT#importCustomFields test. The test asserts property-level custom field import for MyType with Name, Tags, and NoCustom properties. The assertions look thorough and cover the right scenarios.

The binary file cannot be reviewed directly, but the test description explains it includes the required OrientDB internal records (#0:1, #0:2) for correct phase transitions in the importer. That is an important detail and worth having in the PR description (which it is).

Minor suggestions (non-blocking)

• Consider adding a comment explaining why db.lookupByRID() is used instead of sourceRid.getRecord(), so future maintainers understand the thread-local context issue:

  // Use the index's own database to avoid thread-local context ambiguity
  // when multiple databases are open on the same thread.
  final DatabaseInternal db = underlyingIndex.getComponent().getDatabase();

Summary

Both fixes are correct, minimal, and well-targeted. The PR description clearly explains the root causes. Test plan is solid. Approve.

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.01%. Comparing base (aeb3cff) to head (9619b0d).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3773      +/-   ##
==========================================
- Coverage   65.17%   65.01%   -0.16%     
==========================================
  Files        1580     1580              
  Lines      116263   116275      +12     
  Branches    24658    24659       +1     
==========================================
- Hits        75775    75600     -175     
- Misses      30193    30363     +170     
- Partials    10295    10312      +17     

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

Code Review

Overall this is a focused, well-scoped set of fixes. A few observations below.

LSMTreeFullTextIndex.searchMoreLikeThis (LSMTreeFullTextIndex.java:704)

The fix is correct and also happens to be a nice micro-optimization - the db reference is now resolved once outside the loop instead of implicitly on every iteration via the thread-local. Good change.

ImmutableVertex.modify / RID.getPageId(BasicDatabase) (ImmutableVertex.java:74,80 / RID.java:196)

The thread-local DatabaseContext issue is real and the fix is clean. However, the same pattern still exists unchanged in two sibling classes:

• ImmutableEdge.java:69,74 - rid.getPageId() (no-arg, uses thread-local)
• ImmutableDocument.java:81,87 - same

Both have a database field available, exactly as ImmutableVertex does, so the same latent bug exists there. Since those classes weren't triggering a test failure today doesn't mean they won't in a multi-database-on-one-thread scenario. Worth fixing in the same pass to keep the codebase consistent.

ArcadeGraphProvider - discrepancy between PR description and actual diff

The PR summary says only testCompareNaN was added to IGNORED_TESTS, but the diff actually adds five tests:

Arrays.asList("testCompareNaN", "testAnd", "testNot", "testOr", "testXor")

testAnd, testNot, testOr, and testXor are not mentioned anywhere in the description. These are semantic correctness tests (boolean comparability operations) - silently skipping them without explanation could mask real compatibility gaps. Please document why each of these four extra tests must be ignored, or confirm whether they were accidentally included.

Minor

The binary resource orientdb-export-customfields.gz - no concerns with including it, but it would be helpful to keep a plain-text source (or at least a comment/note) describing exactly how it was generated, so it can be regenerated or updated if the importer changes in the future.

Summary: The core fixes look correct. Two action items worth addressing before merge:

1. Apply the getPageId(database) fix consistently to ImmutableEdge and ImmutableDocument.
2. Clarify or remove the four undocumented ComparabilitySemanticsTest entries.

Code Review (continued - graph engine changes)

The existing review covers LSMTreeFullTextIndex and the test resource. This covers the remaining graph engine changes.

Fix 3: VertexIterator - was passing null as database

The old constructor called super(null, current) - the database field in ResettableIteratorBase was always null in this code path. This is a pre-existing latent bug now correctly fixed by passing the actual database from vertex.getDatabase().

The change from rid.asVertex(false) to (Vertex) database.lookupByRID(rid, false) is consistent with VertexIteratorFilter, and the cast is safe as long as edge segments only store vertex RIDs (which is enforced by the graph engine). No regression here.

Fix 4: ImmutableVertex.modify() - pins database explicitly

Replacing rid.getPageId() with rid.getPageId(database) is correct - database is the field of the ImmutableVertex instance (already pinned at load time), so this avoids the thread-local ambiguity for the same reason as the index fix.

Fix 5: RID.getPageId(BasicDatabase) overload

Clean addition. The two-line body duplication between getPageId() and getPageId(db) is minor; an alternative would be:

public PageId getPageId() {
    return getPageId(requireDatabase());
}

This would be slightly more DRY but is non-blocking. Either way is fine.

Concern: ArcadeGraphProvider - scope of ignored tests

The PR description mentions testCompareNaN, but the patch actually ignores 5 tests in ComparabilitySemanticsTest: testCompareNaN, testAnd, testNot, testOr, testXor.

Are all 5 failing for the same root cause (COMPARABILITY.compare() throwing IllegalStateException instead of returning false)? If so, the comment should say so. If only testCompareNaN was failing in CI, the others may be precautionary - that distinction is worth documenting so a future reviewer knows whether these are confirmed failures or just preventive exclusions.

The comment currently reads:

// TinkerPop 3.8.0 comparability semantics: COMPARABILITY.compare() throws IllegalStateException for mixed types
// instead of returning false, causing checkHasNext(false,...) to propagate the exception as a failure

Suggested clarification: note explicitly whether all 5 tests were observed to fail, or only testCompareNaN.

Overall: The graph engine fixes are correct and address the same root cause (thread-local database context) consistently across VertexIterator, VertexIteratorFilter, ImmutableVertex, and LSMTreeFullTextIndex. The only non-blocking items are the minor code duplication in RID and the underdocumented scope of ignored Gremlin tests.

Code Review - RemoteMutableEdge changes

The new getOutVertex(), getInVertex(), and getVertex() methods in RemoteMutableEdge were not covered in earlier review rounds. A few issues:

ResultSet resource leak (blocking)

ResultSet extends AutoCloseable. The loadVertex helper never closes it. This should use try-with-resources:

private Vertex loadVertex(final RID rid) {
    try (final ResultSet result = remoteDatabase.query("sql", "select from " + rid)) {
        if (result.hasNext())
            return result.next().getVertex().orElse(null);
        return null;
    }
}

Unsafe Optional.get() (blocking)

result.next().getVertex().get() - getVertex() returns Optional<Vertex>. Calling .get() without checking isPresent() will throw NoSuchElementException if the RID points to a non-vertex record. Use .orElse(null) instead (combined with the fix above).

Missing BOTH direction guard in getVertex() (minor)

getVertex(DIRECTION) silently falls through to getInVertex() for any non-OUT direction, including BOTH, which is not meaningful for a single edge. Suggest an explicit guard:

else if (direction == Vertex.DIRECTION.IN)
    return getInVertex();
else
    throw new IllegalArgumentException("BOTH is not a valid direction for getVertex()");

Tests use mocks only (non-blocking)

The unit tests stub RemoteDatabase and verify delegation but do not exercise the actual SQL query path or resource closure. The resource leak above would not be caught by these tests.