fix(#9794,#9806,#9808,#9777,#9812): declared text form in patterns, legacy temporal inline maps, REGEXP guard, agg.* arrays, native select stale index - #9820

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.

Merge Risk: 🟡 Moderate · up to fbaac

An import can report zero warnings despite losing a constraint, and a SELECT racing with an index drop can fail before returning a row. Address the cursor race before merging.

Pre-merge checks | 3 | 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check		The pull request also adds independent changes for #9806, #9808, #9777, and #9812. These changes cover legacy temporal matching, REGEXP validation and importer handling, primitive-array aggregates, an…	Move the #9806, #9808, #9777, and #9812 implementation and tests to separate pull requests. Keep this pull request limited to the Issue #9794 changes and their supporting tests.
Docstring Coverage		Docstring coverage is 48.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 81 functions across 20 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check		The title accurately summarizes the five fixes in the pull request, including declared text matching, legacy temporal patterns, REGEXP validation, aggregate arrays, and stale-index handling. It is lon…
Description check		The description provides a detailed change summary, motivation, related issue references, implementation notes, and test results. It does not use the template headings and omits the checklist, but it …
Linked Issues check		Issue #9794 requires typed declared properties to find values written from text, including repeated MERGE, inline maps, equality, inequality, IN, simple CASE, joins, and indexed paths. `InlineProperti…

---

Full details: Out of Scope Changes check

Explanation

The pull request also adds independent changes for #9806, #9808, #9777, and #9812. These changes cover legacy temporal matching, REGEXP validation and importer handling, primitive-array aggregates, and native select during index drops. Their dedicated production changes and regression tests do not implement Issue #9794.

---

• Fix all pre-merge checks with AI

• 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.}

Review of #9820

Overall this is a well-reasoned change. Routing everything through InlineProperties.declaredValue / isTextAgainstNonText / matchesProperty removes the per-site STRING-only copies, and the IndexNestedLoopJoin and ScanPropertyHashIndex fixes keep indexed and non-indexed plans consistent. I read the main-source diff in detail but did not run the build or tests, and did not go through the test files.

Points to consider

1. Lenient text-to-number conversion. declaredValue calls Type.convert, so any text it accepts becomes a match (for example '5.7' or ' 5' against an INTEGER may truncate or trim to 5). That would be the same truncation the PR rejects for n.p = 1.5. Please add a regression test for non-canonical numeric text ('5.7', '05', ' 5', '1e2') to pin the intended behaviour.
2. Swallowed exceptions on a hot path. declaredValue does catch (RuntimeException) and returns null. In a scan, a text like 'abc' against an INTEGER builds an exception per record. Consider a cheap pre-check or a non-throwing conversion (the old code used Type.convertOrNull).
3. Scan cost. matchesProperty now pays a schema lookup on every text/non-text mismatch for any scalar-declared property, not only STRING. The isTextAgainstNonText gate bounds it, but a quick mixed-type scan benchmark would be reassuring.
4. InExpression range path. The LongRangeList canonical-spelling shortcut only applies to STRING declarations. Please confirm a test covers n.p IN range(...) with a stored text against an INTEGER-declared property.
5. ComparisonExpression ordering. It tries toDeclaredValue(left, rightValue) first, then the right side. With declared properties on both sides the first hit wins. Probably fine, but a test would document it.
6. Style. The lazy declared / declaredResolved pair in InExpression.evaluateTernary is a bit clunky. A tiny helper would read better.

Tests

