fix(#5968): CSV/JSON importer skip-and-log mode instead of aborting on first bad row

[lvca]

Summary

Follow-up from #5967 (fix for #5905/#5906: Type.convert() now rejects an out-of-range numeric value being narrowed to INTEGER/SHORT/BYTE instead of silently wrapping it).

CSVImporterFormat.loadDocuments/loadVertices and JSONImporterFormat.parseRecords had no per-row/per-record error handling: a single malformed or out-of-range property value aborted the entire bulk import job, which is expensive to iterate on for large imports (fix one bad row, restart, hit the next one, repeat).

Behavior change in the default (abort) path — please read

While fixing this, CSVImporterFormat.loadVertices turned out to have a pre-existing bug that this PR also fixes, even without opting into -onRowError skip: vertices are persisted via database.async(), and a persist-time failure (e.g. a missing mandatory property, only caught on the async worker thread) was previously only logged at SEVERE — the import completed "successfully" even though some vertices silently failed to persist. That handler now captures the failure and the import aborts with an ImportException instead of silently succeeding. Note this still isn't full atomicity for vertices: they persist in commitEvery-sized database.async() batches, and a persist-time failure only rolls back the batch containing the bad record - any earlier batch already committed stays durably persisted (see csvVertexImportAbortsOnOutOfRangeValueByDefaultButPriorBatchesSurvive, which pins this down). "abort" means "fail loudly" for vertices, not "nothing is imported" the way it does for documents. This is arguably a correctness fix and is covered by csvVertexImportAbortsOnAsyncPersistTimeFailureByDefault, but it is a behavior change in the default path: an existing pipeline that was unknowingly relying on "vertex import completes even if a few rows fail validation" will now see that import abort instead of silently dropping rows.

Changes

• ImporterSettings: new opt-in -onRowError skip|abort setting (also usable from IMPORT DATABASE ... WITH onRowError=skip), defaulting to abort to preserve today's behavior for backward compatibility (aside from the fix above). Invalid values are rejected eagerly. Governs document/vertex rows only — edges already skip-and-log unconditionally, since an unresolved from/to reference is expected during graph import, not a data error.
• CSVImporterFormat.loadDocuments/loadVertices: per-row try/catch around the build+save. In skip mode, each row commits (or rolls back) its own transaction instead of sharing one transaction for the whole file — a failure that only surfaces at index time (a duplicate key) already has a bucket write by then, and only rolling back that row's own transaction can undo it without discarding an unrelated, already-committed row ("ghost record" — present in the bucket, never indexed). loadVertices additionally switches to a synchronous per-vertex save() in skip mode instead of database.async(), for the same reason (an async batch rollback would otherwise take down every other vertex queued in the same uncommitted batch, not just the failing one). This trades import throughput — effectively a batch size of 1 for the whole run while skip mode is on, not just the failing row — for the guarantee that skip mode can never lose more than the one bad row, nor leave a ghost record behind.
• JSONImporterFormat: a Type/schema conversion error is caught at every nested BEGIN_OBJECT/BEGIN_ARRAY recursion site in parseRecord()/parseArray(), not just at the top level — a failure buried in a nested mapped object could otherwise unwind past the enclosing object's own reader.endObject(), desyncing the JsonReader for the rest of the array. A nested failure discards the whole enclosing top-level record (not just the bad nested field) via the existing top-level rollback path, since a nested record can already have a bucket write that only a full transaction rollback can safely undo. createdDocuments/createdVertices/createdEdges counters are snapshotted before each top-level record and reset on rollback, so a record that gets allocated-then-discarded is never counted. A genuinely malformed JSON structure (an IOException, not a data error) is not caught anywhere in this chain and still aborts the import, even in skip mode.
• Both formats reuse the existing ImporterContext.errors counter (already used elsewhere in JSONImporterFormat for other non-fatal skip conditions) rather than adding a new one; it's surfaced in Importer.load()'s returned summary map ("errors").

Test plan

Issue5968ImporterSkipOnRowErrorTest (in integration, 38 tests) covers, for both CSV and JSON:

• Default abort behavior reproduces the original bug (ImportException with the expected root cause), including the async persist-time-failure fix above.
• -onRowError skip imports the valid rows, skips the bad one, and reports the expected errors/createdDocuments/createdVertices counts.
• Ghost-record regression: a duplicate key detected only at commit time doesn't leave a bucket-only, unindexed record behind, for both CSV documents and vertices.
• Nested JSON mapped object failure: the whole enclosing record is discarded, sibling records are still processed, no reader desync.
• IMPORT DATABASE ... WITH onRowError=skip (the SQL entry point) is covered separately from the CLI-arg constructor.
• Invalid -onRowError value is rejected eagerly at settings-parsing time.

Ran CSVImporterIT, JSONImporterIT, GraphImporterCSVTest, GraphImporterTypedPropsTest alongside the new tests — all green, no regressions. mvn -pl integration -am install (compile + full module test run) passes. Full suite repeated 3x to rule out flakiness in the async/sync transaction paths.

🤖 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

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

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 of #5974 (onRowError skip|abort for CSV/JSON importer)

Nice fix, and the approach (opt-in setting, default-preserving, reusing ImporterContext.errors) is sound. I traced through MutableDocument.set() -> convertValueToSchemaType() -> Type.convert() to confirm the out-of-range IllegalArgumentException from #5967 is indeed thrown synchronously inside .set(), before .save()/async().createRecord() is reached, so the new try/catch does reliably catch the case this PR targets. A few things worth a look before merge:

1. loadVertices won't catch errors that only surface at persist time (medium)

loadDocuments uses synchronous document.save() inside the try block (CSVImporterFormat.java:140), so any validation error (mandatory property, unique index, etc.) is caught by the new try/catch.

loadVertices, however, hands the vertex to database.async().createRecord(v, ...) (CSVImporterFormat.java:255), which just enqueues the record for a worker thread. Schema conversion in .set() is synchronous and gets caught, but anything that only validates at persist time (DocumentValidator/mandatory-property checks, unique-index conflicts) happens later in DatabaseAsyncCreateRecord.execute() on the async worker thread, outside this method's try/catch entirely. That path swallows the exception into the pre-existing database.async().onError(...) handler (just a SEVERE log) - it doesn't increment context.errors, and it doesn't abort the import even in the default abort mode. So for vertices, "abort" isn't actually guaranteed for this whole class of errors, and "skip" mode's errors count will under-report them. Documents don't have this gap because they're fully synchronous.

Worth at least documenting the limitation, or (better) wiring context.errors.incrementAndGet() into the async onError handler and, for abort mode, propagating a failure so the import actually stops (e.g. via waitCompletion() result / a shared error flag checked after the loop).

2. loadEdges isn't covered and already had its own unconditional skip-and-log (pre-existing, low)

CSVImporterFormat.loadEdges (untouched by this PR) already wraps createEdgeFromRow(...) in a try/catch that unconditionally logs and continues on any exception, regardless of -onRowError (CSVImporterFormat.java:357-367). That's not a regression from this PR, but now that documents/vertices have an explicit default-abort semantic, the inconsistency is more visible: a bad edge row silently continues even with the default settings, while a bad document/vertex row aborts. Might be worth a follow-up to bring edges under the same setting for consistency, or at least a comment noting the divergence.

3. Minor: JSON "record #%d" log number can be off (low)

JSONImporterFormat.java:158 logs context.parsed.get() as the record number, but context.parsed is also incremented for nested embedded objects/arrays parsed via recursive parseRecord calls (JSONImporterFormat.java:183). For records with nested objects the logged number won't line up with the top-level array index. Cosmetic only, doesn't affect the errors counter or behavior.

Test coverage

The new Issue5968ImporterSkipOnRowErrorTest covers the abort/skip pair for both CSV vertices and JSON documents, plus eager rejection of an invalid -onRowError value - good coverage for the synchronous type-conversion case. Given point (1) above, it might be worth adding a case that exercises a vertex validation failure that only surfaces at persist time (e.g. a missing mandatory property) to confirm/document current behavior there.

Nit

ImporterSettings.onRowError stores the raw string as passed (case preserved) rather than normalizing to lowercase; harmless since isSkipOnRowError() does a case-insensitive comparison, but a normalize-on-set would avoid any future direct-comparison bugs if another spot ever compares onRowError directly.

Overall this is a solid, backward-compatible fix for the reported issue (#5968) with a reasonable, well-tested implementation for the document/CSV-vertex-set-time paths. The main thing I'd want addressed or at least explicitly acknowledged before merge is the async vertex-persist gap in point 1, since it affects both the advertised default abort behavior and the accuracy of the errors count for skip mode.

[lvca]

Addressed in bf0b516:

1. Async vertex persist-time errors (medium) - fixed. loadVertices' onError callback now increments context.errors and captures the first failure; after waitCompletion(), the import rethrows and aborts in the default abort mode instead of silently logging. Added regression tests for both modes using a mandatory-property violation that only surfaces on the async worker thread. Note: because vertices go through database.async(), a failing task rolls back its whole in-flight batch (DatabaseAsyncExecutorImpl#executeTask) - "skip" guarantees the bad row itself is never persisted, but doesn't guarantee sibling rows in the same uncommitted batch survive (documented in the new test).
2. Edge-path divergence (low) - added a comment in loadEdges explaining why it intentionally stays an unconditional skip-and-log independent of -onRowError (unresolved from/to vertex references are expected during graph import, not data corruption). Left as-is rather than folding into this setting, to avoid changing edge-import semantics as a side effect of this fix.
3. JSON record number in logs (low) - fixed, now logs the top-level array index instead of context.parsed (which also counts nested embedded objects/arrays).
4. Case normalization nit - fixed, onRowError is now lowercased on set.

All existing importer tests (CSVImporterIT, JSONImporterIT, GraphImporterCSVTest, GraphImporterTypedPropsTest) plus the expanded Issue5968ImporterSkipOnRowErrorTest (7 tests now) pass.

[claude]

Review

Solid fix for a real pain point (aborting a large bulk import on the first bad row). The abort-by-default behavior preserves backward compatibility, the opt-in flag is validated eagerly, and the new tests do a good job distinguishing the synchronous validation path from the async persist-time path for vertices. A few things worth a look before merge.

Correctness: "skip" mode can silently drop valid vertices, not just the bad one

CSVImporterFormat.loadVertices persists vertices via database.async().createRecord(...). Looking at DatabaseAsyncCreateRecord.execute(), when a persist-time failure happens (mandatory property, unique index, etc.) it rolls back the whole active transaction on that async worker thread, not just the failing record. That worker's transaction batches up to ASYNC_TX_BATCH_SIZE operations before committing (default 10,240, see GlobalConfiguration.ASYNC_TX_BATCH_SIZE).

So in -onRowError skip mode, a single bad row near the end of a batch can roll back thousands of already-"created" valid vertices queued in the same uncommitted batch on that worker, and context.errors/the "Skipped rows" log will still only show 1. The new test csvVertexImportCountsAsyncPersistTimeFailureWhenOptedIn actually documents this exact caveat in a comment ("does not guarantee that sibling rows queued in the same uncommitted async batch survive") and deliberately avoids asserting a survivor count, so the behavior is understood, just not visible to a user of the feature. Given the whole point of skip mode is "don't lose the good rows over one bad one," this is worth calling out more prominently, e.g. in the -onRowError doc/comment, or by lowering the effective async batch size while skip mode is active for vertex imports so a persist-time failure has a smaller blast radius. Synchronous failures (out-of-range values caught in the per-row try/catch before the record ever reaches the async queue) don't have this problem, and neither does the JSON path since each record commits its own transaction, so this is specific to CSV vertex persist-time failures.

Minor / non-blocking

• CSVImporterFormat.loadDocuments still registers a log-only database.async().onError(...) callback (line ~121) and calls database.async().waitCompletion(), even though document creation in that method is fully synchronous (document.save(), not database.async().createRecord(...)). It looks like dead/vestigial code predating this PR; since loadVertices right below it was just fixed to correctly handle async persist-time errors, it might be worth a follow-up to either use the same pattern for documents (if it's ever going to be made async) or drop the unused registration to avoid confusion. Not introduced by this PR, just adjacent.
• The skip-mode log messages pass only e.getMessage() (not the exception itself) to LogManager, so no stack trace is recorded for a skipped row. That's a reasonable tradeoff to avoid log spam on large imports with many bad rows, but it can make root-causing a skipped row harder after the fact; consider DEBUG-level logging of the full exception.
• context.errors is reused for several unrelated pre-existing "soft" conditions in JSONImporterFormat (missing @cat/@type, unresolved @id, etc.) in addition to the new row-skip counter. The PR description calls this out as intentional, but it does mean the "errors" figure in the returned summary map mixes structural mapping issues with actual skipped bad-data rows, worth keeping in mind if this metric is ever surfaced to end users distinctly.
• Test coverage is good for CSV vertices (sync + async) and JSON documents, but there's no test for -onRowError skip on the CSV document path (CSVImporterFormat.loadDocuments), which also got the try/catch treatment. Might be worth a quick additional case for symmetry, even though the code path is nearly identical to the vertex one.

Things verified while reviewing

• firstAsyncError/context.errors updates from the async onError callback are guaranteed to happen-before waitCompletion() returns (the completion marker is enqueued after the real tasks on each worker's FIFO queue and executeTask invokes onError synchronously before signaling completed()), so the abort-after-waitCompletion() check in loadVertices is race-free.
• IMPORT DATABASE ... WITH onRowError=skip works without any special-casing since ImportDatabaseStatement routes all WITH settings generically through AbstractImporter.setSettings -> ImporterSettings.parseParameter.
• ValidationException/ArcadeDBException/IllegalArgumentException are all unchecked, so catching RuntimeException in the per-row try/catch blocks is sufficient; no checked exceptions are being silently missed.
• Code style matches project conventions (final params/locals, no braces on single-statement if, LogManager printf-style usage consistent with the rest of the file).

Nice, well-tested change overall; the batch-rollback caveat above is the main thing I'd want addressed or at least explicitly documented before merge.

[lvca]

Addressed in e539e9b:

Main point - "skip" can silently drop valid vertices in the same async batch - fixed properly this time, not just documented. loadVertices now forces database.async().setCommitEvery(1) for the duration of the import when -onRowError skip is set (restored afterward). Since a persist-time failure only ever rolls back its own single-operation transaction now, the blast radius is exactly the failing row - siblings queued after it are unaffected. Tightened csvVertexImportCountsAsyncPersistTimeFailureWhenOptedIn to assert the exact surviving count (previously it deliberately avoided that assertion because of this gap). Documented the trade-off (throughput for guarantee) on ImporterSettings.isSkipOnRowError().

Minor points:

• Removed the vestigial database.async().onError(...) registration + waitCompletion() in loadDocuments - confirmed dead code (that method is fully synchronous, nothing is ever queued), not worth a separate follow-up to fix in place.
• Added Level.FINE logging of the full exception (not just getMessage()) alongside the existing WARNING summary for every skip site (CSV documents, CSV vertices, JSON), so root-causing a skipped row doesn't require reproducing it.
• Added csvDocumentImportSkipsOutOfRangeValueWhenOptedIn for CSV-document/skip-mode symmetry with the vertex test.
• context.errors mixing structural JSON mapping issues with row-skip counts: leaving as-is per the earlier note, it's the existing counter's established semantics in this file.

All existing importer tests plus the now-9-test Issue5968ImporterSkipOnRowErrorTest pass (3x repeated run to confirm no flakiness from the async path).

[claude]

Review of #5974 - importer skip-on-row-error

Overall this is a well-scoped, well-tested follow-up to #5967. The abort/skip split, the async-persist-time-failure handling for vertices, and the edge-loop comment explaining why edges are excluded are all good calls. A couple of things worth a look before merge:

1. JSON skip mode can desync the reader on nested/embedded objects (likely bug)

JSONImporterFormat.parseRecords (JSONImporterFormat.java:143-165) relies on the invariant that "a Type/schema conversion error can only be thrown after the record's JSON tokens have been fully consumed" to safely resume at the next array entry. That invariant only holds for a top-level, flat record.

parseRecord recurses for BEGIN_OBJECT/BEGIN_ARRAY properties (JSONImporterFormat.java:209-227, parseArray at 446-489). If a type/mandatory-property error is thrown from inside a nested parseRecord/parseArray call (e.g. an embedded object has a SHORT property that overflows), the exception unwinds past the outer object's own while (reader.peek() != END_OBJECT) loop and its reader.endObject() call (line 238) - those never run. The reader is left positioned mid-object, not at the array-record boundary.

When control returns to parseRecords, reader.peek() == BEGIN_OBJECT (loop condition, line 134) will likely be false for the next token (it's probably NAME, still inside the abandoned object), so the while loop just exits silently - dropping every remaining record in the array without logging or counting them as skipped - and then reader.endArray() (line 172) throws (structural mismatch), aborting the import anyway with a confusing IllegalStateException unrelated to the actual bad value.

So for a JSON import whose mapping has nested embedded objects/arrays with schema'd properties, -onRowError skip doesn't degrade gracefully to "skip this one record" - it can silently drop an arbitrary tail of the array and then still abort. The new tests only cover a flat top-level property (qty), so this path isn't exercised. Worth either scoping the doc comment's safety claim to flat mappings, handling the nested case defensively, or adding a regression test with a nested/embedded object mapping to confirm/disprove this.

2. setCommitEvery(1) mutates shared, database-wide async state (design concern)

CSVImporterFormat.loadVertices (CSVImporterFormat.java:213-219, 289-290) calls database.async().setCommitEvery(1) and restores it in finally. DatabaseAsyncExecutorImpl.commitEvery is a single field shared by all worker threads and all callers of database.async() on that Database instance - it's not scoped to this import.

• If anything else uses database.async() concurrently on the same open database while a skip-mode vertex import runs (a live server doing other async writes, or two importer runs sharing a Database object), this forces every unrelated async batch down to size 1 for the duration - a real throughput hit elsewhere, not just for this import.
• It's also a race on the shared field itself: if another thread calls setCommitEvery(N) concurrently for its own purposes, this import's finally-block restore (database.async().setCommitEvery(originalCommitEvery)) can clobber that unrelated change back to whatever value was captured at import start.

For the typical CLI/offline-import use case this is a non-issue, but nothing in the API scopes commitEvery to a single caller, so it's worth confirming this is only ever invoked where the Database instance isn't shared with concurrent async writers (or documenting that constraint explicitly).

3. Minor

• context.errors is reused for both "hard" row-abort-worthy errors and the pre-existing soft warnings elsewhere in JSONImporterFormat (unresolved @cat/@type, unsupported property types, etc.). Pre-existing behavior, not a regression, but combined with -onRowError skip the reported errors count may not line up 1:1 with "rows actually skipped due to onRowError" if someone tries to correlate the two.
• ImporterSettings.commitEvery (existing CSV-edge-loop batch-size field, default 5000) and DatabaseAsyncExecutorImpl's own commitEvery are two different, same-named knobs. The new doc comment on isSkipOnRowError() references the latter; a one-line disambiguation would help readers skimming the settings field list.

What looks solid

• Default abort behavior is preserved and tested (Issue5968ImporterSkipOnRowErrorTest), including the previously-silent async-persist-time-failure case for vertices - good catch, and a good regression test for it.
• CSV documents' "no rollback needed" reasoning checks out: MutableDocument.set() converts/validates eagerly before .save() writes anything, so a caught exception there really does mean nothing was persisted for that row.
• Removing the dead database.async() registration/waitCompletion() in loadDocuments (which is fully synchronous, not async) is a correct, welcome cleanup.
• Eager rejection of invalid -onRowError values and the edge-loop's explicit, well-explained opt-out are both good calls.

Nice test coverage overall. The two points above are what I'd want resolved or explicitly acknowledged before merging.

[lvca]

Addressed in 9c42e5f:

1. JSON reader desync on nested objects (likely bug) - confirmed and fixed. You were right: the invariant only held for flat top-level records. Every nested BEGIN_OBJECT/BEGIN_ARRAY recursion site in parseRecord()/parseArray() now catches RuntimeException at the call site (both attributeValue/entryValue locals dropped final to allow it). This is safe by induction: whichever nesting level's own createRecord() throws, that level's own reader.beginObject()/endObject() (or beginArray()/endArray()) pair is always fully consumed before createRecord() runs, so catching right there - substituting null for just that property/item and letting the enclosing loop continue normally - never leaves any level's tokens unbalanced. Added jsonNestedObjectImportSkipsOutOfRangeValueAndKeepsProcessingSiblingsWhenOptedIn (a "customer" object nested inside "Order", independently mapped to its own type) plus the abort-mode counterpart, both passing. Updated the safety comment in parseRecords to state the corrected, now-actually-true invariant instead of the flat-only claim.

2. setCommitEvery(1) mutating shared database-wide async state (design concern) - fixed by removing it, not just documenting it. Good catch on the shared-field race. Replaced that approach entirely: in -onRowError skip mode, loadVertices now saves each vertex synchronously (v.save() in an explicit transaction spanning the loop) instead of going through database.async() at all - the same way loadDocuments already sidesteps the batch-rollback problem. Nothing shared or global is touched; abort mode (the default) is completely unchanged and still uses the async path as before, so there's no throughput regression for the common case.

3. Minor points - context.errors mixing and the two same-named commitEvery knobs: left as pre-existing/informational per the earlier reply; the isSkipOnRowError() javadoc has been updated to reflect the corrected (synchronous-save) mechanism.

Full Issue5968ImporterSkipOnRowErrorTest (11 tests now) plus CSVImporterIT/JSONImporterIT/GraphImporterCSVTest/GraphImporterTypedPropsTest pass, repeated 3x to rule out flakiness in the async/sync paths.

[claude]

Review in progress - posting full findings shortly.

[claude]

Review: -onRowError skip|abort importer setting (issue 5968)

Overall this is a solid, well-tested feature: the opt-in default-abort design, the reader-desync analysis for nested JSON, and the abort-mode async persist-time-failure fix (previously silently swallowed even in the default abort mode) are all good. There is one real correctness gap around transaction handling in skip mode, plus a couple of minor nits.

1. Skip mode can commit a partially-written ghost record when the failure happens after the bucket write (e.g. duplicate unique key) [Confirmed]

CSVImporterFormat.loadDocuments/loadVertices open a single transaction before the row loop and only call database.commit() once, at the very end. The per-row catch (RuntimeException e) (CSVImporterFormat.java around line 140-148 for documents, 275-283 for vertices) logs and continues the loop, but unlike JSONImporterFormat.parseRecords, which explicitly calls database.rollback() before continuing, it never rolls back.

That matters because LocalDatabase.createRecordNoLock writes the record into the transaction (bucket.createRecord(...), transaction.updateRecordInCache(...), transaction.updateBucketRecordDelta(...)) BEFORE indexer.createDocument(...) runs. If the failure is a unique-index violation (DuplicatedKeyException, itself an ArcadeDBException/RuntimeException), the bucket slot has already been allocated by the time the exception is thrown, and since an explicit transaction is active (not the implicitTransaction case), there is no automatic rollback of that partial write.

Concretely: -vertices dirty.csv -typeIdProperty Id -onRowError skip (the exact pattern used in this PR's own tests) auto-creates a unique index on Id. If two rows share the same Id, the second row's v.save() throws DuplicatedKeyException inside indexer.createDocument(), after bucket.createRecord() already ran. The catch block increments context.errors and moves on, but the half-created vertex (RID assigned, present in the bucket, bucket-count delta already bumped) is never rolled back, and rides along into the single database.commit() at the end of the loop. The result is a vertex that exists in the bucket (shows up in db.countType()/full scans) but has no reachable index entry, so it can never be found via lookupByKey, can never be resolved as an edge endpoint by loadEdges, and cannot be cleaned up through normal key-based operations. That is worse than the abort behavior this feature is meant to improve on, and it quietly breaks the PR's own stated guarantee that skip only ever loses the bad row; here the bad row is not fully lost, a corrupted fragment of it survives.

The same gap exists in JSONImporterFormat's nested object/array recursion catches (parseRecord's BEGIN_OBJECT/BEGIN_ARRAY cases, parseArray's BEGIN_OBJECT/BEGIN_ARRAY cases): those set attributeValue/entryValue = null and continue without rolling back. Only the top-level parseRecords() catch rolls back correctly, because that's the only place where the whole per-record transaction is scoped and discarded.

The out-of-range/mandatory-property cases this PR's tests exercise do not hit this, because Type.convert() failures happen in MutableDocument.set() (pure in-memory) and mandatory-property failures happen in DocumentValidator.validate(), both of which run before bucket.createRecord(). It is specifically index-time failures (duplicate keys being the realistic one, since a unique index on the id column is exactly what this PR's own vertex-import codepath creates) that fall through the gap. Might be worth rolling back per-row in the CSV paths too (mirroring what JSON already does at the top level), or documenting the limitation explicitly if a full per-row transaction wrap is judged too costly.

2. Skip mode drops periodic commits for vertices (minor/performance)

Pre-PR, vertex import always went through database.async() with its own internal batching/commit cadence. In skip mode, vertices are now saved synchronously one-by-one inside a single transaction that isn't committed until the entire file has been read. settings.commitEvery is effectively ignored for the skip path. For very large CSVs this means the whole import is held in one uncommitted transaction, a different memory/WAL footprint than the batched-async default. It might be worth committing every N rows (still per-row save(), just periodic commit()/begin()) to bound this, especially since large/dirty files are exactly the use case -onRowError skip targets.

3. Minor: stale test doc/name

Issue5968ImporterSkipOnRowErrorTest.csvVertexImportCountsAsyncPersistTimeFailureWhenOptedIn (and its inline comment) still describes the force setCommitEvery(1) on database.async() mitigation from an earlier commit in this PR. The final implementation (commit 4) replaced that with a fully synchronous v.save() for skip mode, so skip mode no longer touches the async executor at all; the comment (and arguably the test name) is now stale/inaccurate.

Nit: broad catch (RuntimeException e)

All the new catch sites catch RuntimeException broadly rather than the specific data-related exceptions (ValidationException, IllegalArgumentException, DuplicatedKeyException, ...). In skip mode, an unrelated bug elsewhere (e.g. an NPE from a code defect) would now be silently logged as a skipped row instead of surfacing as a real failure. This may be an intentional robustness trade-off for a bulk importer, just flagging it as something to be aware of.

Test coverage is otherwise thorough (abort/skip pairs for CSV documents, CSV vertices, JSON documents, JSON nested objects, plus the async persist-time-failure regression and eager -onRowError validation). Finding 1 is not covered because none of the fixtures trigger an index-time failure under skip mode; a test with duplicate -typeIdProperty values in skip mode would likely reproduce it.

[lvca]

Addressed in 5fb47df:

1. Ghost record on index-time failure (Confirmed) - fixed for both CSV paths. You were exactly right: loadDocuments/loadVertices shared one transaction for the whole file, so a per-row catch that just logs-and-continues never undid a bucket write that happened before an index-time failure (duplicate key). Fixed by giving each row its own commit()/rollback() in skip mode - a failing row's rollback can never touch a previously committed one, regardless of how many rows came before it. Added csvVertexImportRollsBackGhostRecordOnDuplicateKeyWhenOptedIn and the document equivalent, both reproducing a real duplicate-key failure and asserting countType() matches the indexed count exactly (this is how I caught it: the naive fix left countType() at 4 while lookupByKey only ever found 3).

Also confirmed and fixed the same gap in JSON's nested parseRecord()/parseArray() catches, which you flagged as the same issue: a nested sub-object can already have a bucket write by the time its own indexing fails. Real nested begin()/commit() (ArcadeDB does support a transaction stack) turned out to be the wrong tool here - each nested commit() is independently durable, so a successful nested sub-record would survive even if its enclosing top-level record later failed, which is a worse atomicity regression than the bug being fixed. Instead, a nested failure now sets a recordFailed flag threaded through parseRecord()/parseArray(), and parseRecords() discards the whole top-level record through its already-correct rollback path when it comes back set. This changes nested-failure granularity from "drop just the bad property" to "drop the whole record" - a larger blast radius, but it reuses the one rollback boundary that's actually safe. Updated the nested JSON test's expectations accordingly (Bob's whole Order is now dropped, not just his Customer).

2. Skip mode dropping periodic commits (minor/performance) - superseded by the fix above: skip mode now commits every row by necessity (that's what makes the rollback safe), so there's no separate batching knob to reintroduce; settings.commitEvery remains meaningful for loadEdges, which this PR doesn't touch.

3. Stale test doc/name - fixed: renamed csvVertexImportCountsAsyncPersistTimeFailureWhenOptedIn → csvVertexImportCountsPersistTimeFailureWhenOptedIn and rewrote the comment to describe the current synchronous-save mechanism instead of the abandoned setCommitEvery(1) approach.

Nit (broad catch RuntimeException) - acknowledged as an intentional bulk-importer trade-off, as you noted; left as-is.

Side note, not part of this PR: chasing the duplicate-key repro surfaced a preexisting, unrelated quirk - AbstractImporter creates the -typeIdProperty index using settings.typeIdPropertyIsUnique (default false) before loadVertices's own "ensure unique index" check runs, so that check's getIndexesByProperties(...).isEmpty() is already false and the intended unique index is silently never created unless the caller explicitly passes -typeIdUnique true. My first version of the vertex ghost-record test didn't reproduce the failure at all because of this. Didn't touch it here since it's orthogonal to -onRowError - happy to file it separately if useful.

14 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x.

[claude]

Review: fix(#5968) CSV/JSON importer skip-and-log mode

Nice work here — the commit history shows this went through several rounds of self-review already (ghost records, JSON reader desync, shared-state mutation), and the final design holds up well under a close read. A few things worth a look before merge:

Performance consideration (the one I'd weigh most)

In -onRowError skip mode, CSVImporterFormat.loadDocuments/loadVertices now commit()+begin() every single row, not just on failure - this replaces the previous single whole-file transaction (documents) / async-batched inserts (vertices) with per-row commits for every row, success or not. That's effectively commitEvery=1 for the entire import whenever skip mode is on, which is a meaningful throughput hit exactly in the "large bulk import" scenario this PR is meant to make more tolerable (per the PR description: "expensive to iterate on for large imports"). It's a documented, deliberate trade-off (the comments explain the ghost-record and async-batch-rollback reasoning well), but it might be worth:

• calling this out explicitly in user-facing docs/--help output for -onRowError, since a user might expect "skip mode" to only cost extra on the row that actually fails, not on every row
• or, as a possible follow-up, batching N rows per transaction in skip mode and only falling back to per-row retry when a batch commit fails - keeps the "skip only loses the bad row" guarantee for the common case while avoiding an always-on commit-per-row penalty

Not a blocker, but given CLAUDE.md's performance mantra for this repo, it'd be good to have a quick before/after throughput number for a large CSV in skip mode vs. abort mode in the PR description.

Minor / polish

1. -onRowError silently doesn't apply to edges. loadEdges always skips-and-logs regardless of the setting (documented in a comment as intentional, since unresolved from/to references are expected). That's a reasonable behavior, but it means the setting's name/semantics are narrower than they read at the CLI level - worth a one-line mention in ImporterSettings.onRowError's javadoc (currently only documented on isSkipOnRowError(), and only in terms of the CSV-vertex-async trade-off) so it's clear the flag governs document/vertex rows only.

2. Test coverage gap for the SQL path. The PR description calls out IMPORT DATABASE ... WITH onRowError=skip as a supported entry point (via ImportDatabaseStatement → setSettings → parseParameter), but Issue5968ImporterSkipOnRowErrorTest only exercises the CLI-arg (new Importer(String[])) path. Worth adding at least one test that goes through the SQL IMPORT DATABASE statement to lock in that integration point, since it's reflection-based (clazz.getMethod("setSettings", Map.class)) and wouldn't fail loudly at compile time if it broke.

3. Slightly loose assertion in jsonNestedObjectImportSkipsWholeRecordAndKeepsProcessingSiblingsWhenOptedIn. It asserts result.get("errors")).isNotNull(), whereas every sibling test in the file asserts an exact count (.isEqualTo(1L)). Tracing through parseRecords()/parseRecord(), the nested-failure path should deterministically increment context.errors exactly once (the nested catch sets recordFailed but doesn't itself increment; the outer parseRecords() catch increments once when it sees the flag). Tightening this to .isEqualTo(1L) would make it as precise a regression guard as its neighbors and would catch any future double-counting bug in the nested-failure path.

4. Edge case: transaction left open on IOException inside the skip-mode per-row loop. The per-row try/catch in loadDocuments/loadVertices only catches RuntimeException. If the CSV parser itself throws IOException mid-loop (malformed input) while a per-row transaction is active (skip mode), that transaction is neither committed nor rolled back before the exception propagates to the outer catch (final IOException e) → ImportException. Importer.closeDatabase() will then unconditionally commit() on the way out (pre-existing behavior, not new to this PR) rather than roll back. Given skip mode's whole premise is "never let anything from an errored row ride into a commit," this is a narrow edge case, but it's a real inconsistency with the invariant the rest of the PR carefully protects. Might not be worth solving now, but flagging since it's adjacent to what this PR is specifically hardening against.

What looks solid

• The JSON reader-desync fix (catching at every nested BEGIN_OBJECT/BEGIN_ARRAY recursion site, since createRecord()/.save() only throws after reader.endObject()/endArray() already ran) is correct and well-reasoned - traced through the recursion and the invariant holds.
• The ghost-record fix (per-row transaction boundaries in skip mode instead of one file-wide transaction) is correct: a rollback on one row's own transaction can never touch a previously committed row.
• Good use of AtomicLong/AtomicReference/AtomicBoolean where shared across the async callback thread vs. the main import thread.
• Default (abort) behavior is preserved for backward compatibility, and the settings validation rejects invalid -onRowError values eagerly as intended.
• Test coverage is thorough for the documented scenarios (out-of-range values, missing mandatory properties at async persist time, duplicate keys/ghost records, nested JSON failures) - good regression tests for each of the five self-review rounds.

Overall this is in good shape; the performance trade-off is the main thing I'd want surfaced/acknowledged explicitly before merge, everything else is minor.

[lvca]

Addressed in a1215f7:

Performance trade-off - not changing the mechanism (per-row commit is what makes the correctness guarantee from round 4 hold), but made it much more visible: ImporterSettings.isSkipOnRowError()'s javadoc now spells out explicitly that skip mode is effectively commitEvery=1 for the whole run, not just the failing row, and why (index-time failures already have a bucket write by the time they're caught, so only that row's own transaction rollback can undo it without risking an unrelated committed row). On the "batch N rows, retry individually on failure" follow-up: I agree it's the right longer-term direction, but it's a real design task on its own (a batch commit failure doesn't tell you which row in the batch was bad without redoing per-row validation anyway, largely re-deriving the per-row logic this PR already has) - leaving it as a follow-up rather than scope-creeping this PR further.

Minor 1 (onRowError doesn't apply to edges) - documented directly on the onRowError field's javadoc now, not just on the derived isSkipOnRowError() method and the loadEdges code comment.

Minor 2 (SQL IMPORT DATABASE path untested) - added sqlImportDatabaseSupportsOnRowErrorSkipSetting, going through db.command("sql", "IMPORT DATABASE ... WITH onRowError=skip") instead of the CLI-arg constructor, to lock in that reflection-based forwarding path.

Minor 3 (loose isNotNull() assertion) - tightened to .isEqualTo(1L), matching every sibling test.

Minor 4 (IOException mid-loop leaving a dirty transaction) - traced through this and it's a non-issue by construction, so no code change: the only other throwing call in the row loop is csvParser.parseNext(), which only runs at the top of each iteration - i.e. right after the previous row's transaction was already committed or rolled back and a fresh, empty one begun via the post-catch/post-success database.begin(). There's no window where a parseNext() IOException can coincide with a transaction holding a partial row write. Added a comment explaining this so it doesn't get re-flagged, but left the code as-is.

15 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

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: -onRowError skip importer option (#5974)

Nice feature and the writeup is thorough. I traced the transaction/commit boundaries in detail and found what I believe is a real, reproducible correctness bug in the reported statistics (not in the persisted data itself), plus a couple of style nits.

Bug: createdDocuments/createdVertices counters get inflated on a skipped row

The counters are incremented before the point where a skip-mode rollback can still discard the record, in both the CSV and JSON paths. The database content ends up correct (as your "ghost record" tests confirm), but the errors/createdDocuments/createdVertices summary returned by Importer.load() (and surfaced through IMPORT DATABASE) can overcount actual persisted records.

CSV path (CSVImporterFormat.loadVertices, mirrored in loadDocuments):

v.save();
context.createdVertices.incrementAndGet();   // <-- counted here
database.commit();                           // <-- but can still throw here
database.begin();

Per-transaction unique-index checks are split: LSMTreeIndex/TransactionIndexContext only validates intra-transaction duplicates synchronously during save(). A duplicate against already-committed data (exactly the scenario your own csvVertexImportRollsBackGhostRecordOnDuplicateKeyWhenOptedIn test exercises) is only detected in TransactionIndexContext.checkUniqueIndexKeys(), called from database.commit(). So for that test, v.save() for "BobDuplicate" succeeds, the counter is bumped, and only then does commit() throw and get caught/rolled back — the row is correctly absent from the database, but createdVertices still counts it. The test doesn't assert on result.get("createdVertices"), so this slipped through.

JSON path (JSONImporterFormat.createRecord, integration/src/main/java/com/arcadedb/integration/importer/format/JSONImporterFormat.java:417-437):

record = database.newDocument(typeName);
context.createdDocuments.incrementAndGet();  // <-- counted here, before...
...
record.set(recordProperties);                // <-- ...the SHORT-overflow conversion actually throws

This is worse than the CSV case because the increment happens before the property values are even applied, not just before commit. Your own jsonNestedObjectImportSkipsWholeRecordAndKeepsProcessingSiblingsWhenOptedIn test triggers this: Bob's nested Customer gets newDocument() + counter increment, then record.set() throws on the out-of-range age, recordFailed propagates up, and the whole Order record (Bob's) is rolled back in parseRecords(). context.createdDocuments still counts the phantom Bob Customer (and would count a rolled-back top-level record too, for a top-level-record failure). The test only asserts db.countType(...), not the returned counters, so it doesn't catch it.

Suggested fix direction: only increment createdDocuments/createdVertices after the enclosing transaction actually commits (e.g. move/aggregate the CSV increments to after database.commit() returns; for JSON, increment in parseRecords()'s success path instead of deep inside createRecord(), or track counts per-record and add them to the context only once recordFailed is confirmed false and database.commit() succeeds).

Minor style nit

A few comments reference the PR review process directly, e.g. in JSONImporterFormat.java:

// ... (see #5974 review) ...
// ... (see #5974 review, round 2) ...

and in the new test's Javadoc: "Regression test for the PR #5974 review finding (round 4, "Confirmed")...". Per this repo's CLAUDE.md guidance, comments shouldn't reference the current fix/task/review since that context belongs in the PR description and rots over time. Worth rephrasing these to describe the invariant itself (which the comments otherwise do very well) rather than citing the review round that found it.

What looks solid

• Transaction boundary reasoning for the "no partial batches" guarantee (per-row commit in skip mode, sync vertex save instead of database.async()) is correct and well tested via the duplicate-key "ghost record" tests.
• JsonReader desync analysis for nested object/array failures is correct — catching at each recursion call site so endObject()/endArray() always run before a thrown exception is caught.
• Edges are deliberately excluded from -onRowError (they already skip-and-log unconditionally) — reasonable, documented in the field Javadoc.
• Backward compatibility: default abort preserves current behavior; invalid -onRowError values rejected eagerly.
• Good coverage: abort-vs-skip pairs for CSV vertices/documents and JSON, async persist-time failures, duplicate-key "ghost record" cases, nested JSON mapping, and the SQL IMPORT DATABASE ... WITH onRowError=skip integration point.

Suggestion for test coverage

Given the above, it'd be worth adding an assertion on result.get("createdVertices")/result.get("createdDocuments") in the duplicate-key and nested-failure tests you already have — they're the exact scenarios that expose the counter inflation, just not currently asserted.

No security or performance concerns beyond what's already called out in the PR description (skip mode trading batch throughput for per-row isolation, which is opt-in and clearly documented).

[lvca]

Addressed in 0867a43:

Counter overcount bug (real, confirmed) - fixed in both CSV and JSON paths.

• CSV loadDocuments/loadVertices: moved the createdDocuments/createdVertices increment to after database.commit() succeeds in skip mode (previously right after save(), before the duplicate-key check that only runs at commit time via TransactionIndexContext.checkUniqueIndexKeys()).
• JSON: createRecord() counts on allocation, before properties are even set - deep inside a recursive call chain that's harder to reorder safely. Instead of threading counts through parseRecord()/parseArray()/createRecord()/convertMap(), parseRecords() now snapshots context.createdDocuments/createdVertices/createdEdges before attempting each top-level record and resets them to the snapshot on any rollback (a direct catch or a nested recordFailed) - this correctly undoes increments from nested records too, regardless of how deep the failure occurred, without touching the allocation-time increment sites themselves.
• Added result.get("createdVertices"/"createdDocuments") assertions to the duplicate-key ghost-record tests and the nested-JSON test - exactly the scenarios you identified as slipping through since only countType() was asserted before.

Style nit (comments referencing "#5974 review"/"round N") - removed all of them from source comments and test Javadoc, rephrased to describe the invariant itself per CLAUDE.md's guidance that fix/task/review context belongs in the PR, not in code.

15 tests in Issue5968ImporterSkipOnRowErrorTest (now with counter assertions) plus the existing importer suites pass, repeated 3x.

[claude]

Review

Overall this is a well-engineered fix: the CSV/JSON per-row skip logic correctly reasons through transaction boundaries (per-row commit/rollback to avoid "ghost" bucket-only records on duplicate-key/commit-time failures), JsonReader stream positioning after nested failures, and counter rollback on a failed record. Test coverage is strong (abort/skip pairs, ghost-record regression, nested-object failure, SQL WITH integration, invalid-value rejection).

A few things worth a second look before merge:

1. Behavior change in the default (abort) path for CSV vertex import, not just the new opt-in path

loadVertices now registers an onError handler on database.async() that captures the first persist-time failure and, after waitCompletion(), rethrows it as an ImportException (CSVImporterFormat.java, the firstAsyncError block). Previously this handler only logged SEVERE and the import completed "successfully" even if some vertices silently failed to persist (e.g. a missing mandatory property caught only on the async worker thread, as exercised by the new csvVertexImportAbortsOnAsyncPersistTimeFailureByDefault test).

This is arguably a correctness fix, and it's covered by a test, but it's a behavior change for the default mode that isn't mentioned in the PR summary (which frames the change as purely additive/opt-in and backward-compatible). Any existing pipeline that was unknowingly relying on "vertex import completes even if a few rows fail validation" will now see those imports abort. Worth calling out explicitly as an intentional fix in the PR description so it doesn't surprise anyone bisecting a newly-failing import job.

2. Skip mode collapses CSV vertex import to synchronous, batch-size-1 commits

Well documented in the code/Javadoc, but worth flagging explicitly in review: enabling -onRowError skip on loadVertices abandons database.async() batching entirely in favor of a synchronous save()+commit()+begin() per row. For large vertex files this is a large throughput regression relative to the default commitEvery-batched async path, not just "somewhat slower." That's a reasonable trade-off for a debugging/data-cleaning opt-in flag, but it might be worth a doc note (e.g. in -help output or the settings Javadoc) recommending the flag be used to find/fix bad rows and then turned back off for the bulk load, rather than left on permanently for production-sized imports.

3. -onRowError skip is silently a no-op for single-top-level-object JSON imports

In JSONImporterFormat.load(), when mapping == null (a single JSON object, not an array), the code comments correctly note there's no sibling record to continue with, so the flag has nothing to do there and any nested failure still aborts. That's a reasonable limitation, but from a user's perspective, setting -onRowError skip and having it silently do nothing for this import shape (vs. throwing/warning that it's unsupported for this source shape) could be confusing. Consider a one-time INFO/WARNING log when skipOnError is set but this path is taken, so users don't assume protection they don't have.

4. Minor: context.errors conflates several unrelated skip reasons

The new onRowError-skip increments reuse the existing context.errors counter, which is also incremented for pre-existing, unrelated conditions (missing @cat/@type in a mapping, unresolved edge endpoints via context.skippedEdges, etc. - see the many other context.errors.incrementAndGet() call sites in JSONImporterFormat). That's a deliberate choice per the PR description ("reuse the existing counter rather than adding a new one"), but it means the errors value in the returned summary map can't distinguish "N rows were skipped due to -onRowError" from other pre-existing soft-skip conditions. Not a blocker, just worth knowing if anyone downstream tries to alert on this number specifically for the new feature.

5. Style: very dense, repetitive ALL-CAPS block comments

The file already used all-caps inline comments before this PR, so the convention itself is consistent, but several of the new comments (e.g. the near-duplicate reasoning blocks in loadDocuments/loadVertices/parseRecords about per-row transaction boundaries and ghost records) are quite long and repeat the same argument three times across the file. Might be worth condensing to one canonical explanation (e.g. on ImporterSettings.isSkipOnRowError(), which already has a full Javadoc) with short pointers ("see isSkipOnRowError() Javadoc") at the other call sites, to keep the hot-path methods scannable.

Things that look solid:

• Per-row transaction isolation correctly prevents a failing row's rollback from touching a previously-committed row's data (verified by the ghost-record tests).
• The nested-JSON-object case correctly catches at each recursive call site rather than at the top level, so the JsonReader never desyncs mid-record - and a genuinely malformed JSON structure (an IOException, not caught anywhere in the chain) still aborts even in skip mode, which is the right call.
• Counter snapshot/restore in parseRecords (createdDocumentsBefore/etc.) correctly prevents overcounting from a record that gets allocated-then-rolled-back.
• No new dependencies, no leftover debug output, -onRowError value validated eagerly with a clear error message.

Nice test coverage overall - the abort/skip pairing per scenario plus the two ghost-record regression tests give good confidence in the transaction-boundary reasoning.

[lvca]

Addressed in 0277dfb:

1. Behavior change in the default abort path (not flagged in the description) - fair catch, this was under-communicated. Rewrote the PR description with a dedicated "Behavior change in the default (abort) path" section calling out that loadVertices's async persist-time-failure handling was a pre-existing silent-drop bug that this PR also fixes unconditionally (not gated behind -onRowError), so an import that used to "succeed" while silently dropping some vertices will now correctly abort.

2. Skip mode throughput trade-off - already documented in ImporterSettings.isSkipOnRowError()'s Javadoc; there's no -help/usage-text infrastructure in this module to extend, so no code change here, but it's now also stated plainly in the PR description.

3. -onRowError skip silently a no-op for single-top-level-object JSON - fixed: JSONImporterFormat.load() now logs a one-time WARNING when skip mode is set but the source is a single object with no -mapping, explaining there's no sibling record to recover into.

4. context.errors conflating skip reasons - acknowledged, no change (consistent with the earlier reply on this).

5. Repetitive ALL-CAPS comments - condensed the near-duplicate per-row transaction/ghost-record reasoning in loadDocuments/loadVertices down to short pointers at ImporterSettings.isSkipOnRowError()'s Javadoc (which already has the full explanation), keeping only what's specific to each method.

15 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x.

[claude]

Review

I read through the diff in ImporterSettings, CSVImporterFormat, JSONImporterFormat, and the new Issue5968ImporterSkipOnRowErrorTest. Overall this is a well-scoped, carefully-reasoned change: the per-row transaction boundary in skip mode, the "roll back the whole nested JSON record instead of leaving a partial write" logic, and the async persist-time failure fix for vertices are all correct as far as I can trace them, and the test suite is unusually thorough (abort vs skip, ghost-record/duplicate-key rollback, nested JSON failures, the SQL IMPORT DATABASE ... WITH onRowError=skip entry point). The inline comments explaining why each transaction boundary is where it is are genuinely helpful for a change this subtle.

Potential bug: -onRowError skip doesn't protect against CSV-syntax-level parse errors

In both loadDocuments and loadVertices, the per-row try/catch only wraps the row-processing body, not the csvParser.parseNext() call that produces the row:

for (long line = 0; (row = csvParser.parseNext()) != null; ++line) {
  ...
  try {
    // build + save
  } catch (final RuntimeException e) {
    if (!skipOnError) throw e;
    ...
  }
}

parseNext() runs in the for loop's condition, outside the try. univocity-parsers' TextParsingException (thrown for things like a value exceeding maxCharsPerColumn/maxColumns, or an unterminated quoted field) is a RuntimeException, so a syntactically-bad row will propagate straight out of loadDocuments/loadVertices and abort the whole import even with -onRowError skip set. That's arguably exactly the "malformed row" case the PR description calls out ("a single malformed or out-of-range property value aborted the entire bulk import job"), so it's worth at least a conscious decision (and maybe a doc note) rather than being an accidental gap. The comment above skipOnError in loadDocuments only reasons about IOException from parseNext(), not about the RuntimeException case, which suggests this wasn't considered.

By contrast, JSONImporterFormat.parseRecords doesn't have this asymmetry: the whole parseRecord(...) call (including all of the reader's scalar-token calls, not just the nested-object/array recursion) is inside the per-record try, so any RuntimeException from the reader is caught at the record boundary and respects -onRowError skip. Might be worth bringing CSV in line, e.g. by restructuring the loop so parseNext() is called inside the try (with a way to continue on failure while still advancing to the next row).

Minor / non-blocking

• context.errors bookkeeping is solid for the parseRecords path (single increment point, counters snapshotted/restored on rollback), but it does implicitly assume parseRecords/parseRecord are never invoked concurrently for the same ImporterContext. That's true today (no parallelism inside the JSON reader loop), just flagging it as an invariant that a future change (e.g. splitting a large JSON array across worker threads) could silently break, since context.createdDocuments.set(before) isn't a CAS.
• JsonlImporterFormat (JSON Lines) has its own independent parsing path and doesn't get -onRowError skip support from this PR. Probably fine as a deliberate scope cut given the PR is framed as CSV+JSON, but might be worth a one-line mention in the PR description/issue if it isn't already planned as a follow-up.
• In loadVertices, when skipOnError is true, database.begin() is called before the try (InputStreamReader ...) block (i.e., before the file is even opened). If opening/reading the source throws IOException right away, that transaction is never explicitly committed or rolled back inside this method; it's left for whatever wraps Importer.load() to clean up. Low-severity (empty transaction, and it looks like existing code elsewhere in this class already relies on the caller to reconcile transaction state), but worth a second look given how carefully the rest of this PR handles transaction boundaries.

Nit

The new inline comments are long, ALL-CAPS paragraph blocks (matches the file's existing short-comment convention in spirit, but these are much longer). They're genuinely useful given how non-obvious the transaction semantics are, so I wouldn't cut them, just flagging the density in case a shorter Javadoc-style summary was preferred for future maintainers skimming the method.

Nice work on the ghost-record fix in particular, that's a subtle correctness issue to catch and the regression test (csvVertexImportRollsBackGhostRecordOnDuplicateKeyWhenOptedIn) pins it down well.

[lvca]

Addressed in 585166b:

CSV-syntax-level parse errors not caught (the main point) - investigated this empirically rather than guessing, since the fix here hinges entirely on whether univocity-parsers can safely resume after a TextParsingException. Wrote a throwaway probe (CsvParser directly, a value exceeding maxCharsPerColumn on the middle row of a 3-row file, catch-and-continue calling parseNext() again): the parser did not cleanly resume at the next line. It returned a truncated, wrong row for what should have been the row right after the failure, and then silently dropped the following good row entirely - with no exception, no signal, nothing landing in context.errors. That's a worse outcome than aborting: a "skip" that silently corrupts/loses an unrelated good row defeats the entire point of this feature.

Given that, I intentionally left csvParser.parseNext() outside the per-row try/catch and documented why (with the empirical finding) in both loadDocuments and loadVertices. Row-content validation failures (out-of-range values, missing mandatory properties, duplicate keys) are skippable, as already covered; CSV-syntax-level structural errors still abort even in skip mode, same as JSONImporterFormat still aborts on a genuinely malformed JSON structure (an IOException) rather than trying to resync mid-token. Happy to revisit if there's a known-safe way to resync univocity-parsers after a TextParsingException that I'm not aware of, but I'd rather under-promise here than ship a silent-data-loss path.

loadVertices leaked-transaction edge case - fixed: moved the skip-mode database.begin() to after the source file is successfully opened (beginParsing() succeeds), so an IOException from opening the file can no longer leave a transaction dangling for the caller to reconcile.

Minor points - context.errors single-threaded assumption and JsonlImporterFormat scope cut: both acknowledged as correct observations, no code change (JSONL is a genuinely separate parsing path and out of scope for a CSV/JSON-focused fix; happy to file it as a follow-up if there's interest). Comment density nit: left as-is per your own note that you wouldn't cut them given how non-obvious the transaction semantics are.

15 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x.

[claude]

Review

Reviewed the diff and the current state of ImporterSettings, CSVImporterFormat, and JSONImporterFormat (this branch is checked out locally, so I read the merged files directly rather than just the patch). I was not able to run mvn in this sandbox, so this is a static review only - the PR description states the full suite was run 3x, which I have to take on faith for runtime behavior.

Overall this is a well-scoped, carefully iterated change (9 commits progressively closing gaps: ghost records, overcounting, reader desync, leaked transactions). The pre-existing bug fix (async vertex persist-time failures being silently swallowed even in default "abort" mode) is valuable independent of the opt-in feature. A few things worth a look before merge:

1. Skip mode's per-row commit()/begin() cycle assumes it exclusively owns the transaction

In both CSVImporterFormat.loadDocuments and loadVertices, the guard before the loop is:
```java
if (!database.isTransactionActive())
database.begin();
```
This tolerates being called with an already-active transaction (e.g. via the Importer(Database, String) embedding constructor, or if IMPORT DATABASE ever runs from inside a caller-managed transaction). But once inside the loop, skip mode unconditionally does database.commit(); ...; database.begin(); per row. If the transaction was inherited rather than begun here, the very first row's commit() commits whatever else was pending in that inherited transaction too, not just this row - silently breaking the atomicity the caller may have been relying on.

Is it guaranteed today that loadDocuments/loadVertices are never entered with a pre-existing active transaction when -onRowError skip is set? If not, worth either asserting no active transaction before entering skip mode, or documenting the constraint, plus a regression test exercising -onRowError skip from inside an already-open transaction.

2. Counter snapshot/restore in JSONImporterFormat.parseRecords uses an absolute .set(), not a delta

```java
final long createdDocumentsBefore = context.createdDocuments.get();
...
context.createdDocuments.set(createdDocumentsBefore);
```
This overwrites the shared AtomicLong with a stale snapshot rather than subtracting the delta this record contributed. It's safe today only because JSON record parsing is strictly single-threaded against one ImporterContext (confirmed - Importer.load() calls loadFromSource for url/documents/vertices/edges sequentially on one thread, and nothing else writes these counters concurrently while JSON parsing runs). But it's a fragile pattern: if this context is ever shared across concurrent workers, one thread's rollback would silently erase another thread's legitimate increments. A local delta counter subtracted via addAndGet(-delta) would be safe regardless of future concurrency changes, at basically no cost today.

3. Blanket catch (RuntimeException e) in skip mode can't distinguish "bad row" from "systemic failure"

A disk-full condition or other non-data-related RuntimeException from save()/commit() gets treated exactly like a bad value and skipped - the import would burn through the rest of the file failing (and paying the per-row-transaction cost for) every remaining row instead of failing fast. Given the feature is opt-in and this is called out as a throughput trade-off already, this is more of a note than a blocker, but might be worth a line in the CLI help/docs.

4. A few edge cases described in the PR body aren't covered by regression tests

• CSV-syntax-level parse failures (a univocity TextParsingException) still aborting even in skip mode.
• A genuinely malformed JSON structure (IOException) still aborting in skip mode.
• Multiple consecutive bad rows in skip mode (only single-bad-row scenarios are exercised).

These are exactly the kind of edge case that's easy to regress later given how much the transaction-boundary logic changed across the 9 commits; a test locking in each would help.

What looks solid

• Ghost-record prevention (per-row transaction in skip mode) and the overcounting fix (incrementing counters only after commit() succeeds) are correct and match their regression tests.
• Nested JSON failure handling correctly threads recordFailed through the recursion so a failure at any depth discards only the enclosing top-level record without desyncing the reader for siblings - verified by tracing parseRecord/parseArray call sites against the JsonReader token consumption.
• Good default-preserving design: abort remains the default, so this is opt-in for existing pipelines aside from the (correctly called out) async-vertex-failure correctness fix.

[lvca]

Addressed the one concrete gap in 97a7e2f; the other three are either recurring themes or explicitly framed as non-urgent.

2. JSON's weaker abort-mode atomicity missing from release notes (fixed) - confirmed and agreed: unlike CSV documents' single whole-file transaction, JSON's per-record commit (unconditional, not gated on skip mode) means a later record's failure doesn't roll back earlier successfully-imported ones, even in default `abort` mode. This was already locked in by `jsonDocumentImportAbortsWithoutDiscardingCallersPendingWorkInExternallyManagedTransaction` but never called out in the docs alongside the equivalent CSV vertex caveat. Added a paragraph mirroring it.

1. Comment density - same recurring theme from many earlier rounds, already weighed repeatedly; not doing another pass.

3. `context.errors` overloading - explicitly framed as "worth considering as a fast-follow if `-onRowError skip` sees real usage," i.e. future work outside this PR's scope, not something to act on now.

4. `stopParsingQuietly` swallowing on the success path too - explicitly "probably fine, just flagging for awareness," no concrete ask.

Verified: no code changes this round (docs-only), so the previous verification (36 tests, 3x clean runs, broader importer suites, full compile check) still stands unchanged.

[claude]

Review

Reviewed the diff (AbstractImporter, Importer, ImporterContext, ImporterSettings, CSVImporterFormat, JSONImporterFormat, and the new Issue5968ImporterSkipOnRowErrorTest). Note: I wasn't able to run mvn verify/the new test class in this sandbox (build commands weren't approved), so this is a static read rather than a confirmed green build — please make sure CI is green before merge.

Strengths

• The transaction-ownership bookkeeping (callerTransactionActiveOnEntry / ownsTransaction / beginRowTransaction) is genuinely tricky (self-managed vs. externally-managed Database, nested vs. reused transactions, schema-auto-creation side effects) and, as far as I traced it by hand, is internally consistent: JSONImporterFormat.parseRecordsArray's trailing database.commit() correctly balances the per-record begin()/commit() pairs plus the entry-level begin() in parseRecords, so a caller-owned transaction really is left untouched on both the success and abort paths. Nice catch on the ghost-record case (bucket write surviving an index failure) and the pre-existing SEVERE-only async vertex failure silently "succeeding" - both are real bugs worth fixing and are pinned down with regression tests (csvVertexImportRollsBackGhostRecordOnDuplicateKeyWhenOptedIn, csvVertexImportAbortsOnAsyncPersistTimeFailureByDefault).
• Test coverage is unusually thorough: default-vs-skip for CSV documents/vertices/edges and JSON (top-level object, array, nested object/array), multi-batch async partial-success semantics, caller-transaction preservation on both success and failure, and the SQL IMPORT DATABASE ... WITH onRowError=skip integration point (including the atomic-transaction rejection case that mirrors the HTTP handler). That's exactly the kind of regression coverage this change needs given how subtle the transaction semantics are.

Possible issue: stale database.async().onError() handler on externally-managed Database

CSVImporterFormat.loadVertices (around line 381) registers a new database.async().onError(...) handler in abort mode that mutates context.errors and firstAsyncError - both captured from this call's own ImporterContext. The comment at line 377 notes onError() replaces rather than stacks, and nothing restores/clears the handler after loadVertices() returns (success or failure). For the embedding entry point (Importer(Database, String)), the caller keeps using the same live Database after importer.load() returns. If the caller (or another part of the app) triggers unrelated database.async() writes after this import finishes and before any later import re-registers its own handler, those failures will get funneled into this now-returned, presumably-no-longer-inspected ImporterContext instead of whatever the caller's own error handling expects. This existed in weaker form before (just a SEVERE log), but now it has real side effects on shared context state. Worth a finally that restores the previous handler (or at least clears it) after loadVertices() completes, particularly for the externally-managed-Database case.

Nits

• Comment density: several methods (notably loadDocuments/loadVertices in CSVImporterFormat, parseRecords/parseRecordsArray in JSONImporterFormat, and the Javadoc on ImporterSettings.isSkipOnRowError()/ImporterContext#callerTransactionActiveOnEntry) carry paragraph-length comments on nearly every statement. The reasoning is valuable given how non-obvious the transaction semantics are, but CLAUDE.md's guidance for this repo is to default to no comments and add one only where the WHY is non-obvious - at this density the comments start to compete with the code for attention and will be easy to let drift out of sync on the next change. Consider trimming to the truly load-bearing invariants (e.g. "commit() is only correct here because X") and moving the broader design narrative into a single class-level note or the PR/release-notes doc, which already covers this well.
• attachConcurrentAsyncError (CSVImporterFormat.java:271-273) compares Throwables with .equals(). Throwable doesn't override equals, so this is reference equality in practice, but == would state the intent (identity, not value equality) more clearly.
• The errors AtomicLong in ImporterContext now conflates several unrelated non-fatal conditions (skipped rows via onRowError=skip, pre-existing nested-JSON-conversion failures, etc.), which the release notes for this PR already call out as a limitation ("there is no separate counter for -onRowError skip specifically"). Not a blocker since it's documented, but a dedicated skippedRows counter alongside errors would make the import summary more actionable for callers who want to alert on skip-mode specifically rather than grep log lines.

Performance/behavior change callout (already well documented)

The default-path behavior change for CSV vertex imports (async persist-time failures now abort instead of silently logging at SEVERE) and the throughput cost of -onRowError skip (batch size effectively drops to 1, vertices lose database.async() parallelism entirely) are both called out clearly in the release notes and Javadoc. Just flagging that this is a real behavior change on the default path or existing users who may be unknowingly relying on "import completes even if some vertices fail validation" - worth making sure it's prominent in the actual release announcement, not just the docs file.

No blocking correctness issues found beyond the stale-handler question above, which I'd treat as worth confirming/fixing but not necessarily blocking if the team judges the embedding-after-import-reuse scenario unlikely in practice.

[claude]

Review

I read through the full diff (both parent commits of the merge are checked out locally in this environment) and traced the underlying LocalDatabase.begin()/commit()/rollback() nesting semantics against ImporterContext, ImporterSettings, CSVImporterFormat, and JSONImporterFormat to verify the claims made in the PR description hold up in the actual code, not just the commit messages.

Overall

This is an unusually well-engineered change. The transaction-ownership reasoning is subtle (self-managed vs. externally-managed Database, schema-auto-creation transactions vs. caller transactions, async batch rollback blast radius, JSON reader desync on nested failures) and the code matches the extensive Javadoc/inline comments exactly. I verified several of the trickier claims directly against LocalDatabase:

• begin() really does push a nested TransactionContext when one is already active (LocalDatabase.java:536-540), and commit()/rollback() only pop/act on the last one (popIfNotLastTransaction()), which is the load-bearing assumption behind JSONImporterFormat's "always nest, never touch the caller's level" strategy.
• parseRecord() really does call reader.endObject() (line 340) before createRecord() (line 347), which is what makes catching RuntimeException at each nested BEGIN_OBJECT/BEGIN_ARRAY call site (rather than deeper) safe from reader desync.
• options.put(name, value) in ImporterSettings.parseParameter() runs unconditionally after the switch, so settings.options.containsKey("commitEvery") in the new vertex skip-mode warning correctly detects an explicitly-set value.

I did not find a correctness bug in the core logic. The test suite (Issue5968ImporterSkipOnRowErrorTest, 36 test methods) is genuinely comprehensive - it covers the cross product of CSV/JSON x document/vertex x abort/skip x self-managed/externally-managed database x top-level/nested x sync/async failure that a change this deep in transaction handling needs.

Minor / non-blocking notes

1. Complexity concentration in CSVImporterFormat. TransactionOwnership, computeTransactionOwnership(), beginRowTransaction(), and the ownsTransaction/transactionActiveOnEntry threading add real cognitive load to loadDocuments/loadVertices. It's justified by genuine bugs found (dangling schema-creation transactions, caller-transaction hijacking), and it's thoroughly commented, but it's now one of the more intricate state machines in the importer module. Worth keeping in mind for whoever maintains this next, not asking for a rewrite given how well it's tested.

2. attachConcurrentAsyncError's identity check via .equals() (CSVImporterFormat.java, the !asyncError.equals(target.getCause()) / !asyncError.equals(target) checks): since Throwable doesn't override equals(), this is intentionally a reference-identity check (done this way to satisfy an ErrorProne lint per the commit history). A one-line comment at the call site noting why equals() is used for what's really an identity comparison would save a future reader from "simplifying" it into a real equality-by-message check.

3. Two different transaction-ownership strategies for the same conceptual problem (CSV reuses/commandeers the active transaction once ownership is established; JSON always nests). This is explicitly documented via contrasting class-level Javadocs on both formats, so it's a deliberate, explained choice rather than an oversight - just flagging it as the kind of divergence that's easy to lose track of if a third format importer is ever added.

4. Behavior change in the default abort path is significant and well-flagged - a CSV vertex import that previously "succeeded" while silently dropping vertices on async persist-time failures will now throw. This is correctly called out in both the PR description and docs/release-26.9.1.md; worth double-checking any existing internal import pipelines/docs/examples that might rely on the old silent-drop behavior before this merges, since it's a real behavior change for users who never touch -onRowError.

Performance

Skip mode's cost (per-row transaction, synchronous vertex saves losing database.async() parallelism) is opt-in and clearly documented, including the fact that -commitEvery/-parallel become silently inapplicable to vertices while it's enabled (with a log line calling that out). The default abort path's added overhead is limited to the failure paths (extra waitCompletion()/rollback bookkeeping); the success path looks unchanged in cost.

Security

No concerns - -onRowError values are validated eagerly against an allowlist (abort/skip), and the new exclusive-transaction-ownership guard actually closes a real hazard (skip mode silently committing/discarding a caller's unrelated pending transaction).

Nice work on the test coverage and on documenting the non-obvious invariants inline, this would have been very easy to get subtly wrong.

[lvca]

Addressed in 710b913 and 5eee805.

Stale database.async().onError() handler on externally-managed Database (fixed, 710b913) - confirmed the reviewer's read against DatabaseAsyncExecutorImpl.onError(): it's a plain field assignment (onErrorCallback = callback;), and there's no getter to save/restore whatever was registered before loadVertices() runs. For the Importer(Database, String) embedding constructor, that meant a leaked handler could keep routing the caller's own later, unrelated database.async() failures into this call's now-stale ImporterContext. Added a handlerActive AtomicBoolean: the handler closure returns immediately once it's false, and the finally block flips it false only after this method's own async work has already been drained via waitCompletion() on every path that reaches it - so it can only suppress reactions to later, unrelated work, never miss one of this import's own errors. Added csvVertexImportDeactivatesAsyncErrorHandlerAfterCompletingSoLaterUnrelatedFailuresAreNotMisattributed, which runs an import to completion, then triggers a separate async failure directly against the same Database and confirms it isn't attributed to the finished import. Verified non-vacuous: with the guard removed it fails (expected: 1L but was: 2L).

Comment density (fixed, 5eee805) - valid per CLAUDE.md's default-to-no-comments guidance. Trimmed the paragraph-length comments in loadDocuments/loadVertices/loadEdges (CSVImporterFormat) and load/parseRecords/parseRecordsArray/parseRecord (JSONImporterFormat) down to their load-bearing invariants, dropping restated cross-references and duplicated reasoning across symmetric branches (108→67 comment lines in CSVImporterFormat, 64→47 in JSONImporterFormat). Left ImporterSettings#isSkipOnRowError()'s and ImporterContext#callerTransactionActiveOnEntry's Javadocs as-is - both are already the single consolidated reference the trimmed inline comments point back to, which is exactly where this design narrative belongs per the reviewer's own suggestion.

.equals() vs == on Throwable in attachConcurrentAsyncError - this has gone back and forth across several rounds (Codacy/ErrorProne flagging raw !=, other reviewers preferring ==+suppression). Keeping .equals(): it satisfies the automated Codacy gate cleanly, and since Throwable doesn't override equals() the behavior is identical to == either way. Not changing again absent a concrete new reason.

Dedicated skippedRows counter - explicitly flagged by the reviewer as non-blocking and already covered by the release notes' documented limitation on context.errors conflating multiple non-fatal conditions. Leaving this as a possible separate follow-up rather than scope creep on this PR.

37 tests in Issue5968ImporterSkipOnRowErrorTest (up from 36) plus the existing importer suites pass, repeated 3x, plus a clean module install.

PR: #5974

[lvca]

Addressed in b352a89. This review found no correctness bugs and called the change "unusually well-engineered" - the four notes were all explicitly minor/non-blocking.

attachConcurrentAsyncError's .equals() used as identity check (fixed) - added a one-line comment at the call site: Throwable doesn't override equals(), so this is really a reference-identity check, done via .equals() instead of == to satisfy ErrorProne's Throwable-reference-equality lint (per prior review rounds). Exactly what was asked - a future reader won't mistake it for a same-message comparison and "simplify" it into one.

Complexity concentration in CSVImporterFormat - explicitly not asking for a rewrite ("justified by genuine bugs found... thoroughly commented"), just flagging maintenance cost. No action requested.

Two transaction-ownership strategies (CSV reuses, JSON nests) for the same conceptual problem - explicitly called "a deliberate, explained choice rather than an oversight" via the contrasting class-level Javadocs already in place. No action requested.

Default-abort-path behavior change worth double-checking against internal pipelines - this is a request to verify real-world usage/deployments aren't relying on the old silent-drop behavior, not a code change. That's outside what I can verify from the repo, so flagging it to the maintainer directly rather than acting on it.

37 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x, plus a clean module install.

PR: #5974

[claude]

Review

This is a well-scoped fix for #5968, and the transaction-ownership reasoning is unusually well documented (both in the release notes and in-code Javadoc/comments). I read through CSVImporterFormat, JSONImporterFormat, ImporterSettings, ImporterContext, AbstractImporter, Importer, and the new Issue5968ImporterSkipOnRowErrorTest suite. Overall this looks solid and thorough; below are a few things worth a second look before merge.

Correctness

1. context.createdDocuments can undercount relative to what actually persisted, in the caller-managed-transaction + "abort" mode case. In CSVImporterFormat.loadDocuments, on a mid-loop row failure with ownsTransaction == false (i.e. the database was handed in with an already-active caller transaction), the exception is rethrown without ever reaching context.createdDocuments.addAndGet(documentsCreatedThisFile) (lines ~192-197). Rows saved before the failing one (e.g. "Alice") remain durably in the shared transaction (per csvDocumentImportAbortsWithoutDiscardingCallersPendingWorkInExternallyManagedTransaction, which asserts db.countType("Widget", true)).isEqualTo(1)), but context.createdDocuments never gets credited for them. That test doesn't check the counter, so this discrepancy between "what's actually in the database" and "what the import summary reports" isn't locked in either way. Minor, edge-case-of-an-edge-case (only triggers when embedding + pre-existing transaction + default abort mode), but worth a conscious decision either way.

2. beginRowTransaction's "commit the ambient transaction, then start fresh" behavior is a real semantic change for self-managed databases, not just an implementation detail. For a self-managed DB, AbstractImporter.openDatabase() always leaves an ambient transaction active (its own database.begin() at the end), and beginRowTransaction now unconditionally commits that ambient transaction before starting the per-row loop whenever it's called with ownsTransaction == true (i.e., always for self-managed DBs). Previously, loadDocuments just checked if (!database.isTransactionActive()) database.begin(); and reused whatever was already open. This is well-tested for the "skip" + schema-auto-create scenario, but I didn't see an equivalent test asserting the analogous behavior is safe for "abort" mode + vertices (i.e., loadVertices's new if (ownsTransaction && database.isTransactionActive()) database.rollback(); in the per-row catch, which previously never existed at all for vertices - a synchronous v.set() failure used to propagate straight out with no rollback call). It's probably fine (schema mutations survive rollback per the doc comments), but a targeted regression test here (abort mode, fresh embedding constructor, lazily-created vertex type, first-row synchronous failure) would close the gap symmetrically with csvDocumentImportAbortsOnFirstRowFailureWhenSchemaAutoCreatedViaEmbeddingConstructor.

3. database.async().onError() replaces rather than stacks handlers, which the PR correctly identifies and works around with handlerActive - good catch. Just flagging that this is inherently fragile if another part of the codebase ever also calls database.async().onError() concurrently with an in-flight import on the same externally-managed Database (e.g., from another thread); the "last registration wins, first one silently stops reacting" semantics of the underlying API are a pre-existing landmine this PR works around locally but doesn't fix at the source. Not a blocker, just worth a comment/issue for the underlying async().onError() API if that's tractable separately.

Design / behavior-change risk

• The PR description is upfront about this, but worth re-flagging as a review point: the default (abort) path behavior change for CSV vertex imports (async persist-time failures now abort with ImportException instead of only logging SEVERE) is a legitimate correctness fix, but it is a breaking behavior change for any existing pipeline that relies on "vertex import completes even if some rows silently fail validation." Since this ships as part of a bug-fix PR rather than being gated behind the new opt-in flag, it's worth confirming this is intentional for the target release (it reads like it is, given the detailed release notes) and not something that should instead be opt-in symmetric to -onRowError skip.

Performance

• Correctly documented that -onRowError skip drops effective batching to 1 row/transaction for documents, and additionally forfeits database.async() worker-thread parallelism entirely for vertices. Since it's opt-in and off by default, this is an acceptable and well-communicated tradeoff.

Test coverage

• The new test file is extremely thorough (33 tests covering abort/skip x documents/vertices/edges/JSON, nested JSON failures, multi-batch survival, ghost-record prevention, caller-transaction isolation, handler-leak prevention, etc.). This is genuinely excellent regression coverage for a change this subtle.
• One gap noted above (Bump gremlin.version from 3.4.10 to 3.5.1 #2): no "abort mode + vertices + schema auto-create + first-row failure" test, to sit alongside the existing "skip mode" and "document abort mode" variants of that same scenario.
• Minor: attachConcurrentAsyncError uses Throwable.equals() for identity comparison (relies on Throwable not overriding equals, so it's reference equality in practice) - == would make the identity intent explicit and avoid relying on that implicit contract.

Nit

• CSVImporterFormat.loadDocuments's per-row catch (final RuntimeException e) calls database.begin() unconditionally after logSkippedRow/context.errors.incrementAndGet() in skip mode, with no guard against database.begin() itself throwing (e.g., resource exhaustion). Extremely unlikely to matter in practice, just noting for completeness since the rest of the PR is careful about failure-path symmetry elsewhere.

Nice work on the exclusive-transaction-ownership guard for -onRowError skip (rejecting it outright when a transaction is already active, e.g. under IMPORT DATABASE ... WITH onRowError=skip over HTTP) - that's the kind of edge case that's easy to miss and it's directly tested (sqlImportDatabaseRejectsOnRowErrorSkipInsideAtomicTransactionLikeHttpDoes).

[claude]

Review

I read through the full diff (ImporterSettings, AbstractImporter, Importer, ImporterContext, CSVImporterFormat, JSONImporterFormat) and the 1444-line regression test class, and traced the transaction-ownership logic by hand (couldn't run mvn verify in this sandboxed review environment, so please make sure CI is green before merge).

Overall

This is a very carefully engineered change. The transaction-ownership problem (per-row commit/rollback vs. not stepping on a caller-managed transaction) is genuinely tricky, and the PR handles it consistently across all four paths (CSV documents, CSV vertices, CSV edges, JSON records), with callerTransactionActiveOnEntry captured at the one reliable point (AbstractImporter#openDatabase(), before any schema auto-creation can touch transaction state). The pre-existing "vertex persist failures were only logged at SEVERE and silently swallowed" bug is a good catch and a real correctness fix independent of the opt-in feature.

Correctness

I couldn't find a scenario that breaks the invariants documented in the Javadoc/comments. A few things I specifically traced and confirmed hold up:

• Self-managed DB vs. externally-managed DB (embedding constructor / IMPORT DATABASE) are handled distinctly via ownsTransaction, and the guard against skip mode running inside an already-active caller transaction (newExclusiveTransactionRequiredException) is checked before any schema side effect (typeIdProperty/index auto-creation) in loadVertices, not after - so a rejected import doesn't leave schema artifacts behind.
• createdDocuments/createdVertices/createdEdges counters are only committed to context after the owning transaction's commit() succeeds (both CSV per-row skip mode and the JSON nested-record snapshot/restore), avoiding the overcount-then-rollback race a naive "increment right after save()" would hit.
• The async vertex path drains waitCompletion() on every exit path (including the new RuntimeException catch for a synchronous per-row failure) before propagating, so an externally-managed DB (no closeDatabase() safety net) can't return control with an in-flight, uncounted async write.
• beginRowTransaction's commit-then-begin-fresh behavior when ownsTransaction && transactionActiveOnEntry correctly handles the "schema auto-creation left a transaction open on entry" case, and there's a dedicated test for it.

Minor / nit-level observations

• The -commitEvery/-parallel-has-no-effect INFO notice in loadVertices checks settings.options.containsKey(...), which only catches values set via parseParameter (CLI args / WITH clause). A caller using the embedding API and setting importer.settings.commitEvery directly on the field won't trigger the notice, even though the setting is still silently ignored. Very low impact (just a missing log line), not worth blocking on.
• The comment density in CSVImporterFormat/JSONImporterFormat/ImporterSettings is unusually high for this codebase's normal style. It's justified here - the invariants really are non-obvious and the comments explain why, not what - but it makes the two format classes noticeably heavier to read. Worth a skim to see if any can be trimmed once the PR settles, not a blocker.
• Importer.closeDatabase() duplicates AbstractImporter.closeDatabase() verbatim (pre-existing, not touched by this PR) - unrelated to this change, just noting it's dead weight if anyone's cleaning up nearby.

Test coverage

Excellent - genuinely one of the more thorough regression suites for a change like this. It covers both formats, both modes, the ghost-record scenario (bucket write surviving without its index entry), multi-batch async survival semantics, nested JSON object/array failures without desyncing the reader, the HTTP-atomic-transaction rejection path via db.transaction(...), and the IMPORT DATABASE ... WITH onRowError=skip SQL integration point. Each test also carries a Javadoc explaining why the scenario matters, which will help when one of these fails in the future.

Docs / behavior-change communication

The release notes callout for the vertex "abort" default-path behavior change is exactly the kind of thing that should be flagged loudly, and it is - good execution here. The distinction drawn between the document path's whole-file atomicity and JSON's per-record atomicity in "abort" mode is also correctly caveated rather than glossed over.

No security or dependency concerns (no new dependencies; all changes are internal to the importer's transaction/error-handling flow).

[lvca]

Addressed in fe98d64. No correctness bugs found in this review either; two concrete, actionable points and several already-settled/non-blocking ones.

context.createdDocuments can undercount vs. what's actually staged, in the caller-managed-transaction + abort-mode-failure edge case (addressed by documenting the deliberate choice) - confirmed the mechanism: on a mid-loop failure with ownsTransaction == false, the throw skips the post-loop addAndGet(documentsCreatedThisFile) entirely, so rows saved before the failing one aren't credited even though they're still staged in the caller's own transaction. Made the conscious call the review asked for: this is correct as-is, not a bug - whether those staged rows ultimately become durable is the caller's own future commit/rollback decision on a transaction this import never controlled, so a failed import shouldn't take credit for them in its own summary (crediting them now could also overcount if the caller later rolls back for an unrelated reason). Documented the reasoning at the documentsCreatedThisFile declaration and added an assertion locking in the current (zero) count to csvDocumentImportAbortsWithoutDiscardingCallersPendingWorkInExternallyManagedTransaction, so this stays a deliberate choice rather than an untested discrepancy.

Missing "abort mode + vertices + schema-auto-create + first-row synchronous failure" test (added) - added csvVertexImportAbortsOnFirstRowFailureWhenSchemaAutoCreatedViaEmbeddingConstructor, symmetric to the document-side and "skip"-mode vertex-side coverage of the same scenario. Traced the actual failure mechanism to confirm it's genuinely synchronous: updateDatabaseSchema() infers Node.Id as LONG from the analyzed CSV data, so v.set("Id", "notanumber") throws IllegalArgumentException directly from the per-row try, before ever reaching database.async().createRecord(). Confirms the auto-created type/index survive and no vertex is durably counted.

database.async().onError() replace-not-stack as a "pre-existing landmine" - agreed this is a real characteristic of the underlying API, correctly identified as already worked around locally (handlerActive) rather than something this PR should fix at the source. Not acting on the suggested standalone follow-up issue for the API itself without confirming there's appetite for it - flagging to the maintainer.

Default abort-path behavior change worth confirming for the target release - already flagged to the maintainer in the previous round's reply; no new action.

Throwable.equals() vs == - already addressed last round with an explanatory comment (b352a89) after a different reviewer raised the same point; not changing again.

database.begin() unguarded against its own failure in the per-row catch - explicitly called "extremely unlikely to matter in practice, just noting for completeness." Every other begin()/commit()/rollback() call in this method has the same characteristic; guarding only this one would be inconsistent for a failure mode (resource exhaustion) that would take down the JVM/import regardless. Per this repo's CLAUDE.md guidance against validating scenarios that can't practically happen, not adding a guard here.

38 tests in Issue5968ImporterSkipOnRowErrorTest (up from 37) plus the existing importer suites pass, repeated 3x, plus a clean module install.

PR: #5974

[lvca]

No code changes this round - all three points are explicitly minor/non-blocking, and none warrant a change:

-commitEvery/-parallel no-op notice misses direct field sets via the embedding API - correctly identified: settings.options.containsKey(...) only catches values set through parseParameter (CLI args / WITH clause), not importer.settings.commitEvery = ... set directly on the public field. Explicitly flagged as "very low impact... not worth blocking on." A real fix would need settings-level explicit-set tracking (there's no way to distinguish "left at default" from "explicitly set to the same value as default" on a plain field) - that's infrastructure disproportionate to what it buys (one extra log line), so not adding it.

Comment density in CSVImporterFormat/JSONImporterFormat/ImporterSettings - already addressed in 5eee805 two rounds ago (108→67 comment lines in CSVImporterFormat, 64→47 in JSONImporterFormat), which another reviewer requested and this round's reviewer independently flagged as "worth a skim... not a blocker." Leaving it there: further trimming risks cutting into the genuinely non-obvious WHY context this reviewer's own "Correctness" section relied on to verify the invariants by hand.

Importer.closeDatabase() duplicates AbstractImporter.closeDatabase() - explicitly called out as pre-existing and untouched by this PR. Out of scope here; not folding in unrelated cleanup.

No test/behavior changes, so the previous round's verification (38 tests, 3x clean runs, broader importer suites, clean install) still stands unchanged.

PR: #5974

[claude]

Review

This is a thorough, well-engineered PR. The transaction-ownership reasoning (callerTransactionActiveOnEntry, per-row commit/rollback, the async→sync switch for vertices in skip mode) is careful and the accompanying comments/Javadoc make the non-obvious invariants explicit rather than leaving them implicit. The docs entry and test suite (38 @Test methods, not 27 as the PR description says, but close enough) cover the scenarios called out in the summary, including the trickier ghost-record and multi-batch-survival cases. I independently verified that the repeated if (ownsTransaction && database.isTransactionActive()) database.rollback() pattern used after a failed commit() is safe: TransactionContext.commit() rolls back internally and sets status INACTIVE before rethrowing on DuplicatedKeyException/NeedRetryException/etc., so isTransactionActive() correctly returns false afterward and the guard skips a redundant rollback rather than double-rolling-back or masking the original exception.

Potential issue: JSONImporterFormat.parseRecords()'s outer RuntimeException handler assumes an invariant the code doesn't actually enforce

In parseRecordsArray(), the loop condition itself sits outside the per-record try:

```java
while (reader.peek() == BEGIN_OBJECT) {
...
try {
... parseRecord() ...
database.commit();
} catch (final RuntimeException e) {
if (database.isTransactionActive())
database.rollback();
...
}
database.begin();
}
```

Back in parseRecords(), the wrapping catch relies on the assumption that any RuntimeException reaching it has already gone through the per-record catch (and thus already been rolled back):

```java
} catch (final RuntimeException e) {
// Any RuntimeException reaching here has already passed through parseRecordsArray()'s per-record catch, which
// (in "abort" mode, the only mode where it rethrows) already rolled back its own nested level. Don't roll back
// again here: that would discard the caller's unrelated pending work instead.
throw e;
}
```

That's true for exceptions raised inside parseRecord()/saveAnonymousRecord()/commit(), but reader.peek() in the while condition runs after database.begin() for the next record and before the try — if it ever throws a RuntimeException (as opposed to the IOException the sibling catch block handles), the transaction opened for that iteration is never rolled back here, and the exception propagates up with an active transaction left dangling on the Database. Gson's JsonReader.peek() is declared to throw IOException and in practice malformed input surfaces as MalformedJsonException/EOFException (both IOException), so this is unlikely to trigger today — but it's worth noting that CSVImporterFormat's equivalent loop (csvParser.parseNext(), also evaluated in the loop condition, outside the per-row try) does defend against exactly this case in its own outer catch (RuntimeException e):

```java
} catch (final RuntimeException e) {
// A source-level failure (parseNext()) escaped the loop and never went through the per-row catch above.
if (ownsTransaction && database.isTransactionActive())
database.rollback();
throw e;
}
```

For consistency (and defense-in-depth, since the JSON invariant isn't structurally guaranteed), consider mirroring that in JSONImporterFormat.parseRecords()'s RuntimeException catch: if (database.isTransactionActive()) database.rollback(); before rethrowing. Since parseRecords() is only reached when there's no caller-owned outer transaction to protect (the eager callerTransactionActiveOnEntry check up top, plus the fact that database.begin() always nests a fresh level per LocalDatabase#begin()), this rollback can't discard unrelated caller work the way an unconditional one legitimately could elsewhere — so no extra ownsTransaction-style guard condition is even needed here, just the isTransactionActive() check itself.

Minor / non-blocking

• loadDocuments()'s old, unconditional database.async().onError(...) registration + trailing database.async().waitCompletion() are removed in this PR. Worth double-checking that call was genuinely vestigial (document import already only ever used synchronous document.save(), never database.async()) rather than serving some other purpose — from reading the surrounding code it does look dead, so removing it looks correct, just flagging since it's an incidental cleanup riding along with the main change.
• The PR description says the new test class has "27 tests"; it's actually 38 @Test methods. Not a functional issue, just worth fixing in the description for future reference.

Strengths worth calling out

• The -onRowError skip requires-exclusive-control-of-the-transaction guard, checked eagerly before any schema side effects, is exactly the right place to fail loudly instead of silently doing the wrong thing inside a caller's transaction.
• Deferring context.createdDocuments/createdVertices increments until after a successful commit() (rather than right after save(), as the old code did) fixes a latent over-counting bug for the case where a duplicate key is only detected at commit time — this looks like a genuine correctness improvement beyond what's described in the PR summary.
• Good catch on the pre-existing CSV vertex SEVERE-log-and-continue bug; turning that into a hard ImportException is the right default-mode behavior even though it's a documented breaking change.

Nice work overall — the reasoning behind each transaction-ownership decision is unusually well-documented for a change this subtle.

[lvca]

Addressed in ebe6f53. Good catch on the dangling-transaction gap - worth disclosing how it went, since my first attempt was wrong.

JSONImporterFormat.parseRecordsArray()'s reader.peek() outside the per-record try (fixed, with a self-caught regression along the way) - my first attempt followed the suggestion literally: added if (database.isTransactionActive()) database.rollback(); to parseRecords()'s own outer catch, reasoning that this method always begins a fresh nested transaction and never touches a caller's. That reasoning turned out to be wrong: by the time a RuntimeException reaches that outer catch, parseRecordsArray()'s inner per-record catch may have already correctly rolled back its own level and popped the transaction stack down to an externally-managed Database's own, still-active caller transaction - rolling back again there discards the caller's unrelated pending work instead. Re-running the full regression suite before pushing caught this immediately: jsonDocumentImportAbortsWithoutDiscardingCallersPendingWorkInExternallyManagedTransaction failed with the caller's transaction wrongly gone.

Fixed at the actual source instead: reader.peek() now runs inside its own small try/catch at the top of the loop, where the active transaction is unambiguously this loop's own level (just begun via database.begin(), untouched by anything else this iteration) - safe to roll back unconditionally, with none of the ambiguity that broke the first attempt. The existing, already-correct per-record try/catch is untouched.

No dedicated regression test added for this specific path: as the review itself notes, Gson's JsonReader.peek() only throws IOException in practice, so this is defense-in-depth against a scenario not reachable through the public API today, not a currently-triggerable bug - consistent with not manufacturing tests for structurally-unreachable paths.

loadDocuments()'s removed database.async() registration - verified vestigial - confirmed loadDocuments has zero database.async() calls anywhere; it only ever used synchronous document.save(). The removal was correct dead-code cleanup, not a functional change.

PR description said 27 tests, actually 38 - fixed directly in the PR description.

38 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x, plus a clean module install.

PR: #5974

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 85.07% diff coverage

Metric	Results
Coverage variation	Report missing for f80265f1
Diff coverage	✅ 85.07% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (f80265f)	Report Missing	Report Missing	Report Missing
Head commit (c40c466)	185989	132526	71.25%

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 (#5974)	221	188	85.07%

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]

Review

Went through ImporterSettings, AbstractImporter, Importer, ImporterContext, CSVImporterFormat, JSONImporterFormat, and the new Issue5968ImporterSkipOnRowErrorTest (38 tests). Overall this is a careful, well-reasoned change - the transaction-ownership bookkeeping (callerTransactionActiveOnEntry / TransactionOwnership / ownsTransaction) is exactly the kind of thing that is easy to get subtly wrong in an importer that can run either standalone or against a caller-supplied Database/transaction (embedding API, IMPORT DATABASE over HTTP), and the reasoning is traced end-to-end with in-code comments and dedicated regression tests (ghost records, prior-batch survival, schema-auto-create-then-row-1-fails, ownsTransaction=false paths, etc.). The default-path fix for the vertex async persist-failure being silently swallowed (previously SEVERE-logged only) is a legitimate, well-isolated correctness fix and is called out clearly as a behavior change in the PR description.

Possible issue: async error handler is not restored for externally-managed databases

In CSVImporterFormat.loadVertices (abort mode), database.async().onError(...) unconditionally replaces whatever handler was already registered on the Database, and handlerActive is set to false in finally to stop the (still-installed) lambda from reacting to later, unrelated async failures once loadVertices() returns. That correctly prevents post-import failures from being misattributed to this import's context (covered by csvVertexImportDeactivatesAsyncErrorHandlerAfterCompletingSoLaterUnrelatedFailuresAreNotMisattributed), but it does not restore the handler that was registered before the import ran - the comment notes there is no getter to save/restore it.

Practical effect for the embedding constructor (new Importer(db, url)) on a Database the caller keeps using afterward: if the caller had registered their own custom onError handler before running the import, that handler is gone for good after loadVertices() returns - replaced by the now-inert lambda - rather than being restored. Pre-PR, the equivalent overwrite at least kept logging SEVERE for later unrelated errors (misattributed, but visible); post-PR those later errors go completely silent (no default handler exists at the DatabaseAsyncExecutorImpl level when onErrorCallback == null). This is a narrow edge case (only matters for long-lived externally-managed databases with a pre-existing custom async handler that continue to use database.async() after the import), and it is arguably a pre-existing anti-pattern this PR deepens rather than a fresh regression, so I would not block on it - but it might be worth a one-line callout in ImporterSettings's Javadoc for the embedding constructor, or a follow-up if DatabaseAsyncExecutor ever gains a way to read back the current handler.

Everything else checked out

• CSV per-row transaction handling (loadDocuments/loadVertices): the ownsTransaction gating on rollback/commit correctly distinguishes 'this import's own transaction' from 'the caller's pre-existing one,' and the skip-mode-requires-exclusive-transaction guard (newExclusiveTransactionRequiredException) is checked eagerly before any schema side effects, with a clear, actionable message.
• JSON nested-failure handling: catching at every BEGIN_OBJECT/BEGIN_ARRAY call site (rather than only at the top level) is the right fix for the reader-desync problem described in the PR body - confirmed that Type conversion happens in createRecord() after reader.endObject() has already run for that nesting level, so catching at the call site (not inside the callee) is what keeps the JsonReader in sync for sibling properties/array entries. The createdDocuments/createdVertices/createdEdges counter snapshot-and-reset around each top-level record is necessary since createRecord() increments those before save()/commit() can fail - good catch.
• Vertex async draining on synchronous failure: attachConcurrentAsyncError plus the database.async().waitCompletion() calls added to the RuntimeException/IOException catch blocks in loadVertices close a real gap (a synchronous per-row failure used to skip the post-loop waitCompletion(), which was previously masked for self-managed databases by close() draining the queue on the way out, but not for externally-managed ones).
• Style/conventions: adheres to the repo's brace/final conventions, no fully-qualified names where an import would do, no new dependencies, no leftover System.out/debug code.
• Test coverage: strong - both formats, both modes, CLI-arg and IMPORT DATABASE ... WITH onRowError=skip entry points, the atomic-HTTP-transaction rejection path (via db.transaction(() -> ...) standing in for DatabaseAbstractHandler), ghost-record regressions for both documents and vertices, multi-batch partial-survival for the pre-existing-but-newly-fixed vertex async bug, and the ownsTransaction=false / externally-managed-transaction paths for both success and failure. I did not spot a gap worth calling out.

Not verified in this environment

I was not able to run mvn verify/the new test class in this sandbox (tool restrictions), so I am relying on static reading rather than an actual green run - worth confirming CI is green (the PR description says it was run 3x locally with no flakiness, which is reassuring).

Nice work on the documentation-in-comments here - the invariants around transaction ownership are genuinely subtle and future readers (including future PRs touching this code) will benefit from them being spelled out.

[lvca]

Addressed in d538066. This review found no correctness bugs ("everything else checked out" across CSV/JSON transaction handling, async draining, style, test coverage) - only one narrow, explicitly non-blocking point.

Embedding constructor doesn't restore a caller's own async error handler (documented) - agreed this is real but narrow: only matters for a long-lived externally-managed Database with a pre-existing custom onError handler that keeps using database.async() after the import returns. Took the suggested lightweight route rather than building out save/restore machinery on DatabaseAsyncExecutor for it: added a Javadoc callout on the Importer(Database, String) embedding constructor itself, explaining that handlerActive only makes the import's own handler inert afterward, it doesn't restore whatever was there before, since there's no getter to read it back. Agreed with the review's framing that this is arguably a pre-existing anti-pattern this PR deepens (pre-PR, the overwrite at least kept logging SEVERE for later unrelated errors) rather than a fresh regression, so a follow-up on DatabaseAsyncExecutor itself would need to be its own separate change if there's appetite for it.

38 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x, plus a clean module install.

PR: #5974

[claude]

Review: bulk importer skip-and-log mode (#5968)

I did a close read of the transaction-ownership logic in CSVImporterFormat, JSONImporterFormat, ImporterSettings, ImporterContext, and AbstractImporter, plus the new Issue5968ImporterSkipOnRowErrorTest (38 tests). Overall this is a very carefully engineered change, the transaction bookkeeping (callerTransactionActiveOnEntry, ownsTransaction/transactionActiveOnEntry, per-row begin/commit/rollback) is unusually well reasoned and the accompanying comments make the intent easy to verify line by line.

What I checked specifically

• Traced every database.begin()/commit()/rollback() call in loadDocuments/loadVertices/parseRecords/parseRecordsArray across both abort and skip modes, including the externally-managed-database and lazy-schema-auto-creation edge cases the tests target.
• Verified the beginRowTransaction() restructuring (which now commits+restarts an already-active ambient transaction before the per-row loop, in both modes for loadDocuments) doesn't silently change durability of schema DDL created moments earlier by updateDatabaseSchema(). The PR's own comment/test (csvVertexImportSkipModeSurvivesFirstRowFailureWhenSchemaAutoCreatedViaEmbeddingConstructor and the abort-mode counterpart) note schema mutations aren't governed by the data transaction's rollback in ArcadeDB, so this is a no-op in practice, not a hidden atomicity regression. Good that this was traced empirically rather than assumed.
• Checked the async onError handler lifecycle in loadVertices (handlerActive flag, firstAsyncError capture, draining via waitCompletion() on every exit path including the synchronous per-row throw path). Didn't find a path where an async failure from this call's own vertices could be lost or misattributed to a later, unrelated database.async() user.
• Checked the JSON recordFailed propagation through nested parseRecord/parseArray recursion, and the counter snapshot/restore (createdDocumentsBefore/etc.) around each top-level record's rollback. Looks correct: a nested failure is caught at the recursion call site (before the reader desyncs), bubbles up as a discarded top-level record, and counters are restored precisely to their pre-record snapshot.

I didn't find a functional bug in this logic. A couple of minor, non-blocking observations:

1. Doc/Javadoc scope nit: ImporterSettings.onRowError's field Javadoc and the class-level statement "edges already skip-and-log unconditionally" are written as a general rule, but that unconditional edge behavior is only true for CSVImporterFormat.loadEdges. In JSONImporterFormat, an edge is created via the same convertMap()/createRecord() path as documents/vertices and is subject to the same per-record onRowError handling (a save() failure on an edge can still abort the whole JSON import in abort mode, or trigger a whole-record skip in skip mode) - this is pre-existing behavior, not introduced here, but readers of the new Javadoc might reasonably assume it also covers JSON edges. Might be worth a one-line clarification ("...for CSV; JSON edges follow the same per-record rule as documents/vertices").
2. Repeated pattern: if (ownsTransaction && database.isTransactionActive()) database.rollback(); appears close to identically in six places across loadDocuments/loadVertices. Given the file already introduces a small helper (beginRowTransaction), a rollbackIfOwned(database, ownsTransaction) helper could remove some duplication, purely a readability nit.
3. Performance tradeoff is real and clearly disclosed: -onRowError skip drops CSV vertex imports to fully synchronous, single-threaded, batch-size-1 saves, which is a large throughput cliff for big files. This is already called out prominently in the settings Javadoc and release notes, so no action needed, just flagging that it's worth the reviewer's attention since it's an easy footgun for someone enabling it "just in case" on a large import.

Test coverage

The 38-test suite is thorough: default-abort regression coverage (including the async persist-time-failure fix), skip-mode happy path with counts, ghost-record regressions for both CSV documents and vertices, nested JSON failure isolation, the SQL IMPORT DATABASE ... WITH onRowError=skip entry point separately from the embedding constructor, and eager rejection of invalid -onRowError values and of skip combined with an already-active transaction. I didn't spot an obviously missing scenario.

Behavior-change disclosure

The PR description and release note are upfront about the two behavior changes in the default abort path (CSV vertex async persist-time failures now abort instead of silently succeeding; JSON's pre-existing per-record-commit gap is now explicitly documented as not fully atomic). Given abort is the default, that's the right thing to call out loudly, and it's done clearly here.

Nice work on the transaction reasoning and the regression tests that pin down some genuinely subtle interactions (ghost records, reader desync, dangling schema transactions).

[lvca]

Addressed in c40c466. No functional bug found in this review either - two readability nits, both implemented.

onRowError Javadoc's "edges always skip-and-log unconditionally" is CSV-specific, not general (fixed) - correct: JSON edges go through the same per-record convertMap()/createRecord() path as JSON documents/vertices and are subject to the same onRowError handling; only CSVImporterFormat.loadEdges has the unconditional skip-and-log rule. Clarified the field Javadoc on ImporterSettings.onRowError so a reader doesn't assume the CSV-specific carve-out also covers JSON. Pre-existing behavior, not introduced by this PR - documentation-only fix.

Repeated if (ownsTransaction && database.isTransactionActive()) database.rollback(); (fixed) - extracted a rollbackIfOwned(database, ownsTransaction) helper and replaced all six occurrences across loadDocuments/loadVertices, mirroring the existing beginRowTransaction()/computeTransactionOwnership() pattern already in this file. Pure deduplication, no behavior change.

Performance tradeoff (skip mode's throughput cliff) - already disclosed prominently in the settings Javadoc and release notes; no action needed, just noted as worth the maintainer's attention.

38 tests in Issue5968ImporterSkipOnRowErrorTest plus the existing importer suites pass, repeated 3x, plus a clean module install.

PR: #5974

[claude]

Review

This is a well-engineered fix with an unusually thorough test suite (38 tests, Issue5968ImporterSkipOnRowErrorTest.java) that pins down subtle transaction-ownership edge cases most PRs wouldn't think to cover (caller-managed vs. self-managed Database, async batch boundaries, nested JSON reader desync, schema auto-create races). I traced the actual transaction ownership logic against the source (not just the diff) and didn't find a correctness bug. Details below.

Verified correct

• callerTransactionActiveOnEntry snapshot (AbstractImporter.openDatabase()) is captured before any schema auto-creation touches the transaction state, and is computed exactly once per load() call. Importer.load()'s final commit is correctly gated on it, so a genuinely caller-owned transaction is never committed/rolled back by the importer on either the success or failure path.
• CSV per-row commit/rollback in skip mode (loadDocuments/loadVertices): counters (createdDocuments/createdVertices) are only incremented after database.commit() succeeds, so a commit-time failure (e.g. duplicate key detected at index time) can't overcount. rollbackIfOwned correctly no-ops when ownsTransaction is false, so a caller's pre-existing transaction is never touched even on a mid-loop synchronous failure in default "abort" mode.
• Vertex async draining: every exit path (success, IOException, RuntimeException) calls database.async().waitCompletion() before the firstAsyncError check / before finally deactivates the error handler (handlerActive), so a persist-time failure on the async worker thread can't be missed or attributed to the wrong (later, unrelated) call. Good catch on the pre-existing bug where this was previously only logged at SEVERE and silently ignored.
• JSON nested-object handling: catching at each BEGIN_OBJECT/BEGIN_ARRAY recursion site (rather than only at the top level) is the right fix to avoid desyncing the JsonReader, and the recordFailed flag correctly routes any nested failure through the enclosing record's own rollback rather than trying to partially undo a bucket write. The createdDocuments/createdVertices/createdEdges before/after snapshot-and-reset in parseRecordsArray is applied unconditionally (both abort and skip modes), which is correct and keeps the reported counts consistent with what's actually durable.
• Guard ordering: the exclusive-transaction-ownership check (newExclusiveTransactionRequiredException) runs before any schema side effect in both loadDocuments/loadVertices and parseRecords, matching the test that asserts the auto-created unique index isn't left behind when the guard rejects the run.

Minor, non-blocking observations

1. database.async().onError() handler replacement, not restoration (CSVImporterFormat.loadVertices): this permanently discards any error handler the embedding caller had registered before calling Importer(Database, String).load(), since DatabaseAsyncExecutor has no getter to save/restore it. This is honestly documented in the constructor's Javadoc and a regression test locks in the cleanup-on-exit behavior, but it's still a footgun for embedders using database.async() directly alongside the importer. Might be worth a follow-up to add handler stacking (or at least a "get current handler" accessor) to DatabaseAsyncExecutor at some point, but not something this PR needs to solve.
2. Double rollbackIfOwned calls in the default "abort" path of loadDocuments/loadVertices: a per-row RuntimeException is rolled back once in the inner per-row catch, rethrown, and then the outer catch (RuntimeException e) calls rollbackIfOwned again. It's harmless (rollbackIfOwned checks isTransactionActive() first), but slightly redundant - could simplify by not rolling back in the inner catch when !skipOnError (since it rethrows immediately and the outer catch handles it anyway), or by adding a one-line comment noting the redundancy is intentional/defensive.
3. Counter reset via .set() rather than delta-based in parseRecordsArray (context.createdDocuments.set(createdDocumentsBefore) etc.): correct today since JSON record parsing is strictly single-threaded, but it's a latent trap if this loop is ever parallelized later (a .set() would clobber concurrent updates from other records). Not urgent, just worth a short comment noting the single-threaded assumption if it isn't already documented elsewhere.
4. Performance trade-off is real and clearly communicated (per-row transactions, vertices dropping to fully synchronous single-threaded saves under -onRowError skip), and it's called out prominently in the release notes, Javadoc, and a runtime INFO log when -commitEvery/-parallel are set alongside skip. Good that this stays opt-in with abort as the default.
5. Behavior change in the default path is real (vertex import now aborts loudly on an async persist-time failure it previously swallowed at SEVERE). This is correctly framed as a correctness fix in the PR description and release notes, but since it can break pipelines that were unknowingly relying on the old silent-failure behavior, it'd be worth double-checking whether this warrants a bigger callout than a release-notes paragraph (e.g. a CHANGELOG/breaking-changes heading) given ArcadeDB's versioning conventions.

Test coverage

Excellent - covers both CSV and JSON, documents/vertices/edges, default vs. skip mode, embedding constructor vs. CLI vs. SQL IMPORT DATABASE entry points, caller-owned vs. self-owned transactions, multi-batch async vertex persistence, nested JSON objects on both document and vertex mappings, and negative cases (malformed JSON structure, CSV syntax-level parse failure, invalid -onRowError value, skip mode rejected inside an active transaction). No @Tag("slow")/@Tag("benchmark") needed given the fixtures are tiny.

Not run

I wasn't able to execute mvn compile/mvn test in this review environment (build commands weren't permitted), so this review is based on careful static tracing of the transaction-ownership logic against the actual source files rather than a live test run. Given the PR description states the full suite was run 3x with mvn -pl integration -am install, I'd still recommend confirming CI is green before merge, particularly given how much of this hinges on transaction nesting/commit-ordering subtleties that are easy to get right in review but wrong under real concurrency.

Overall: solid fix for a real pain point (aborting multi-hour imports on the first bad row), with unusually careful attention to not regressing the embedding API's transaction contract. No blocking issues found.

[codecov]

Codecov Report

❌ Patch coverage is 80.09050% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.07%. Comparing base (33f7e71) to head (c40c466).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...ntegration/importer/format/JSONImporterFormat.java	66.66%	24 Missing and 3 partials ⚠️
...integration/importer/format/CSVImporterFormat.java	88.54%	9 Missing and 6 partials ⚠️
...rcadedb/integration/importer/AbstractImporter.java	0.00%	0 Missing and 1 partial ⚠️
...rcadedb/integration/importer/ImporterSettings.java	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5974      +/-   ##
==========================================
+ Coverage   68.22%   69.07%   +0.84%     
==========================================
  Files        1797     1797              
  Lines      153395   153646     +251     
  Branches    32481    32509      +28     
==========================================
+ Hits       104658   106134    +1476     
+ Misses      35338    34116    -1222     
+ Partials    13399    13396       -3     

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