fix: incremental view sources, STRING over stored values, faithful JSONL/GraphSON/GraphML round trips, NULL string methods (#10205 #10204 #10202 #10201 #10193) - #10253

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 30.39% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 181 functions across 31 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly summarizes the main changes and identifies the five related issues. It is long but remains specific and relevant.
Description check		The description provides detailed change summaries, motivation, related issue references, implementation notes, test coverage, and known limitations. It does not use the template headings or include t…
Linked Issues check		The PR meets the coding requirements for all five active linked issues. For #10205, MaterializedViewSources collects named FROM types across the parse tree and rejects untrackable incremental sour…
Out of Scope Changes check		The changes stay within the linked issue scope. Materialized-view source analysis and listener changes support #10205. Stored-value validation supports #10204. JSONL type hints, link reconciliation, a…

Full details: Description check

Explanation

The description provides detailed change summaries, motivation, related issue references, implementation notes, test coverage, and known limitations. It does not use the template headings or include the checklist, but the required information is mostly present.

Full details: Linked Issues check

Explanation

The PR meets the coding requirements for all five active linked issues. For #10205, MaterializedViewSources collects named FROM types across the parse tree and rejects untrackable incremental sources. Listener lifecycle changes cover subtype writes, reload, alter, and drop. For #10204, DocumentValidator rejects incompatible stored values before a STRING declaration. Regression tests cover numeric values and indexes. For #10201, JSONL version 2 adds sparse type hints, exact-path link reconciliation, dangling-link reporting, and bucket restoration. For #10202, the PR adds ArcadeDB GraphSON and GraphML value handling for scalar, temporal, binary, array, list, map, and mixed-type values. Gremlin accepts the required array types and LocalDate. For #10193, SQLMethodLength and SQLMethodReplace return null for the required null inputs. The linked regression tests cover these behaviors.

Full details: Out of Scope Changes check

Explanation

The changes stay within the linked issue scope. Materialized-view source analysis and listener changes support #10205. Stored-value validation supports #10204. JSONL type hints, link reconciliation, and bucket handling support #10201. GraphSON, GraphML, Gremlin array, and LocalDate changes support #10202. Null string-method changes support #10193. The added tests and compatibility handling directly support these objectives. No unrelated change is demonstrated.

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

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

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

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

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review of #10253

I reviewed the engine changes closely (views, STRING validation, NULL methods) and skimmed the JSONL, GraphML and GraphSON code. I did not run the tests.

What looks good

• MaterializedViewSources walks the parse tree, so every clause is covered the same way. Refusing REFRESH INCREMENTAL at DDL time for untrackable sources is better than reporting STALE later.
• Views add nothing to the read path: hasAfterWriteListenersInLineage() is gated by an AtomicInteger, and registerListeners is idempotent.
• Each fix has a regression test named after its issue, and the tests that pinned old behavior were updated.

Concerns

1. STRING declaration now scans every record (10204). canBeViolatedOn returns true for any STRING property, so CREATE PROPERTY T.name STRING on a large type now reads all records (it used to skip the read). Correct, but it is a cost inside a schema change; please confirm it is accepted and document it. Also !(stored instanceof String) rejects Character or other CharSequence values; please check nothing legitimately stores those.
2. Behavior changes need release notes. length() on NULL now returns NULL (was 0), and replace() returns NULL (was the search string). Both match SQL, but queries like WHERE name.length() = 0 change behavior.
3. Views. adoptEveryReadType logs an unparsable stored query only at FINE, so the view silently tracks fewer types; consider WARNING. In alterMaterializedView, checkIncremental runs outside the lock on current.getQuery() and could see a stale query after a concurrent ALTER; re-check inside the lock.
4. Link/edge limit. address.city and out() are untracked while the view reports VALID. Please state this in the user docs for REFRESH INCREMENTAL.
5. JSONL format version 2. Please confirm how an older importer treats a v2 file (ignore the y tree or fail clearly), and add a test importing a v1 file with the current importer to pin "old exports import as before".
6. Style. The PR is large (about 2.7k lines) and covers five unrelated issues; smaller PRs are easier to review and revert. MaterializedViewSources has a trivial text() wrapper around getText() and a fully qualified CommandSQLParsingException in the Javadoc (import it). The ALL-CAPS comments (e.g. in Type.java) don't match the surrounding style.
7. Test gaps. Update and delete on a subtype created after the view (not just create); restart persistence of a multi-source view including a subquery source; GraphML/GraphSON round trip of a mixed-type key containing nulls.

The design is sound. To settle before merge: the scan cost (1), release notes (2), and the importer version check (5).

Review addressed in 766abcb:

• (3) The incremental check now runs inside the ALTER's lock on the query of the view being replaced. An unparsable stored view query is now logged at WARNING.
• (6) Removed the text() wrapper. The Javadoc now imports CommandSQLParsingException instead of using the fully qualified name. The new comment in Type.java is in normal case. The field Codacy flagged in JsonlImporterFormat moved to the top of the class.
• (7) Added tests: delete on a subtype created after the view; a grouped view over a subtype subquery with updates before and after a reopen; and a null in a GraphML/GraphSON key that holds mixed value types.
• (1) The full read on CREATE PROPERTY ... STRING is intended: it is the same scan every other declared type already triggers, and without it there is no way to know whether stored values are strings. Character and other CharSequence values are not a problem: ArcadeDB stores them as STRING and reads them back as String.
• (2, 4) Done in arcadedb-docs: a NOTE in the 26.11.1 upgrade page, including the WHERE name.length() = 0 consequence. The materialized-views page states the link/edge limitation.
• (5) An older importer reads a version 2 file without failing: it reads only p, r and t, ignores the y tree, and loads the values the way it always did. Version 1 files are pinned by JsonLImporterIT, whose fixtures are handwritten exporterVersion: 1 files without a y tree, and that IT passes.

Review

I read the engine changes (#10193, #10204, #10205) and JsonlImporterFormat (#10201) in full. I only skimmed the Gremlin/GraphML/GraphSON changes (#10202) and the tests. Overall the design is solid: the parse-tree walk for view sources is a clear improvement over "first FROM identifier", and each fix comes with a regression test named after its issue. I found one real bug risk and a few smaller points.

Bug risk

JsonlImporterFormat.reconcileUnresolvedLinks sorts with an inconsistent comparator.

links.sort((a, b) -> a.path().length > 0 && a.path()[0] instanceof Integer x && b.path().length > 0
    && b.path()[0] instanceof Integer y ? Integer.compare(y, x) : 0);

It returns 0 whenever either side is not an integer-led path. That is not a total order, because "equal" is not transitive. For example, [3] vs "m" is 0, "m" vs [1] is 0, but [3] vs [1] is not 0. List.sort (TimSort) can throw IllegalArgumentException: Comparison method violates its general contract on 32 or more elements. It can also silently leave the list in the wrong order. A record with many pending links, mixed between list and map/embedded properties, would hit this. The exception would be caught by the per-record catch, but the record's links would be left unreconciled and counted as an error.

Suggested fix: only sort within one property (group by property(), then by descending index). Alternatively make it a total order, e.g. integer-led paths first in descending order, everything else after. The descending order is only needed for the removeIfMissing case, where path.length == 1.

Smaller points

1. Write-path cost with views. hasAfterWriteListenersInLineage() turns on the trigger-lineage walk for AFTER create/update/delete on every type in the database as soon as any INCREMENTAL view exists, not only on the view's source types. The PR text says this is a "no read path cost" trade-off, and that is true. A one-line note in the Javadoc that unrelated writes now also walk the lineage would be useful, since CLAUDE.md puts performance first. A quick micro-measurement on a database with one view would help.
2. CREATE PROPERTY ... STRING now does a full scan. canBeViolatedOn now returns true for STRING, so requireDeclaration reads every record on every plain CREATE PROPERTY x STRING over a non-empty type. Previously this was skipped. It is correct for CREATE PROPERTY STRING over stored integers is accepted without converting them, so an index on it disagrees with a scan #10204, but it is a cost on large types, and ALTER PROPERTY paths probably go through the same check. This is worth mentioning in the PR description or docs.
3. Validation message. validateStoredValue embeds the stored value (": " + stored) and unquoted type and property names in the suggested UPDATE statement. A very large value would make a huge message, and a name needing quoting would give a non-working suggestion. Consider truncating the value and quoting the identifiers.
4. Behavior change. length() on null now returns null instead of 0, and replace() with a null arg returns null. These are correct per SQL, but they can break existing queries that rely on length() = 0 for nulls or ifnull(...)-less arithmetic. Please call this out in the release notes or changelog.
5. Importer: dangling links now become null. This is the right call, but a MANDATORY or NOT NULL link property will now fail the whole record at reconcile time (the new comment says so). Consider a test covering that case and the -onRowError behavior.
6. adoptEveryReadType swallows RuntimeException and only logs a WARNING. That is acceptable for load robustness, but the view then silently keeps the narrower source list and may go stale. A test that loads a legacy view definition with a multi-type query would pin the intended behavior (the PR may already have one; I did not verify).
7. Style (CLAUDE.md). MaterializedViewSources.collect uses a String[] untrackable = { null } out-parameter. A small private accumulator class or returning a result object would be cleaner and a bit more idiomatic. Also, the PR touches the engine, integration, and gremlin modules, so please confirm that the gremlin/graphql tests were run with mvn verify, per the CLAUDE.md note, not test.

Nothing here is a security concern: there are no new dependencies or external inputs beyond SQL and JSONL parsing, and the query walk is bounded by the parse tree.

Second review, addressed in 2fcbecd:

• Comparator (bug risk): link reconciliation now sorts with a total order: by property, then list positions highest first, then every other path. manyDanglingLinksOfMixedKindsInOneRecord covers it with 40 links each in a typed list, a typed map and an untyped list in one record.
• (1) Added a Javadoc note on hasAfterWriteListenersInLineage. Its cost is one walk of a cached array of registries, each a list that is empty unless something listens.
• (3) The refusal message caps the value at 100 characters and back-quotes the identifiers in the suggested UPDATE.
• (5) Added aDanglingMandatoryLinkFailsTheImportInsteadOfBeingRepointed.
• (6) Added aViewStoredWithOnlyItsFirstSourceTracksAllAfterLoad: schema.json is rewritten to the old single-source list, and after the load the view tracks and refreshes on its second source.
• (7) Replaced the String[] out-parameter with a Collector. The gremlin tests run through gremlin-it with verify (-Pintegration, -am): 611 unit tests and 1,956 IT/TinkerPop suite tests passed.
• (2, 4) As noted above: the scan is intentional, and the docs carry the upgrade notes.

Review

I read the PR description and the engine-side diff closely (MaterializedViewSources, LocalDocumentType, string methods, Type). I skimmed the gremlin and integration changes and did not build or run anything. It is a well-scoped set of fixes with a regression test per issue, and the comments explain the why.

Positives

• MaterializedViewSources walks the parse tree instead of using the first FROM identifier, so subqueries in FROM, WHERE, projection and LET are covered by one mechanism.
• Refusing REFRESH INCREMENTAL at DDL time for untrackable sources beats reporting STALE later, and the error names the source and the alternative.
• hasAfterWriteListenersInLineage() keeps views off the read path.
• Each issue has its own IssueNNNN test, and the old Issue9112 expectation is updated explicitly.

Points to consider

1. SQLMethodLength behaviour change: null.length() used to return 0. Queries such as WHERE s.length() = 0 that relied on it will stop matching null rows. This matches SQL semantics and is probably right, but please call it out in the release notes and check that Studio or internal SQL does not depend on it.
2. Write-path cost: the fireAfter* methods now walk the trigger lineage for every type once any incremental view exists, not only for source types. The cost is small and documented, but a quick insert-throughput benchmark with and without a view would back it up.
3. Collector in MaterializedViewSources: names starting with a dollar sign are skipped on the assumption they are LET variables. Please add a test for parent/current-style dollar references inside nested subqueries. Also check that backticked and dotted names go through identifierName the same way the old code handled them.
4. Link/edge limit: values reached via address.city or out() are not tracked, so the view stays VALID while stale. Documenting it in the user docs for REFRESH INCREMENTAL, or warning at creation time when the projection has a dotted path or graph function, would help.
5. Issue 10204 is a breaking change: CREATE PROPERTY STRING over stored integers used to succeed and now fails. The message hint is good. Please mention it in the release notes.
6. JSONL format version 2: please confirm a test shows an older reader or importer ignores the new y type tree cleanly. I only saw round-trip tests.
7. Size: five independent fixes across engine, integration and gremlin (about 30 files) in one PR. Splitting would ease review and revert. Not a blocker.
8. Test hygiene: check whether any new round-trip test needs the slow tag, and that no debug System.out is left. The reported Issue6462 timing flake passing on rerun is noted.

No security concerns and no new dependencies seen. No blocking issues from my side.

Third review: no blockers. Point 3 is now pinned by aBackQuotedSourceAndParentReferencesInANestedSubquery: back-quoted Item/Owner are sources, $parent.$current is not, and refresh on the nested source works. The rest were answered above: the release/upgrade notes for 1, 4 and 5 are in arcadedb-docs; the version 2 JSONL compatibility (6) works because the reader ignores keys it does not read and JsonLImporterIT pins version 1 files; and the new tests (8) run in about a second each, with no debug output.

Review of #10253

Well-reasoned change set: comments explain the why, fixes are scoped, tests accompany each issue. I reviewed the engine part of the diff (my view of the diff was truncated, so I did not cover the JSONL import/export, #10202 and server changes).

Strengths

• MaterializedViewSources walks the parse tree rather than special-casing clauses, so no clause can be forgotten. Refusing REFRESH INCREMENTAL at DDL time for untrackable sources (TRAVERSE, MATCH, function, bucket...) is better than reporting a stale view.
• The read path is untouched; hasAfterWriteListenersInLineage() only adds an atomic read on AFTER writes.
• registerListeners is idempotent and adoptEveryReadType degrades gracefully (logs a warning).

Potential issues

1. Counter drift in polymorphicViewListeners. Please confirm every path that removes or replaces a view (DROP VIEW, DROP TYPE, restoreSchemaMembersFromJSONLocked replacing a same-named view, schema reload/close, ALTER) goes through unregisterListeners exactly once. If the counter stays above zero, every AFTER write walks the lineage (perf only). If it goes negative, view maintenance is silently disabled (correctness). Consider a lifecycle test (create, alter, replace, drop, then assert hasPolymorphicViewListeners() is false).
2. unregisterListeners iterates view.getSourceTypeNames(), but addSourceTypeNames and retargetSourceType mutate that list. Make sure no mutation happens between register and unregister, or listeners leak on the old registries.
3. MaterializedViewImpl.addSourceTypeNames reassigns sourceTypeNames without synchronization; if the field is not volatile, concurrent readers (listener or scheduler) may see a stale list.
4. Full scan on CREATE PROPERTY ... STRING. canBeViolatedOn is now true for every STRING property, so adding any STRING property to a populated type forces a full read where it was skipped before. Intentional for CREATE PROPERTY STRING over stored integers is accepted without converting them, so an index on it disagrees with a scan #10204, but it is a cost increase for the most common property type; worth documenting, and skipping when the type is empty.
5. length() and replace() on null now return null (was 0 / the search string). Semantically correct, but user-visible: call it out in the release notes (ifnull(x.length(), 0) is the migration path).
6. Collector.collect is recursive over the parse tree; extremely deep queries could overflow the stack. Unlikely, but a depth guard is cheap.
7. Style nits: trailing blank line before the closing brace in MaterializedViewSources; the long string concatenation in the validator message would read better with a local variable for the class name.

Tests

Coverage looks good (Issue10193StringMethodsOnNullTest, updated Issue9112... test, view tests). Suggested additions: the counter lifecycle test above; a test that a subtype created after the view still triggers incremental refresh; a test that dropping a type referenced only inside a subquery is refused.

No security concerns found. Nothing blocks merging once item 1 is confirmed.

Fourth review: item 1 is confirmed and pinned by theListenerCountFollowsCreateAlterReopenAndDrop, which runs create ×2, ALTER to MANUAL and back (twice), reopen, and drop, and checks hasPolymorphicViewListeners() after each step. The count can't drift: registerListeners is a no-op for a view that already has a listener, and unregisterListeners decrements only when one was set. Schema reload and replacement go through unregisterMaterializedViewRefresh, and a closed schema drops its counter along with it.

• (2) The source list is completed before registration on every path: adoptEveryReadType runs before installMaterializedViewRefresh, and before the new view's registration in ALTER. A rename keeps the same type object, so the unregister resolves the new name to the registry it registered on.
• (3) sourceTypeNames is already volatile.
• (4) Intended, and in the docs. On an empty type the scan reads nothing.
• (6) The walk is no deeper than the parse tree, and the ANTLR parser that built that tree recursed just as deep.
• (7) Removed the trailing blank line.
• Test suggestions: already covered by aSubtypeCreatedAfterTheViewRefreshesItToo and aTypeReadOnlyByASubqueryCannotBeDroppedUnderTheView.

Review

This PR bundles five separate fixes (#10205, #10204, #10202, #10201, #10193) across engine, gremlin and integration. Each has its own regression test, which is good. I reviewed the engine-side changes in detail and only skimmed the gremlin and integration parts.

Observations

Scope

• Five unrelated fixes in one PR make review and revert hard. The JSONL/GraphSON/GraphML format change (new format version 2) would have been better separate from the small engine fixes.

#10204 (STRING over stored numbers)

• canBeViolatedOn now returns true for every STRING property with checkType. So ExistingRecordsCheck.requireDeclaration no longer skips the full scan for a plain STRING property, and CREATE PROPERTY ... STRING on a large type reads every record. That is probably intended, since it is the only way to detect the problem. Please confirm the cost is acceptable and mention it in the docs or release notes.
• DDL that used to succeed is now rejected. The updated Issue9112ExistingRecordsConstraintsTest documents it, and the asString() hint in the message is helpful.
• In validateStoredValue, the String.valueOf and truncation only run on the failure path, so there is no hot-path cost.

#10193 (NULL string methods)

• length() on NULL now returns null instead of 0. This matches SQL, but it is visible to existing queries such as WHERE name.length() = 0. Please call it out in the release notes.
• replace() returning null when an argument is null is correct.

#10205 (incremental view sources)

• Collecting FROM targets from the ANTLR parse tree is reasonable. Please check:
  • Alias and LET-variable shadowing, e.g. FROM $parent.x or a LET-defined subquery must not be read as a type name.
  • The refusal list covers FROM [#1:0, #1:1] and FROM :param, as well as TRAVERSE, MATCH, functions and buckets.
• fireAfterCreate/Update/Delete now use hasAfterWriteListenersInLineage() instead of hasTriggers(). Make sure it is cached or invalidated correctly when a trigger or view is registered on a supertype after the subtype exists. A test for a late-added subtype combined with a late-dropped view would help.
• Refusing DROP TYPE for a subquery source is a good safety fix.

#10201 and #10202 (formats)

• The sparse "y" type tree keeps old exports importable. Please add a test that imports a version 1 export (if one is not already there), and one where "y" names a property that is missing from "p".
• Clearing and counting dangling links (danglingLinks) is much better than leaving a RID that names another record.
• Please confirm XXE protection (DTD and external entities disabled) for the XML parser that ArcadeGraphMLReader uses on untrusted input.
• Please confirm ValueTypeHints decoding maps type names from the file only to a fixed set of types and never loads classes reflectively.

Style (CLAUDE.md)

• I saw no fully qualified names in the engine changes. Please double check the gremlin classes.
• Check that no System.out is left in the new code.
• Tag any heavy round-trip tests @Tag("slow").

Tests

• The PR notes Issue6462CrossSlotWaitCompletionMissesCascadeTest as flaky. It is unrelated, but worth tracking.

Overall this is solid work with good regression coverage. Before merging I would verify the XXE and type-hint decoding points, and add the release-note callouts for length() and the STRING DDL change.

Fifth review, addressed in the latest push:

• XXE: ArcadeGraphMLReader turns off SUPPORT_DTD and IS_SUPPORTING_EXTERNAL_ENTITIES. anExternalEntityInAGraphMLFileIsNotResolved imports a file whose entity points at a local file and checks that the file's content never reaches the graph.
• Type-hint decoding: ValueTypeHints.decode is a closed switch over a fixed set of tokens. Nothing is loaded reflectively, an unknown token is read as untyped JSON, and EMBEDDED only builds a document of a type the target schema already has. aTypeTreeEntryWithoutItsPropertyOrWithAnUnknownTypeIsIgnored covers a y entry with no matching p property and an unknown token.
• Version 1 exports: pinned by JsonLImporterIT, whose fixtures are handwritten exporterVersion: 1 files without a y tree.
• View sources: $-variables are skipped (aBackQuotedSourceAndParentReferencesInANestedSubquery). RID arrays (FromRidArrayContext) and parameters (FromParamContext/FromParamArrayContext) are refused. hasAfterWriteListenersInLineage reads live counters, and the lineage itself is keyed to the types serial, so a subtype added later is picked up. That is covered by aSubtypeCreatedAfterTheViewRefreshesItToo and theListenerCountFollowsCreateAlterReopenAndDrop.
• Style and docs: no fully qualified names or System.out in the new code. Each new test runs in about a second, so none needs slow. The release notes for length() and the STRING DDL change are in the 26.11.1 upgrade page of arcadedb-docs.
• The Issue6462 timing flake is pre-existing and unrelated.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review of the visible part of the diff (engine changes and engine tests, first ~40KB). I did not read the gremlin and integration modules (GraphSON/GraphML/JSONL, ValueTypeHints), and I did not run any tests.

Good

• Each fix has a regression test named after its issue.
• Walking the ANTLR tree for source types is sturdier than the old first-FROM-identifier lookup.
• Refusing REFRESH INCREMENTAL at DDL time for TRAVERSE/MATCH/function/bucket/etc. sources is better than reporting STALE later.
• The view listeners only add work to the AFTER write path.

Concerns

1. Cost of CREATE PROPERTY ... STRING (CREATE PROPERTY STRING over stored integers is accepted without converting them, so an index on it disagrees with a scan #10204). canBeViolatedOn now returns true for every STRING property. ExistingRecordsCheck.requireDeclaration therefore does a full scan of the type for every plain STRING declaration, where it used to skip the read. That is probably acceptable, since the check is only needed when records exist. Please confirm it is not on a hot path, such as CREATE PROPERTY during import or schema restore, or ALTER PROPERTY. A cheap countType == 0 short-circuit would help if it is not there already. Also check that validateStoredValue is not used for per-write validation, where the new instanceof String branch would reject non-String values that STRING conversion currently accepts.
2. Breaking behaviour change (SQL string methods on NULL: s.replace('a', 'b') returns 'a' and s.length() returns 0 instead of NULL #10193). length() on null now returns null instead of 0, and replace() returns null instead of params[0]. This matches SQL semantics and is probably right, but it can change results for existing queries such as WHERE s.length() = 0 and ifnull(...). It deserves a release-note or docs mention. I also suggest a test that s.length() > 0 and s.length() = 0 on a null value behave as expected (both false).
3. Mixed read paths. hasAfterWriteListenersInLineage() makes every AFTER write on any type walk its lineage while any INCREMENTAL view exists (the Javadoc says so). That is a deliberate trade-off. A per-type check ('is any registry in my lineage non-empty') would avoid taxing unrelated types. Consider caching it per type graph, as you already do for the lineage array.
4. Listener bookkeeping.
   • registerListeners now returns early when view.getChangeListener() != null. alterMaterializedView uses copyWithRefreshMode. Please verify the copy carries no stale listener and that the old view's listener is unregistered. Otherwise the polymorphicViewListeners counter and the registrations can drift.
   • polymorphicListenerUnregistered() should only run when a listener was actually registered. I could not see the guard at the top of unregisterListeners.
   • adoptEveryReadType in ALTER runs a full ANTLR parse while the schema lock is held (MaterializedViewSources.of). It is small, but it is better done before taking the lock.
5. Parse-tree matching.
   • FromIdentifierContext uses identifier(0), so check how a type:xyz or other multi-identifier FROM form is handled.
   • name.startsWith("$") skips LET variables, but a quoted type literally named $x would be skipped too (unlikely).
   • A DROP/rename of a type that is only a nested source now has to be handled in retargetSourceType. Please make sure a test covers rename.
6. Style. The addSourceTypeNames double contains check is redundant: the second one only dedups within typeNames. Using a LinkedHashSet would be simpler.
7. Tests.
   • Add tests for restarting the database (view reload via adoptEveryReadType), if they are not already in the part I did not see.
   • Type.TYPES_BY_USERTYPE.put(LocalDate.class, DATE) is global, so a quick test of Type.getTypeByValue(LocalDate) and of any code that previously relied on the miss would be good.

The PR bundles five unrelated issues (views, validation, SQL methods, JSONL, GraphSON/GraphML), which makes it hard to review and to revert. Splitting would be preferable if it is not too late.

🤖 Generated with Claude Code

Sixth review, addressed in the latest push:

• (1) validateStoredValue runs only from ExistingRecordsCheck (DDL), never on a write. The JSONL import declares properties before it loads any record, so the scan reads an empty type. ALTER PROPERTY passes checkType = false, so it is not affected.
• (2) Added aNullLengthMatchesNoComparison: length() = 0 and length() > 0 both skip the NULL row, length() IS NULL matches it, and ifnull(s.length(), 0) = 0 is the migration path. The release note is in the docs.
• (4) copyWithRefreshMode builds a new instance with no listener. ALTER unregisters the old view first, and unregisterListeners returns early when no listener was set, before it decrements. theListenerCountFollowsCreateAlterReopenAndDrop pins this. The parse in ALTER stays under the lock because the previous review asked for that: the check must read the query of the view it replaces.
• (5) Added renamingASourceReadOnlyByASubqueryIsRefused: the rename is refused, and the view keeps working. The $ rule applies only to unquoted names, so a back-quoted type is collected through QUOTED_IDENTIFIER.
• (6) addSourceTypeNames now merges through a LinkedHashSet.
• (7) Added aLocalDateIsAKnownDateValue: getTypeByValue and getTypeByClass already answered DATE through the inheritance walk, so the map entry changes no answer. Reload is covered by theListenersSurviveAReopen and aViewStoredWithOnlyItsFirstSourceTracksAllAfterLoad.
• (3) Not changed: a per-type cache would have to be invalidated on every listener registration. The walk is a few reads of an array cached per type graph, and only while an incremental view exists.

Review

This PR bundles five separate fixes (#10205, #10204, #10202, #10201, #10193). I read the diff and did not run the tests, so these are code-reading findings only.

What looks good

• Every fix has a regression test named after its issue (Issue10205..., Issue10204..., Issue10201..., Issue10202..., Issue10193...).
• The tests whose old behaviour changed (Issue9112ExistingRecordsConstraintsTest, JsonLImporterIT) were updated.
• The diff has no System.out leftovers and no wall-clock assertions.
• For views loaded from an older schema, adoptEveryReadType degrades to a WARNING with the stored source list. A query that no longer parses therefore doesn't break schema load.
• Refusing REFRESH INCREMENTAL at DDL time for sources no type event can report (TRAVERSE, MATCH and so on) is better than leaving a view marked VALID while it is stale.
• Rejecting CREATE PROPERTY STRING over stored integers matches the other declared types. The error message tells the user how to convert.
• Registering the view listener on the polymorphic trigger registries, and dispatching only on AFTER writes, keeps the read path unchanged.

Points to consider

1. Scope. Five unrelated areas in one PR (schema views, DDL validation, JSONL, GraphSON and GraphML, SQL methods) make review and bisecting harder. If it is practical, the PR could be split. At minimum the commits should map one-to-one to the issues.
2. Behaviour changes.
   • Refusing CREATE PROPERTY STRING and DROP TYPE of a view's subquery sources can break existing scripts and migrations. Both should be called out in the release notes.
   • Clearing dangling links to null on JSONL import is safer than the old behaviour. Please make sure danglingLinks is surfaced in the import log or console output too, so the data loss is visible.
3. Export format version 2. Old exports are said to import as before. Please confirm that an importer built before this change fails clearly, or ignores the unknown "y" key, when given a v2 file. Is there a test with a v1 fixture and one with a v2 fixture?
4. adoptEveryReadType catches RuntimeException broadly. It is fine for the fallback. Consider logging the stack trace only at a debug level, to avoid noisy warnings on every schema load.
5. Performance.
   • ValueTypeHints builds the "y" tree per record on export. Please confirm that schema-full types skip all allocation (the description says no tree is written), and that the loop avoids boxing for undeclared values.
   • MaterializedViewSources re-parses the view query during schema load for older views. That is a one-time cost, but it adds up on databases with many views.
6. Security. ArcadeGraphMLReader and the GraphML code use an XML parser. Please verify that external entities and DTDs are disabled (XXE hardening) on the XMLInputFactory it uses, because imports take untrusted files.
7. Flaky test. The description mentions Issue6462CrossSlotWaitCompletionMissesCascadeTest failing once and passing on rerun. If it is a known flake, link the issue. If this change could affect it, please check.

I found no blocking problems in the parts I read. Items 3 and 6 are the ones I'd confirm before merging.

🤖 Generated with Claude Code

Pre-verified the head 4621543f44 with the reproductions filed on the five issues, on eclipse-temurin 25 (-XX:+UseCompactObjectHeaders) and 21, against main a759246daa. The current head 346ef1fbdc adds tests and one engine change since, in MaterializedViewImpl.java (the source-list merge), so the #10205 row is from before that change; I will run it again on the merge commit. JDK 25 and 21 print the same on every probe (identity hashes aside).

All five fixed. Two notes on the #10202 row. Min.java as posted compares each imported value with the Java value it wrote, so java.util.Date and Instant still print CHANGED to LocalDateTime on the head: that is the store's own mapping, since the source database reads them back as LocalDateTime too; it also prints float[] by identity. The counts above come from a copy that compares with what the source database reads back and prints float[] by content. The two INSERT FROM SELECT cases in SqlWriteCandidates.java are #10192, fixed in #10250, and unchanged here as expected.

Seventh review, no blockers. The two items to confirm before merging are already covered by tests:

• (3) Version 1 import is pinned by JsonLImporterIT (handwritten exporterVersion: 1 fixtures). Version 2 is pinned by Issue10201JsonlRoundTripFidelityTest, including aTypeTreeEntryWithoutItsPropertyOrWithAnUnknownTypeIsIgnored. An older importer reads only p, r and t, so it ignores y and loads the values as before.
• (6) XXE is pinned by anExternalEntityInAGraphMLFileIsNotResolved: SUPPORT_DTD and IS_SUPPORTING_EXTERNAL_ENTITIES are both off.
• (2) danglingLinks is in the IMPORT result and in the importer's returned statistics, and the import logs a WARNING with the count.
• (5) A schema-full record allocates no tree: properties whose declaration restores their type are skipped before any encoding, and the y object is created only for the first property that needs a hint.

Thanks @tae898 for re-running the reproductions on both JDKs.

Eighth review: each point was already covered in an earlier round.

• (2) theListenerCountFollowsCreateAlterReopenAndDrop asserts the counter after create, ALTER both ways, reopen and drop. unregisterListeners only decrements when a listener was set, and a failed ALTER refuses before anything is unregistered.
• (1) The extra cost is one walk of a cached array of empty registries, per AFTER write, and only while an incremental view exists.
• (3, 4, 5) Covered in the arcadedb-docs upgrade notes and the materialized-views page.
• (7) aTypeTreeEntryWithoutItsPropertyOrWithAnUnknownTypeIsIgnored, plus version 1 fixtures in JsonLImporterIT.
• (8) manyDanglingLinksOfMixedKindsInOneRecord checks the exact order of the surviving LIST OF LINK elements.
• (9) No new test binds a port or asserts elapsed time. The gremlin tests ran with verify through gremlin-it.

The CodeRabbit major finding is fixed (923ffdb), and CodeRabbit resolved the thread.

Review (engine changes read closely; gremlin/integration only skimmed; I did not build or run tests)

Good structure: parse-tree walk for view sources, DDL-time refusal of unsupported REFRESH INCREMENTAL queries, and a regression test per issue.

Points to consider

1. CREATE PROPERTY ... STRING cost. canBeViolatedOn is now true for every STRING property, so requireDeclaration no longer skips the scan for a plain STRING. DDL becomes O(records) for the most common type. This is the intended fix for CREATE PROPERTY STRING over stored integers is accepted without converting them, so an index on it disagrees with a scan #10204, but please mention it in the description and confirm the scan stops at the first violation.
2. Write path with any view present. hasAfterWriteListenersInLineage() makes every AFTER write on every type walk the trigger lineage once any INCREMENTAL view exists. Documented, but tracking listeners per source type (and subtypes) would avoid the cost for unrelated types.
3. Listener counter balance. polymorphicViewListeners is a bare AtomicInteger. Please verify unregisterListeners returns early when the view has no listener (no double decrement), and add a test that creates, alters, drops and reloads a view and asserts hasPolymorphicViewListeners() is false.
4. alterMaterializedView. adoptEveryReadType(newView) runs before listeners are re-registered, and registerListeners is now a no-op when view.getChangeListener() != null. If copyWithRefreshMode carries over the old listener, newly adopted source types may never get registered. Please confirm with a test (ALTER to INCREMENTAL on a view with a subquery source, then write to that source).
5. NULL semantics. length() on NULL changes from 0 to null, which can break queries like WHERE s.length() = 0. Worth a release-note line.
6. Minor. The view query is parsed again on every schema load (adoptEveryReadType). Acceptable. The STRING check rejects anything that is not a String instance, so please confirm no path legitimately stores a Character or enum in a STRING property. The error message echoes up to 100 chars of stored data, which could be sensitive in logs.

Style and tests

• Follows repo conventions (final, imports, AssertJ style, no System.out in the engine diff).
• Please check whether the large round-trip tests in integration and gremlin need @tag("slow").
• The PR bundles five issues across three modules, which makes review and revert harder. Splitting is optional.

Overall a solid set of fixes. Items 1, 3 and 4 are the ones I would like checked before merge.

Ninth review: the three items it asks to check already have tests.

• (1) requireDeclaration throws on the first record that violates the declaration, so the scan stops there. On a type whose values are all strings it reads every record once, the same scan every other declared type already triggers, and the docs say so.
• (3) unregisterListeners returns before it decrements when the view has no listener. theListenerCountFollowsCreateAlterReopenAndDrop asserts hasPolymorphicViewListeners() through create, ALTER both ways, reopen and drop.
• (4) copyWithRefreshMode builds a new instance with no listener, so registerListeners registers it with the source list adoptEveryReadType just completed. anAlterToIncrementalTracksEverySource runs ALTER MANUAL→INCREMENTAL on a view with a WHERE-subquery source, writes to that source, and checks the view.
• (6) The binary serializer stores a Character and an Enum as STRING, and they read back as String.
• (2, 5) Answered earlier. The release note for (5) is in the docs.

Codecov Report

❌ Patch coverage is 69.73533% with 263 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.45%. Comparing base (f7b8a07) to head (a09bdc5).

@@             Coverage Diff              @@
##               main   #10253      +/-   ##
============================================
- Coverage     78.48%   78.45%   -0.03%     
  Complexity     4650     4650              
============================================
  Files          2253     2258       +5     
  Lines        238707   239459     +752     
  Branches      51671    51817     +146     
============================================
+ Hits         187341   187878     +537     
- Misses        32016    32175     +159     
- Partials      19350    19406      +56     

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

A side effect of this PR worth knowing, probably intended: since LocalDate maps to DATE (Type.TYPES_BY_USERTYPE), Result.toJSON() writes a LocalDate stored in an undeclared property as days since the epoch (19724 for 2024-01-02) instead of epoch milliseconds (1704153600000). A declared DATE property was already written as 19724 on 26.10.2, so the two now agree; the commit text does not mention the JSON change, and a JSON consumer of undeclared dates sees the integer change by a factor of 86,400,000.

Embedded, -Duser.timezone=UTC, INSERT INTO Event SET day = ? with LocalDate.of(2024, 1, 2), then SELECT FROM Event and toJSON().get("day"):

Between 5fbf2c0ff8 and a2c54ef20f the only change that touches it is this PR's TYPES_BY_USERTYPE.put(LocalDate.class, DATE). I noticed it because the Python bindings test that to_json_list() writes a DATE as midnight-UTC milliseconds compared it with Result.toJSON() (it now fails in test-python-bindings, e.g. run 39118874295); the bindings keep their documented milliseconds and the test is adjusted on my side, to come with the next bindings update. No change asked here.