Please confirm regression tests exist for each linked issue (#9794, #9806, #9808, #9777, #9812). That includes MERGE twice on a UNIQUE index, plan-vs-scan parity for IndexNestedLoopJoin and ScanPropertyHashIndex, float[]/short[] aggregation inputs, and the unusableRegexp rejections. Use assertThat(...) and no raw wall-clock assertions.

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 #9820

Overall this is a careful, well-documented change. The text-form-only coercion is well argued, and keeping indexed and non-indexed plans consistent is the right call. Each issue has a regression test. I read the main-source diff in full and skimmed the tests. I did not run the suite.

Potential issues

1. Exception cost in scans for temporal declarations. InlineProperties.mayBeReadAs shortcuts integral, boolean and decimal types, but default -> true sends every text to Type.convert for DATE, DATETIME, DURATION and the other temporals. So WHERE n.createdAt = 'foo' throws and catches a RuntimeException per record on a DATETIME property. ScanPropertyHashIndex already uses TemporalUtil.mayBeTemporalString, which could serve as a cheap pre-filter here.
2. Overflow still throws per record. For BYTE, SHORT and INTEGER, a text like '99999999999' passes mayBeReadAs and then fails in Type.convert. Rare, but the same per-record exception pattern. A range pre-check would remove it.
3. Broad catch (RuntimeException) in declaredValue. Intentional and commented, but it can also hide real bugs (for example an NPE from a null database). Narrow it if the exception set is knowable.
4. SelectExecutor.readingIndex allocations. It now allocates two lists plus a getIndexesOnBuckets() snapshot on every indexed SELECT. Probably fine, but please confirm getIndexesOnBuckets() is cheap.
5. LocalProperty.checkRegexp behavior change. setRegexp now throws IllegalArgumentException for LIST, MAP, EMBEDDED, LINK, BINARY and ARRAY_OF_* properties, and for BOOLEAN patterns matching neither value. Scripts and migrations that ran before will now fail. The schema-load path only warns, which is good. Please mention this in the release notes, and check that ALTER PROPERTY ... TYPE does not leave an existing REGEXP that is now unusable (that path looks unchecked).
6. DocumentValidator.unusableRegexp for BOOLEAN tests the pattern with matches(). Confirm the write-path validator also uses full-match semantics. If it uses find(), a pattern such as tru would be rejected here but accepted at write time.
7. ScanPropertyHashIndex.textCandidates. integralOf accepts '+5' and '007', matching Integer.parseInt; a test for those spellings would help. Numeric-looking text falls back to a full scan when there are many fractional values (shared MAX_NON_INTEGRAL), which is acceptable.

Style (CLAUDE.md)

• AbstractAggFunction.toDoubleList and toObjectList go through an intermediate boxed list from asListOrNull, so primitive float[] or double[] inputs box twice. A direct primitive loop would match the "prefer primitives" guideline, though it is not required for the fix.

Tests

Coverage is good, with one regression test per issue. Suggested additions:

• an overflow text against an INTEGER, and a non-date text against a DATETIME scan (points 1 and 2)
• ALTER PROPERTY ... TYPE after a REGEXP is set (point 5)
• the database.select() still fails to the caller with a raw "File with id n was not found" during a concurrent DROP INDEX: its stale-index catch tests isValid()`, which a drop clears last #9812 concurrency test should use deterministic hooks, not sleeps

Security

No injection or auth concerns found. REGEXP validation compiles user patterns, which the write path already does, so there is no new ReDoS surface.

Nothing blocking. I would address points 1, 5 and 6 before merge.

Review of #9820

I read the production diff and skimmed the test list. I did not build the PR or run the tests, so everything below comes from reading the code.

What looks good

• The fixes share their helpers. matchesProperty, declaredValue and isTextAgainstNonText now serve every pattern path, and the MergeStep-local copy is gone. That is the right way to stop the paths from drifting apart.
• InlineProperties.declaredValue converts only through the text form, and the PR description explains why (n.p = 1.5 against a stored 1). That keeps indexed and non-indexed plans agreeing.
• The hot paths are cheap. isTextAgainstNonText gates the schema lookup. mayBeReadAs and integralOf pre-check the text shape, so a scan does not throw an exception per record. InExpression resolves the declaration once, and only if it is needed.
• LocalProperty.checkRegexp only warns while reading the schema from disk, so an existing database still opens. ALTER PROPERTY checks before it scans the records.
• The SelectExecutor change takes a snapshot of the sub-indexes right before each read and compares it afterwards (indexesReadUnchanged). That closes the window where a half-finished DROP could return a short result with no error.
• Each fix has its own regression test class, as the guidelines ask.

Points to check

1. Behaviour change in plain Cypher. On a property declared INTEGER, BOOLEAN, DATE and so on, n.p = '5' now returns true. Before, it was false. The PR argues this is correct, but a user could notice it. Please add it to the release notes or docs.
2. mayBeReadAs and Type.convert can drift apart. mayBeReadAs is a hand-written copy of what Type.convert accepts: digit limits, a FLOAT text starting with I or N, and "contains a digit" for temporals. ScanPropertyHashIndex.integralOf and isDecimalText repeat the same rules. If Type.convert changes, these can silently disagree. A test that runs a table of texts through both mayBeReadAs and Type.convert would catch that. Consider moving the rules into one shared place.
3. Changing a property's type is not re-checked. checkRegexp runs only in setRegexp. ALTER PROPERTY x TYPE LIST on a property that already has a REGEXP can still produce the state the PR is meant to prevent. If a type change is rejected or converted elsewhere, a test showing that would settle it.
4. Duplicated pattern validation. unusableRegexp compiles the pattern for BOOLEAN. checkRegexp and StoredValueConstraints.of compile it again for other types. This is not a performance problem on a schema path, but one place for "is this a valid pattern" would be tidier.
5. Scan fallback in ScanPropertyHashIndex.textCandidates. A text lookup can now return null, which sends the caller to a full scan. That happens when fractional + temporal exceeds MAX_NON_INTEGRAL. Text lookups never did this before. Calling isDecimalText per lookup also trims and parses. If a test covers the fallback, fine. If not, add one.
6. Case-insensitive boolean text. declaredValue and ScanPropertyHashIndex treat 'TRUE' as a boolean. That matches Boolean.parseBoolean. A test that locks it in would help, because the index path (Boolean.valueOf(text)) and the Type.convert path must agree.
7. Style. In InExpression, the declared/declaredResolved pair is a hand-rolled lazy value. It works, but a small helper would read better. In LocalProperty.checkRegexp, reason is reassigned several times. Splitting it into two small methods would be clearer.

Security and performance

I found no security problems. The regex is compiled only on schema operations, not per write. One caveat is that Pattern.compile runs user-supplied patterns, but that was already true. I did not look at how the validator evaluates patterns, so ReDoS risk from a user-supplied pattern is unchecked.

Tests

Coverage looks thorough (a declared type x literal table, with and without an index, joins, UNIQUE, the DROP INDEX race, and agg arrays). Please confirm that Issue9812NativeSelectStaleIndexTest does not depend on timing. It should force the "drop under way" state deterministically rather than race a real DROP. Also run the connected suites (openCypher TCK, the ScanPropertyHashIndex tests, and the SQL ALTER PROPERTY tests) before merging.

Review

I read the main-source part of the diff closely and skimmed the tests and the tail of the diff (ScanPropertyHashIndex, MergeRelationship, SelectExecutor, AlterPropertyStatement, LocalProperty, OrientDBImporter). I did not build the PR or run the tests.

Overall

The design is sound. Converting only through the text form, rather than coercing any operand as #9794 proposed, keeps indexed and non-indexed plans consistent. The rationale is documented well. Each fix has its own IssueNNNN...Test, which matches the regression-test rule. Routing every pattern path through InlineProperties.matchesProperty also removes the duplicated logic that had drifted before. MergeStep's special-case restore of legacy temporal text is a good example.

Points to consider

1. InlineProperties.mayBeReadAs copies the parse rules of Type.convert.
   • It hard-codes digit limits per type, accepted boolean spellings, and first-character checks for float/decimal (I, N, ., +, -).
   • It is a pre-filter, and the comment says it can let through shapes the conversion still refuses, so a mismatch is only a perf miss.
   • The reverse is a correctness risk. If it rejects something Type.convert would accept, such as a future format, a locale-specific decimal, or whitespace handling in Long.parseLong variants, a pattern silently stops matching. Please add a parameterized test that checks mayBeReadAs(t, s) == false implies convertOrNull(...) == null over a corpus of inputs per type. That would catch drift cheaply.
2. ScanPropertyHashIndex now keeps a temporals list and an anyTyped flag.
   • The extra List<RID> is boxed per temporal record. On a large indexed temporal column that is extra GC pressure for a path that only serves text lookups.
   • Consider building it lazily on the first text lookup. Alternatively, use a primitive structure, or skip it unless the property is declared with a temporal type.
3. IndexNestedLoopJoin.declaredKey runs per probe row.
   • declaredValue does a schema lookup, mayBeReadAs, and Type.convertOrNull for every text key.
   • If the same text repeats, which is common for join keys, a small per-operator cache of the last converted value would help. This is optional.
4. InExpression resolves the declared property lazily and caches it with declaredResolved.
   • That is correct, but the control flow is now harder to follow, with two assignment sites.
   • Extracting a small resolveDeclared() helper would be clearer.
5. DocumentValidator.unusableRegexp rejects a REGEXP on BINARY, LIST, MAP and similar types.
   • This is a behavior change for existing schemas, which is a risk for databases that already have such a declaration.
   • Please confirm that loading an existing schema (as opposed to ALTER PROPERTY) never goes through requireUsableRegexp. Otherwise an upgrade could fail at open time. A test for that would be good.
6. AbstractAggFunction now delegates to CypherFunctionHelper.asListOrNull. toObjectList allocates new ArrayList<>(list) even when asListOrNull already produced a fresh list, for example for primitive arrays. That is a minor extra copy on a hot aggregation path.
7. Style. Several new comments are long and narrative. CLAUDE.md asks to match the surrounding density, and a few Javadocs could be trimmed. Also confirm the switch and if formatting in the new code follows the file's existing style.

Tests

Coverage looks good for each issue. Two gaps:

• A UNIQUE-index MERGE test across several declared types (DATE, DECIMAL, BOOLEAN), not just INTEGER.
• A case where the text is not a valid shape for the declared type (for example 'abc' into an INTEGER), checking there are no exceptions and no match, under both a scan and an index plan.

None of these is blocking. Point 1 is the one I would address before merge.

Review (main-source diff only; I did not run the build or tests and did not read the new test classes in detail)

Overall
The changes are coherent. One helper, InlineProperties.declaredValue, now drives matchesProperty, =/<>, IN, simple CASE, the MERGE index lookup and the optimizer operators, which removes the earlier STRING-only copies. Converting only through the text form is justified in the description, because it keeps indexed and non-indexed plans in agreement. The mayBeReadAs pre-check avoids an exception per record in a scan. Reusing CypherFunctionHelper.asListOrNull in AbstractAggFunction is a good simplification.

Points to consider

1. LocalProperty.setRegexp now throws IllegalArgumentException for LIST, MAP, BINARY, EMBEDDED, LINK and ARRAY_OF_* properties, and for BOOLEAN patterns matching neither true nor false. Warning only on load is the right call. Two gaps: changing the property type later (ALTER PROPERTY ... TYPE) is not re-checked against an existing REGEXP, and a release note should mention the new refusal.
2. ScanPropertyHashIndex: the new temporals list holds a RID for every temporal-valued record. Consider capping it like MAX_NON_INTEGRAL; today the cap is only applied at lookup time, after the list is built.
3. isDecimalText and integralOf in ScanPropertyHashIndex repeat the shape rules of the private InlineProperties.mayBeReadAs. If they drift, the hash path and a plain scan will disagree on what a text can be read as. Suggest one shared helper.
4. SelectExecutor.readingIndex calls index.getIndexesOnBuckets() on every lookup. Correct, but check it is a cheap snapshot on the hot path of small point queries.
5. n.flag = 'TRUE' is now true for a BOOLEAN-declared property because the boolean parse is case-insensitive. It matches what a write stores, but it loosens =. Make sure a test pins it down.
6. Style: the InExpression lazy declaredResolved flag with mutable locals would read better as a small helper. Some Javadoc lines in ComparisonExpression exceed 160 columns.

Tests
The description reports broad suites (TCK, schema, sql, graph), which I did not re-run. Please confirm there is a regression test for each of the five issues, including MERGE twice on a UNIQUE-indexed INTEGER with a text value, plan-versus-scan parity for IndexNestedLoopJoin and ScanPropertyHashIndex, and the setRegexp refusal and load-warning paths.

Security
Nothing concerning. Patterns are compiled only on schema DDL, and no new user-controlled regexp is evaluated per record.

🤖 Generated with Claude Code

Review

Overall this is a well-scoped and carefully documented change. The tests listed cover each sub-issue, and the TCK and importer runs are reported green. Notes below, none blocking.

Design (#9794)

• Converting only through the text form is the right call. It keeps scan and index plans consistent ("a fraction equals no integer"), and the PR description explains why.
• isTextAgainstNonText gates the schema lookup, so a scan pays only on a text/non-text mismatch. mayBeReadAs also avoids one exception per record. Both are good for the hot path.
• mayBeReadAs duplicates knowledge of what Type.convert accepts (digit limits, sign handling, boolean case-insensitivity). If Type.convert changes, this pre-filter can silently drift and give false negatives, so MERGE would duplicate again. I'd add a test that cross-checks it against Type.convertOrNull over a table of edge inputs (+5, -0, 007, 2147483648, ' 5', TRUE, NaN, Infinity, .5, 1e3 for DECIMAL/DOUBLE). A pre-filter that is never stricter than the converter is the invariant to pin.
• The FLOAT/DOUBLE/DECIMAL check only looks at the first char (I/N pass for any text). That is fine as a cheap filter, but the conversion exception path is still reachable for texts like 'Nope'. convertOrNull handles it, but the per-record exception cost this was meant to avoid still applies for that shape. Consider a slightly stricter check (NaN / Infinity / -Infinity).
• ComparisonExpression.evaluateWithValues now tries the left side's declaration and then the right's. When both operands read declared properties (e.g. a.x = b.y with a STRING and an INTEGER), the first match wins. This is reasonable, but a test pinning that case would help.
• InExpression mixes the declaredResolved flag with a nullable declared. It works, but a small helper or a lazy holder would be easier to read.

REGEXP guard (#9808)

• setRegexp now throws for BINARY/LIST/MAP/etc. and for BOOLEAN patterns that match neither value. That is a behavior change for any existing Java API caller or script. It is reasonable, and the schema-load path only warns, which is the right trade-off. Please mention it in release notes.
• The BOOLEAN check compiles the pattern, and ALTER PROPERTY then compiles it again. That is negligible, but you could cache the compiled pattern.

agg. (#9777)*

• Using CypherFunctionHelper.asListOrNull is good reuse. toObjectList still copies via new ArrayList<>(list), which is needed for the no-view guarantee. If asListOrNull already allocates a fresh list for arrays, that is a double copy for float[]/short[], so it is worth checking for the large vector columns.

#9812

• Using isStaleRead and isUnchangedSince in SelectExecutor matches the other readers. The test reproduced the failure before the fix, which is good.

Style / CLAUDE.md

• Mostly conforms: final usage and imports look right.
• ComparisonExpression.evaluateWithValues has a javadoc line over 130 columns, and the surrounding code wraps at about 130.
• Confirm the new tests don't assert on raw elapsed time and that none bind hard-coded ports. I saw nothing suggesting either.

Nice work. The main follow-up I'd request is the mayBeReadAs vs Type.convert consistency test.

🤖 Generated with Claude Code

Review

Overall this is a solid, well-documented PR. The scope is wide (five issues), but each fix reuses an existing helper (`CypherFunctionHelper.asListOrNull`, `TypeIndex.isStaleRead`, `TemporalUtil.restoreLegacyText`) and the tests cover each issue. I only read the diff and did not run the tests.

Good

• `declaredValue` converts only through the text form. That keeps scans and index paths consistent, and the PR description explains why it deviates from the issue proposal.
• `isTextAgainstNonText` gates the schema lookup, so the hot comparison path pays nothing for the common case.
• `mayBeReadAs` pre-filters text shapes so a scan does not throw and catch an exception per record.
• The `agg.*` change removes a duplicated switch, and `toObjectList` still returns a fresh copy.

Points to consider

1. `unusableRegexp` and later type changes. `checkRegexp` only runs in `setRegexp`. If a property has a REGEXP and its type is later changed (e.g. `ALTER PROPERTY ... TYPE` to LIST or BOOLEAN), the check does not rerun. This is the same gap `checkBound` may have. Either validate in `setType` or add a test documenting the intended behavior.
2. Exception types differ. `LocalProperty.setRegexp` throws `IllegalArgumentException`, while `requireUsableRegexp` throws `CommandExecutionException`. That is fine if intentional, but the SQL path compiles the pattern first through `StoredValueConstraints.of` and then through `Pattern.compile` again in `unusableRegexp` (BOOLEAN) and `checkRegexp`. A pattern is compiled up to 2-3 times per ALTER. This is cold path, so it is minor, but `unusableRegexp` could take a precompiled `Pattern`.
3. `mayBeReadAs` can drift from `Type.convert`. Its rules are hand-copied: digit counts per integral type, the `NaN`/`Infinity` prefixes, the "contains a digit" rule for temporals. If `Type.convert` changes, this will silently produce false negatives, meaning a pattern that no longer finds a stored value. Please add a test that cross-checks it against `Type.convertOrNull` over a table of texts, so drift fails loudly.
4. BYTE/SHORT limits. The `maxDigits` check is only a pre-filter (BYTE 3, SHORT 5), and the conversion still decides. Make sure the test table covers signs and leading zeros (`'+007'`, `'-0'`) for each integral type.
5. `InExpression` lookup. `declaredResolved` is set in the `LongRangeList` branch only when the value is a String and the range misses. If the loop then needs the property, it reuses that result, which is correct. The control flow with two mutable locals is a bit hard to follow, so a small lazy-holder helper might read better.
6. `SelectExecutor`. The new `indexesRead`/`indexesReadSnapshots` fields are reset in `lookForIndexes()`. Allocating two `ArrayList`s per query is small, but this is on the select hot path. Consider whether the snapshot is only needed when a single index is read. Also confirm that `fallBackToScan(cursors, null)` closes the cursors and handles a null exception.
7. Style. In `SelectExecutor` the new comments are in the file's existing all-caps style, so that matches. In `ComparisonExpression`, one Javadoc line exceeds the 160-column Prettier width. Also, `toDeclaredText` was renamed to `toDeclaredValue` and is package-private. Please check that nothing outside `ast` (e.g. in other modules) calls it.

Tests

Coverage is good: a declared type x literal matrix, with and without an index, joins, UNIQUE, and a regression for each issue. Two suggestions:

• Add a test for the schema-load path (REGEXP on a non-text type loaded from disk only warns), since that backward-compatibility behavior is easy to break.
• Add a negative test showing that `n.p = 1.5` stays false for a stored integer `1`, to lock in the deliberate deviation from the issue.

No security concerns found. The pre-filters actually reduce exception-driven overhead.

Codecov Report

❌ Patch coverage is 80.91286% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.82%. Comparing base (e2e3b75) to head (fbaac82).
⚠️ Report is 8 commits behind head on main.

@@             Coverage Diff              @@
##               main    #9820      +/-   ##
============================================
+ Coverage     77.80%   77.82%   +0.02%     
+ Complexity     4440     4439       -1     
============================================
  Files          2198     2198              
  Lines        229906   230027     +121     
  Branches      49773    49807      +34     
============================================
+ Hits         178867   179010     +143     
+ Misses        32135    32111      -24     
- Partials      18904    18906       +2     

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