#5600 Geospatial misc: STRING stays STRING, GEOSPATIAL precision from SQL, fewer cells for area shapes

[lvca]

Closes #5600. The three findings are independent; each is a separate commit-sized change kept together because they all touch the geospatial path.

1. A STRING property no longer reads back as a Shape

BinarySerializer.deserializeValue sniffed the first characters of every string it read and, when they matched POINT / CIRCLE / LINESTRING / POLYGON / ENVELOPE / BUFFER, parsed the value as WKT and returned a spatial4j Shape. A property declared STRING therefore did not round-trip, and the trigger was a prefix match on arbitrary user text rather than a schema decision.

The sniff is gone: a string reads back as the string that was written, and the startsWith chain no longer runs on every string read in the database.

Backward compatibility is kept where the conversion actually belongs - at the point a geometry is asked for. GeoUtils.parseGeometry(), GeoUtils.parseJtsGeometry() and LSMTreeGeoIndex.toShape() already accepted WKT text, so a pre-26.2.1 database (shapes stored as WKT under TYPE_STRING) keeps working, and so does geo.point(), which returns WKT.

Two callers did rely on the sniff and are fixed at the right layer: SQLMethodIsWithin and SQLMethodIntersectsWith returned null unless the value was already a Shape. They now parse their operands through GeoUtils.parseGeometry, like every geo.* function, so coords.isWithin(...) works on a WKT column and accepts a WKT literal as its parameter too.

Better alternative suggested by the issue and not taken here: an explicit Type.GEOMETRY in the schema would be the cleanest long-term answer, but that is a schema-format change with its own migration story - worth a separate issue rather than folding into this one.

2. GEOSPATIAL precision is settable from SQL

CREATE INDEX ON Location (coords) GEOSPATIAL METADATA {"precision": 6}

parsed, ran, and silently built the index at the default precision of 11.

The issue proposed mirroring the three existing METADATA branches. The better fix is one level up: withType() already returns a specialised builder that owns its metadata for LSM_VECTOR, FULL_TEXT and LSM_SPARSE_VECTOR; GEOSPATIAL was simply missing from that list, which is why its metadata had nowhere to go. It now has TypeGeoIndexBuilder, in the same shape as the others, so precision and tokenization are reachable from SQL and from the Java API (withPrecision, withTokenization) alike.

Two related sharp edges went with it:

• An unusable METADATA is rejected, not dropped. An unknown key, a non-numeric or out-of-range precision, an invalid tokenization, or a METADATA clause on an index type that has no settings at all now raise CommandSQLParsingException. Silently ignoring the clause is what kept this gap invisible. Parsing is unchanged, so the existing parser tests still hold.
• withType() no longer leaves the original builder unconfigured. It returns a new object, so builder.withType(X); builder.create(); - which the pre-existing LSMTreeGeoIndexSchemaTest does - failed with indexType was not specified once GEOSPATIAL got a subclass. The type is now recorded on the original builder as well, which also removes the same latent trap for full-text and vector callers.

Note the builder parses the METADATA keys itself rather than delegating to GeoIndexMetadata.fromJSON(): that method reads a persisted definition, where a missing tokenization means a pre-26.8.1 index and therefore the FULL layout. In a CREATE INDEX a missing key just means the user did not ask, so the creation-time default has to stand.

3. An area shape indexes fewer cells

The issue asked to measure before adopting Lucene's pruneLeafyBranches. Measured, on the frontier cell count:

shape	saving
small square	57%
jagged outline	74%
wide rectangle	0%
linestring	0%
point	0% (by construction)

Worth taking. A complete set of sibling frontier cells now collapses into its parent, recursively. A parent covers the union of its children, so the cover can only grow and a match is never lost; the geo.* predicate post-filters the superset either way.

Better alternative than the issue's proposal. The issue suggested subclassing the strategy to expose createCellIteratorToIndex. That implementation materialises the entire decomposition in an ArrayList (pre-sized to 4096), which is exactly the caveat the issue flagged from Lucene's own javadoc. This implements the prune as a streaming pass over the existing walk instead: only the frontier tokens on the current root-to-leaf path can still be revoked by an ancestor collapsing, so what is held is bounded by subCellsSize * detailLevel regardless of shape size. Points - the hot ingest path of #5478 - take an allocation-free shortcut, since a chain of single-child cells can never hold a complete sibling set.

LSMTreeGeoIndexCellPruningTest asserts the streaming output is identical to Lucene's buffered one, invoking createCellIteratorToIndex by reflection, over 8 shapes x 4 precisions, plus a coverage-never-shrinks property and the one-token-per-point guarantee.

This changes what a new index writes for area shapes. Indexes in the FULL layout are untouched, and FRONTIER has not shipped in any release (#5478 merged into 26.8.1-SNAPSHOT today), so no published database holds the unpruned form.

Tests

New: Issue5600StringWktRoundTripTest (7), LSMTreeGeoIndexCellPruningTest (3). Extended: LSMTreeGeoIndexSchemaTest (+9), LSMTreeGeoIndexTokenizationTest (+2, polygon delete symmetry and a pruned jagged polygon at every query resolution).

Regression: 3182 engine tests in serializer, query.sql, schema, index.geospatial, index.fulltext and the LSM index suites - 0 failures.

[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

🟢 Metrics 53 complexity

Metric	Results
Complexity	53

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 #5604 - Geospatial misc

Thorough, disciplined PR. Three independent fixes on the geospatial path, each landed at the right layer, each with regression tests, and the description explains not just the fix but the alternative not taken. The FRONTIER prune is validated directly against Lucene's own pruneLeafyBranches output across 8 shapes x 4 precisions, which is exactly the kind of oracle test this deserves. Overall this looks ready.

A few observations, mostly minor:

1. STRING no longer sniffs to a Shape (BinarySerializer) - correct

Removing the startsWith sniff on every TYPE_STRING read is the right call: it fixes the round-trip identity bug and removes a per-read prefix chain from the hot deserialize path (a small perf win as a bonus). Backward compat is preserved because every geometry consumer (GeoUtils.parseGeometry/parseJtsGeometry, LSMTreeGeoIndex.toShape) still accepts WKT text. SpatialContext/GeoUtils imports are still used elsewhere in the file, so no dead imports. Good.

• Behavior change worth a release note: any caller relying on document.get("wktColumn") instanceof Shape now gets a String. The PR body acknowledges this; just make sure it lands in the release doc (it does).

2. GEOSPATIAL precision from SQL - correct, one compat flag

Fixing this one level up via TypeGeoIndexBuilder (rather than adding a fourth METADATA branch) is the cleaner fix, and recording the type on the original builder in withType() closes the latent indexType was not specified trap for full-text/vector callers too.

• Backward-compat flag on the "reject unusable METADATA" change. create index ... UNIQUE METADATA {"test": 3} is documented legacy syntax and appears in CreateIndexStatementTestParserTest (as checkRightSyntax, so those tests still pass since parsing is unchanged). But at execution time it now throws CommandSQLParsingException where it previously silently ran. This is intentional and arguably more correct, but it is a runtime-visible breaking change for anyone who passed a harmless METADATA on a plain index. Please make sure the release note calls this out explicitly, not just the geospatial angle.
• Non-integer precision is silently truncated. {"precision": 6.9} passes the instanceof Number check and becomes 6 via number.intValue(). Given the PR's theme is "reject unusable METADATA rather than drop it," rejecting a non-integral precision would be more consistent. Low priority.
• Overload nit. TypeGeoIndexBuilder now has both withMetadata(IndexMetadata) and withMetadata(JSONObject). A literal withMetadata(null) would be an ambiguous call; no current caller does this, but it is a small latent trap. Not blocking.

3. Fewer cells for area shapes - correct, well-bounded

The streaming prune is the right answer over subclassing Lucene's buffered createCellIteratorToIndex, and the PruneFrame reuse keeps it allocation-light (aligns with the perf mantra). The point fast-path avoiding the frame bookkeeping is a nice touch on the #5478 hot path.

• The path[] array is sized detailLevel + 1 and the algorithm leans on assert depth == level. With assertions off (prod default) the invariant isn't checked, but a violation would surface as a loud ArrayIndexOutOfBoundsException rather than silent mis-indexing, and the Lucene-equivalence test exercises the traversal across shapes/precisions - so this is acceptable. Worth keeping the assert as documentation of the one-level-at-a-time GeoHash contract.
• Query path deliberately still walks the full covering set (unpruned) while the write path prunes; the delete-symmetry test (polygonDeleteRemovesEveryEntryOfTheRecord) and prunedJaggedPolygonIsStillFoundAtEveryResolution cover the asymmetry, including the "must not start matching a far-away point" direction. Good.

SQL method fixes (isWithin/intersectsWith)

Parsing both operands through GeoUtils.parseGeometry is consistent with the rest of geo.*. Note this also makes them more lenient than before: a non-geometry params[0] now returns null instead of throwing ClassCastException. That is a reasonable improvement, and the WKT-literal-as-parameter case is covered by legacyShapeMethodsAcceptWktText.

Tests

Strong. Round-trip identity (incl. free text starting with POLYGON/POINT/..., lists/maps, indexable-as-string), real-Shape still-a-Shape, precision/tokenization from SQL with reopen-survival, and the rejection paths (unknown key, out-of-range, non-numeric, invalid tokenization, unsupported-type). The Lucene oracle test plus the coverage-never-shrinks property test are exactly right for the prune. Nice work.

Nothing blocking from my side - the two items I'd most want addressed are the explicit release-note callout for the runtime METADATA-rejection change, and optionally rejecting non-integral precision.

[lvca]

Thanks - addressed the two you flagged, in 0156e7d.

Non-integral precision now rejected. {"precision": 6.9} passed the instanceof Number check and was truncated to 6. A GeoHash precision is a tree level, so truncating silently swallows exactly the kind of typo this validation exists to report. It now raises, and fractionalPrecisionIsRejected covers it.

METADATA rejection called out as breaking in its own right. The release note now carries an explicit blockquote for it rather than leaving it as a bullet under the geospatial heading, and names the concrete legacy shape (CREATE INDEX ... UNIQUE METADATA {"test": 3}) plus the grammar-vs-execution distinction: such a statement still parses, only running it is refused.

Not taken - the withMetadata overload nit. TypeGeoIndexBuilder having both withMetadata(IndexMetadata) and withMetadata(JSONObject) is the exact shape TypeFullTextIndexBuilder has carried since it was written, and TypeLSMVectorIndexBuilder/TypeLSMSparseVectorIndexBuilder follow it too. A literal withMetadata(null) is ambiguous on all four, so renaming only the geospatial one would trade a latent trap nobody has hit for an inconsistency across the builder family. If it is worth closing it should be closed on all of them at once, which is a separate change.

No action on the assert depth == level - agreed with your read: assertions off means a violation surfaces as a loud ArrayIndexOutOfBoundsException, never as silent mis-indexing, and the Lucene-equivalence test walks the traversal across 8 shapes x 4 precisions. Keeping it as documentation of the one-level-at-a-time GeoHash contract.

On the isWithin/intersectsWith leniency you noted: correct, a non-geometry parameter now returns null instead of throwing ClassCastException. That is deliberate - it matches what those two methods already did for a non-geometry receiver, and a WHERE clause that hits one bad row should filter it out rather than fail the query.

[claude]

Review of PR 5604 - Geospatial misc

Reviewed all three findings against the surrounding code. This is a well-scoped, unusually well-documented and well-tested PR. The commit messages, release notes, and Javadoc all explain why, not just what, which made the review much easier. Below are the points worth discussing; none are blockers.

1. STRING stays STRING (BinarySerializer)

Removing the WKT sniff from deserializeValue is clearly the right call: type identity should never be decided by prefix-matching arbitrary user text on every string read in the DB. I verified the compatibility argument holds:

• LSMTreeGeoIndex.put() accepts a WKT String and parses it, so a pre-26.2.1 DB storing shapes as TYPE_STRING still indexes.
• GeoUtils.parseGeometry() accepts Shape / Map / WKT String, so the two rewired SQL methods degrade gracefully (parse failure -> IllegalArgumentException -> null, matching the old "not a Shape -> null" behavior).
• Real Shape values have their own binary type and still round-trip (covered by shapeValueStillRoundTripsAsShape).

Nit: GeoUtils and SpatialContext imports remain used elsewhere in BinarySerializer (serialize path), so no dead imports. This is a genuine behavioral break for anyone relying on document.get(x) instanceof Shape from a STRING column, and it is documented in the release note, which is the right place for it.

2. GEOSPATIAL precision from SQL

The TypeGeoIndexBuilder + withType() fix is the correct layer, and folding GEOSPATIAL into the same specialised-builder pattern as vector/full-text is consistent with the codebase.

• The rejection of unusable METADATA (unknown key, out-of-range/fractional/non-numeric precision, bad tokenization, or METADATA on a type with no settings) is good hardening. I confirmed the existing parser tests use only checkRightSyntax/parse, so they are unaffected: the new rejection is at execution time. Correctly flagged as a runtime breaking change in the release note.
• Recording the type on the original builder before returning the specialised one removes a real latent trap for full-text/vector callers too. super.withType() only assigns the field, so always calling it first is safe.

Maintainability note: TypeGeoIndexBuilder copy constructor hand-copies ~11 fields from copyFrom. This mirrors the other specialised builders so it is consistent, but the pattern is fragile: a new field on TypeIndexBuilder silently will not propagate through withType() for any of these subclasses. Not for this PR, but a shared copyCommonFrom() helper across the four builders would remove a recurring footgun.

3. Fewer cells for area shapes (forEachPrunedFrontierCell)

The streaming prune is the nicest part of the PR: bounding retained state to subCellsSize * detailLevel instead of Lucene full-decomposition buffer, with the allocation-free point shortcut on the hot ingest path. I walked the frame open/close/collapse logic:

• frontier = childCount == 0 || (subCellsSize > 0 && frontierChildren == subCellsSize) is sound: childCount >= frontierChildren and childCount <= subCellsSize, so equality implies all visited children were frontier and the set is complete.
• Each cell token reaches emit at most once, so no duplicate index entries.
• Delete recomputes the same pruned set, so put/remove stay symmetric (covered by polygonDeleteRemovesEveryEntryOfTheRecord).

Two small observations:

• Correctness leans on assert depth == level, i.e. GeoHash pre-order descends exactly one level per step. That is true for a prefix tree (one base-32 char per level) so it holds, but the assert is a no-op without -ea. A malformed traversal in production would not throw; it would misprune silently. Given the invariant is structural this is acceptable; just worth knowing the guard is assertions-only.
• The reference test arcadeTokens() collects into a TreeSet while production extractTokens() collects into an ArrayList. If a duplicate emit ever occurred, the equality-vs-Lucene test would mask it (the set dedups) while production wrote a dup entry. I believe no dup is possible by the argument above, so this is theoretical, but asserting no duplicates on the raw stream would close the gap cheaply.

Tests

Coverage is excellent: round-trip identity, free-text keyword false-positives, lists/maps, indexability, SQL precision/tokenization, the full rejection matrix, delete symmetry for areas, and a direct token-set equality against Lucene own pruneLeafyBranches across 8 shapes x 4 precisions.

Overall: LGTM. The two theoretical notes above are the only things I would consider tightening, and neither blocks merge.

[lvca]

Thanks - both notes addressed in 581087f.

Duplicate emit could hide behind the set comparison. Right, and worth closing: the equivalence test compared TreeSets while production collects into an ArrayList and issues one put() per token, so a duplicate would have been invisible in the test and a real extra index entry in production. noTokenIsEmittedTwice now asserts on the raw stream across the same 8 shapes x 4 precisions. It passes - no duplicate exists - but it would now be caught.

The assert depth == level guard. Digging into this, the concern is narrower than it looks: nothing in the algorithm depends on depth == level. A frame's parent is the frame below it on the stack, found by pop order rather than by level arithmetic, and the array bound is satisfied by a weaker property that holds unconditionally - the pop loop closes every frame whose level is >= the incoming one, so the levels on the stack are strictly increasing and the depth can never exceed the number of distinct levels, detailLevel. A traversal that descended two levels at once would produce depth < level and still associate parents correctly and stay in bounds.

So there is no silent-misprune path here, and the assert is documentation of the GeoHash one-base-32-character-per-level contract rather than a guard the code leans on. I have corrected the comments to say exactly that, since the previous wording implied the dependency you (reasonably) read into it.

copyCommonFrom() across the four builders - agreed, and agreed it is not for this PR. The hand-copied field list is a real footgun: a new TypeIndexBuilder field silently fails to propagate through withType() for all four subclasses, not just the geospatial one. I would rather fix it for the family in one change than half-fix it here. Filing it separately.

[claude]

Review: #5600 Geospatial misc

Reviewed the full diff and traced the integration points (GeoUtils.parseGeometry, LSMTreeGeoIndex.toShape/index put path, the builder lifecycle in CreateIndexStatement, and the withType reordering). This is a strong, well-reasoned PR: each of the three fixes targets the actual root cause rather than the symptom, the rationale is documented at the code, and the test coverage is genuinely thorough. A few observations below, mostly for the record.

What's good

• Fix build support #1 (STRING stays STRING). Removing the WKT prefix-sniff from deserializeValue is correct: the storage layer should never reinterpret a declared type, and moving the conversion to the point of consumption (parseGeometry, parseJtsGeometry, toShape, all of which already accept WKT text) preserves pre-26.2.1 databases. Bonus: dropping the startsWith chain that ran on every string read is a real win on a hot path, fully in line with the repo's performance mantra. I confirmed no remaining engine code depends on the sniff except the two SQL methods, which are fixed here.
• Fix Bump gremlin.version from 3.4.10 to 3.5.1 #2 (precision from SQL). Fixing it one level up in withType() (so GEOSPATIAL gets a real builder like the vector/full-text types) is the right layer, and recording the type on the original builder before returning the subclass removes a genuine latent trap for full-text/vector callers too. The reopen assertions in precisionFromSqlMetadata give real end-to-end confidence.
• Fix Bump slf4j-simple.version from 1.7.31 to 1.7.32 #3 (cell pruning). The streaming prune bounded to subCellsSize * detailLevel instead of Lucene's full-decomposition ArrayList is elegant, and asserting token-set equality against Lucene's own createCellIteratorToIndex (via reflection, 8 shapes x 4 precisions) plus the noTokenIsEmittedTwice, coverage-never-shrinks, and one-token-per-point properties is exactly the way to validate this class of change. The point fast-path avoiding the frame bookkeeping is a nice touch for the GraphBatch: Slow data loading if there is a GEOSPATIAL index #5478 ingest path.

Points worth flagging

1. Breaking change: METADATA on an index type that ignores it now throws. This is the item most likely to bite existing users: a schema script carrying a no-op CREATE INDEX ... UNIQUE METADATA {...} that used to be silently accepted will now fail at execution time. It's clearly the correct behavior and it's documented in release-26.8.1.md, but it's a runtime behavior change on a previously-parsing statement, so it deserves prominence in the changelog / upgrade notes (which it has). No code change requested, just calling it out as the main compatibility risk.

2. Silent null on a malformed WKT literal in isWithin/intersectsWith. Both methods now catch (IllegalArgumentException) return null. That's fine for a column value that isn't a geometry, but a genuinely malformed WKT parameter (a user typo in coords.isWithin('POLYGONN(...)')) now silently yields "no match" rather than surfacing a parse error, which can be confusing to debug. Matching the behavior of the other geo.* functions is reasonable, so this is a judgment call, not a defect, but consider whether a bad literal parameter should be distinguishable from a non-geometry column value.

3. Minor duplication. The try { target = parseGeometry(value); shape = parseGeometry(params[0]); } catch (...) return null; block is identical in SQLMethodIsWithin and SQLMethodIntersectsWith. A shared helper (e.g. on the abstract geo method base or GeoUtils) would keep the two in sync if the parsing contract ever changes.

4. Minor API asymmetry. TypeGeoIndexBuilder.withMetadata(JSONObject) returns void while the sibling withMetadata(IndexMetadata) override returns the builder. Not important, but a chainable return would match the fluent style of the rest of the builder.

Style / conventions

Consistent with the codebase and CLAUDE.md: final used throughout, imports (not FQNs), primitive-array bookkeeping in PruneFrame over object lists, Apache headers on the new files, no System.out debug left behind, and assertThat(...) style tests. The new geo imports in BinarySerializer are still used elsewhere (lines 1339/1402), so no dangling imports from the sniff removal.

Nice work: the reasoning captured in the code and PR description makes this easy to review with confidence.

Automated review by Claude; the maintainer should confirm before merging.

[lvca]

Thanks - all three code points addressed in 5b092c1.

2. Silent null on a malformed WKT literal. Good catch, and on reflection the two operands are not the same kind of input at all, which is what makes this fixable rather than a judgment call:

• The parameter is written in the query. coords.isWithin('POLYGONN (...)') is a typo, and the method already threw for a missing parameter - swallowing a malformed one was the inconsistency. It now propagates.
• The value comes from the record. A row that happens not to hold a geometry is skipped, because failing an entire query over one bad row is worse than filtering it out.

aMalformedParameterIsReportedButABadRowIsJustSkipped pins both halves: a table with one good row and one junk row still returns the good row, and the typo'd literal surfaces.

3. Duplication. With the above the block would have grown in both copies, so extracting it was overdue. Both methods now sit on AbstractSQLGeoRelationMethod and differ only in the SpatialRelation they accept - the parsing contract has exactly one definition, and each concrete class is down to a name, a relation and a syntax string.

4. withMetadata(JSONObject) return type. Made chainable. You are right that the asymmetry with the withMetadata(IndexMetadata) override sitting three lines above it is the odd part; the change is purely additive so no caller moves.

1. The METADATA breaking change. Noted, no code change - agreed it is the main compatibility risk here. It has its own callout in release-26.8.1.md naming the concrete legacy shape and the grammar-vs-execution distinction.

449 tests across the geospatial, geo.* function and SQL-method suites pass on the new commit.

[claude]

Review: #5600 Geospatial misc

Reviewed all 14 files. This is a well-scoped, high-quality PR: three genuinely independent fixes on the geospatial path, each with a clear rationale and strong tests. I found no blocking bugs. Below are confirmations and a few minor observations.

What is solid

• Fix 1 (STRING stays STRING). Removing the per-read WKT sniff in BinarySerializer.deserializeValue is the correct layer to fix this: the storage layer no longer silently changes a declared type, and the startsWith chain no longer runs on every string read (a real hot-path win). Backward compatibility is preserved because every geometry consumer (GeoUtils.parseGeometry, parseJtsGeometry, LSMTreeGeoIndex.toShape/put/line 208, SQLFunctionGeoAsText) already accepts WKT text. I verified the only two callers that relied on the sniff (SQLMethodIsWithin, SQLMethodIntersectsWith) are the ones fixed via the new AbstractSQLGeoRelationMethod. Good factoring into a shared base.

• Fix 2 (precision from SQL). Adding TypeGeoIndexBuilder to the withType() dispatch is the right root-cause fix rather than mirroring branches in the parser. Recording the type on the original builder in withType() (before returning the specialised instance) closes a real latent trap for the vector/full-text builders too. The withMetadata(JSONObject) validation (unknown key / non-numeric / non-whole precision / invalid tokenization) is thorough, and the comment explaining why it does not delegate to GeoIndexMetadata.fromJSON() (persisted-vs-creation default for a missing tokenization) is exactly the non-obvious reasoning worth capturing.

• Fix 3 (cell pruning). The streaming forEachPrunedFrontierCell is the interesting one. I traced the frame/closeFrame logic: frontier = childCount==0 || (subCellsSize>0 && frontierChildren==subCellsSize) correctly collapses only a complete set of sibling frontier cells, the non-CellCanPrune fallback (subCellsSize == -1) safely degrades to "leaf only", and pending tokens are discarded when a parent collapses and flushed when it does not - so no token is double-emitted and coverage only grows. The PruneFrame reuse (one frame per level, growable pending) keeps it allocation-light, and the Point shortcut protects the GraphBatch: Slow data loading if there is a GEOSPATIAL index #5478 hot ingest path. Asserting the output is byte-identical to Lucene's own createCellIteratorToIndex across 8 shapes x 4 precisions is a strong correctness anchor, and polygonDeleteRemovesEveryEntryOfTheRecord + prunedJaggedPolygonIsStillFoundAtEveryResolution cover the two risks that mattered (put/remove tokenize identically; queries still resolve the coarser parents and don't over-match). Well tested.

Minor observations (non-blocking)

1. Behavior changes worth a release-note callout (both intentional and tested).

   • CREATE INDEX ... METADATA {...} on an index type that has no settings now raises CommandSQLParsingException instead of being silently dropped. Correct, but a strictly stricter runtime contract - anyone passing a no-op METADATA to an LSM_TREE/UNIQUE index will now get an error. It is in the release doc; just flagging that it is a semver-visible change.
   • A malformed geometry literal in isWithin/intersectsWith now throws instead of returning null (a bad row is still skipped). Nicely covered by aMalformedParameterIsReportedButABadRowIsJustSkipped.

2. Latent copy-constructor trap in the builder subclasses. TypeGeoIndexBuilder(TypeIndexBuilder copyFrom) (like the vector/full-text ones) copies fields one by one, but TypeIndexBuilder's private fields buildMode, buildMemoryBudgetBytes, buildSpillDirectory, buildMergeFanIn, buildParallelism, and defaultKeyTypesForUndeclaredProperties are not carried over. For GEOSPATIAL this is harmless today (sorted build is rejected for non-LSM_TREE, and geo indexes are not created via the Cypher typeless path), but the pattern means any future field added to the base silently fails to propagate through withType(). Not introduced by this PR, and not worth blocking on, but a candidate for a follow-up (e.g. a protected copyBaseFieldsFrom).

3. Tiny nit: in AbstractSQLGeoRelationMethod.execute, an empty/whitespace-only literal parameter parses to null and yields a silent no-match (rather than the "report the typo" path), since parseGeometry("") returns null. Edge case only; not worth changing.

Note on verification

I could not run mvn in this review sandbox (it requires interactive approval here), so I did not independently reproduce the reported 3182-test run. The static trace of the pruning algorithm, the builder dispatch, and the backward-compat consumers all check out, and the new/extended tests are well-targeted. Per CLAUDE.md, please confirm a local mvn verify on the affected modules (serializer, query.sql, schema, index.geospatial) is green before merge.

Overall: clean, well-reasoned, and thoroughly tested. Nice work.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.77%. Comparing base (49206db) to head (9d02b02).

Files with missing lines	Patch %	Lines
...com/arcadedb/index/geospatial/LSMTreeGeoIndex.java	84.84%	2 Missing and 8 partials ⚠️
...n/java/com/arcadedb/function/sql/geo/GeoUtils.java	30.00%	6 Missing and 1 partial ⚠️
.../java/com/arcadedb/schema/TypeGeoIndexBuilder.java	94.44%	1 Missing and 2 partials ⚠️
...ain/java/com/arcadedb/schema/TypeIndexBuilder.java	62.50%	1 Missing and 2 partials ⚠️
...y/sql/method/geo/AbstractSQLGeoRelationMethod.java	88.23%	1 Missing and 1 partial ⚠️
.../query/sql/method/geo/SQLMethodIntersectsWith.java	66.66%	1 Missing ⚠️
...cadedb/query/sql/method/geo/SQLMethodIsWithin.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5604      +/-   ##
==========================================
+ Coverage   66.73%   66.77%   +0.03%     
==========================================
  Files        1705     1707       +2     
  Lines      140950   141071     +121     
  Branches    30484    30505      +21     
==========================================
+ Hits        94057    94194     +137     
+ Misses      34235    34203      -32     
- Partials    12658    12674      +16     

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

[claude]

Code review (automated) - PR #5604

Reviewed all 15 files. This is a well-structured, unusually well-documented change: the intent behind each decision is captured in Javadoc/comments and the test coverage is strong (identity assertion against Lucene's own pruner, coverage-never-shrinks property, delete symmetry, degenerate operands). Below are the things worth confirming, none of them blocking.

Correctness - looks right

• Relation-method semantics preserved. SQLMethodIsWithin keeps target.relate(shape) == WITHIN and SQLMethodIntersectsWith keeps != DISJOINT, with target = value and shape = parameter, matching the pre-refactor behavior. The refactor also fixes a latent bug: the old (Shape) params[0] cast would ClassCastException on a WKT-string literal parameter; the new path parses it. Nice.
• Serializer round-trip. GeoUtils/SpatialContext imports in BinarySerializer are still used elsewhere (shape write path), so removing the sniff leaves no dead imports, and a real Shape still round-trips via its own binary type (shapeValueStillRoundTripsAsShape confirms).
• Builder plumbing. withType() now recording the type on the original builder before returning a subclass removes a real latent trap; the subclass constructors set their own indexType, so the earlier super.withType() call is harmless for vector/full-text.

Things worth a second look

1. Breaking change: METADATA on an index type that ignores it now throws. This is the right call and it is documented, but it is an execution-time behavior change for any existing schema script that carries a no-op METADATA clause on a UNIQUE/LSM_TREE/HASH index. Worth making sure no internal tooling, test fixtures, or Studio-generated DDL emit such clauses. (FULL_TEXT with metadata == null correctly falls through to the plain create() - good.)

2. AbstractSQLGeoRelationMethod catches only IllegalArgumentException for the value operand. GeoUtils.parseGeometry funnels WKT parse failures into IllegalArgumentException, so bad-text rows are correctly skipped. But a stored Map operand whose x/y are non-Number would surface a ClassCastException, which is not caught and would fail the whole query rather than skip the row. Unlikely for a value column, but if you want the "one bad row is skipped" contract airtight, consider catching RuntimeException (or normalizing that cast in parseGeometry).

3. FRONTIER delete symmetry across a snapshot upgrade. Since the prune changes what a new index writes, a database built on an earlier 26.8.1-SNAPSHOT (unpruned FRONTIER) and then run on this build would, on DELETE, recompute the pruned (smaller) token set and could leave the extra unpruned entries orphaned. As the PR notes, FRONTIER has not shipped, so this only touches nightly-snapshot users - acceptable, just flagging it since polygonDeleteRemovesEveryEntryOfTheRecord only exercises the same-version write/delete path.

Minor / nits

• getTokenBytesNoLeaf(null) allocates a fresh BytesRef per cell on the ingest hot path (both the point shortcut and the main walk). Since extractTokens already materializes a List<String>, the win is small, but a reusable scratch BytesRef would trim per-cell garbage on high-precision area shapes - in keeping with the project's GC-pressure mantra.
• withGeoType() slightly duplicates the fact that withType(GEOSPATIAL) already returns a TypeGeoIndexBuilder; it is consistent with the existing withLSMVectorType()/withFullTextType() pattern, so fine, just noting the redundancy.
• The PruneFrame path array bound relies on depth == level for a GeoHash tree; the assert documents rather than enforces it in production. Given the grid is always GeohashPrefixTree this is safe, and the strictly-increasing-levels argument in the comment is the real guarantee.

Tests

Coverage is excellent: the reflection-based identity check against Lucene's buffered pruner is the right way to pin this, and noTokenIsEmittedTwice guarding against duplicate index entries (which the set-equality check would hide) is a sharp catch. I could not execute the suite in this environment, but the changes are compile-clean on inspection and the author reports 0 failures across the affected engine suites.

Overall: solid, careful work. My only real ask is a sanity check on items (1) and (3) before merge.

[lvca]

Both pre-merge checks done, plus the two nits, in 9d02b02.

(1) No-op METADATA on a plain index - swept the repo, nothing breaks. Scanned every text file (not just Java) with a concatenation-aware match, excluding target/ and docs/: 227 CREATE INDEX … METADATA occurrences, all LSM_VECTOR (179), FULL_TEXT (23), GEOSPATIAL (10) or LSM_SPARSE_VECTOR (7).

The one that needed proving is Studio, the only place that builds the clause by concatenation - studio-database.js:1485, if (metadataJson != null) command += " METADATA " + metadataJson;. metadataJson is initialised to null and assigned at exactly one site, inside the LSM_SPARSE_VECTOR branch, so Studio can only ever emit METADATA on a sparse-vector index. Safe.

The only two plain-type occurrences are CreateIndexStatementTestParserTest:30 and :45, and they are parse-only: checkSyntax calls parse() + validate(), and validate() never inspects metadata - the new rejection is in executeDDL. They stay green, which the run confirms.

Incidental find, pre-existing and not from this PR: Studio's LSM_VECTOR branch also never sets metadataJson, so Studio emits CREATE INDEX … LSM_VECTOR with no METADATA - which executeDDL has always rejected with "LSM_VECTOR index requires METADATA". Filing separately.

(2) ClassCastException from a point map with non-numeric x/y. Real hole, fixed - and at the source rather than in the two methods: GeoUtils.parseGeometry cast map.get("x") straight to Number. Now it reports an IllegalArgumentException like every other unparseable geometry, so the "one bad row is skipped" contract holds for every geo.* consumer, not only isWithin/intersectsWith. Widening the catch to RuntimeException would have hidden genuine bugs instead.

(3) Snapshot-upgrade delete symmetry. Correct, and worth stating rather than leaving implied. The release note now carries it explicitly: a database written by a 26.8.1-SNAPSHOT build from between the FRONTIER change and this one holds unpruned area cells that a delete on this build would no longer fully remove; REBUILD INDEX rewrites it. Point-only indexes are unaffected, since pruning never applies to them.

Nit - BytesRef per cell. Taken. All three walks now retarget a single scratch BytesRef (getTokenBytesNoLeaf(reuse) just repoints bytes/offset/length at the cell's own array - no copy), and the token becomes a String immediately so nothing outlives the next call. This is the ingest path #5478 exists to keep cheap, so the per-cell garbage was worth removing.

Nit - withGeoType() redundancy. Agreed it is redundant with withType(GEOSPATIAL); keeping it for symmetry with withLSMVectorType()/withFullTextType(), since CreateIndexStatement reads uniformly across the four branches.

Nit - assert depth == level. Agreed, and that is now what the comment says: the strictly-increasing-levels argument is the real guarantee, the assert documents the GeoHash contract.

464 tests green across the geospatial, geo.*, SQL-method and Cypher-spatial suites.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 53 complexity

Metric	Results
Complexity	53

View in Codacy

🟢 Coverage 91.98% diff coverage · -7.11% coverage variation

Metric	Results
Coverage variation	✅ -7.11% coverage variation
Diff coverage	✅ 91.98% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (49206db)	140950	106753	75.74%
Head commit (9d02b02)	173063 (+32113)	118777 (+12024)	68.63% (-7.11%)

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 (#5604)	162	149	91.98%

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%

_{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 #5600 Geospatial misc

Reviewed all 6 commits end-to-end. This is a high-quality, well-motivated PR: three genuinely independent findings, each fixed at the right layer, with the fixes and their reasoning documented in code, in the release note, and in tests. The self-review commits (fractional precision, no-duplicate-token assertion, malformed-parameter handling, point-map CCE, BytesRef reuse) already caught most of what I would have flagged. Below is what stood out. Nothing here is blocking.

Strengths worth calling out

• The pruning is validated against the reference, not against itself. LSMTreeGeoIndexCellPruningTest invokes Lucenes createCellIteratorToIndex` by reflection and asserts set equality over 8 shapes x 4 precisions, plus a coverage-never-shrinks property and the one-token-per-point guarantee. Exactly the right way to gain confidence in a hand-rolled streaming reimplementation of a buffered algorithm.
• Removing the WKT sniff from BinarySerializer.deserializeValue is the correct call. A prefix match on arbitrary user text driving the deserialized type was a latent correctness hazard well beyond geo (POLYGON shaped, see attached became a Shape), and it ran on every string read in the DB. Moving the conversion to the point of consumption is right, and backward-compat holds because every geometry consumer already accepts WKT.
• withType() recording the type on the original builder removes a real latent trap for the full-text/vector callers too, not just geo. Good to generalize it.
• The PruneFrame bookkeeping (one frame per stack level, reused, pending[] grown in place) keeps the ingest path allocation-conscious, and the point fast-path stays allocation-free.

Minor / discussion points

1. Per-row WKT re-parsing on the geo predicate path (performance, not a regression). With the sniff gone, coords.isWithin(...) and geo.within(coords, ...) now call GeoUtils.parseGeometry(value) for the records WKT string on every row of a scan. This matches what the geo.*functions already did and is strictly better than the old per-read sniff, so a net win - but for large scans over aSTRING`-typed WKT column there is no per-value caching. Nothing to change here; worth keeping in mind if geo predicate throughput ever shows up in a profile.

2. The METADATA-on-unsupported-type rejection is a genuine execution-time breaking change (already flagged in the note). The !isEmpty() guard correctly lets an empty object through, so only a non-empty clause fails. Worth double-checking outside this PR: schema export/import and replication - if any of them can emit CREATE INDEX ... METADATA {...} with non-empty metadata for a plain LSM_TREE/HASH/UNIQUE index (e.g. a round-tripped old export), import would now throw where it previously ignored the clause. The release note covers the user-facing case; just confirming the internal tooling never produces such a statement.

3. TypeGeoIndexBuilder.withMetadata(IndexMetadata) accepts null and stores it, after which geoMetadata() throws IllegalStateException. This is tested and only reachable by explicit API misuse, so it is fine - but leaving the builder in a state where create()/setters fail is a slightly surprising API shape. A no-op-on-null (mirroring the JSONObject overload) would be more consistent. Purely cosmetic.

4. The assert depth == level documentation rewrite (commit 3) is a good clarification - the array bound genuinely rests on levels on the stack being strictly increasing, not on the GeoHash one-char-per-level contract, so the code is safe even if a grid ever descended by more than one level.

Test coverage

Thorough and targeted: round-trip identity (incl. lists/maps and free-text-with-geo-keyword cases), real-Shape-still-a-Shape, legacy method WKT acceptance, malformed-parameter-reported-vs-bad-row-skipped, precision/tokenization from SQL and Java API, all rejection paths, delete symmetry for area shapes, and the Lucene-equivalence + coverage-monotonicity properties for pruning.

Note: I could not execute the suite in this review environment (Maven invocation required an approval not available here), so the above is based on code reading; the PR reports 3182 engine tests green across the affected packages.

Overall: LGTM. The points above are minor/informational - none should block merge.

[lvca]

Thanks. Point 2 was the one with a real question behind it, so I traced it end to end - the answer is no internal path can produce CREATE INDEX <plain type> ... METADATA {...}, because nothing in ArcadeDB round-trips a schema through generated DDL at all:

• Schema export/import is structural JSON, not SQL. JsonlExporterFormat:98 writes LocalSchema.toJSON() verbatim; JsonlImporterFormat.loadSchema rebuilds through createDocumentType/createProperty/getOrCreateTypeIndex and, for vectors, buildTypeIndex(...).withLSMVectorType().withMetadata(...). There is no SQL exporter in the codebase - the formats are jsonl, graphml, graphson.
• Backup/restore is a raw file copy. FullBackupFormat zips configuration.json, schema.json and the component files; restore unzips them.
• HA replicates the schema JSON, not DDL. RaftReplicatedDatabase ships getSchema().getEmbedded().toJSON() in a SCHEMA_ENTRY, and ArcadeStateMachine.applySchemaEntry applies it with schema.update(new JSONObject(...)). The wire format has no SQL field. Snapshot install copies schema.json as a file.
• LocalSchema never touches CreateIndexStatement - it rehydrates indexes from the persisted JSON via setMetadata + addIndexInternal.
• REBUILD INDEX stays on the builder path (buildTypeIndex(...).withMetadata(rebuildMetadata).create()), so a plain index whose JSON happens to carry metadata is unaffected: the rejection lives only in the SQL statement, and the builder APIs still accept metadata for any type.

The only string-built CREATE INDEX statements in main sources are TextEmbeddingsImporterLSM (LSM_VECTOR, with metadata - handled by its own branch) and RemoteSchema.createTypeIndex (never emits METADATA). So the only reachable break is a hand-written user statement, which is what the release note covers.

Point 1 (per-row WKT re-parsing) - agreed on all counts, including that it is a net win over the per-read sniff it replaces. Noted for the day it shows up in a profile; no change here.

Point 3 (withMetadata(IndexMetadata) accepting null) - I'd rather leave it. IndexBuilder.withMetadata assigns whatever it is given, and TypeFullTextIndexBuilder does the same, so a no-op-on-null would make the geospatial builder the only one of the four that quietly ignores an explicit null. It is also not geospatial-specific: create() with null metadata dereferences metadata.typeName in the base class regardless of subtype. Same reasoning as the overload nit earlier - if this shape is worth changing, it is worth changing for the family, which is #5606's territory.

Point 4 - thanks, that was exactly the intent of the rewrite.

[lvca]

Closing the loop on the verification asked for in the earlier review: mvn -o -pl engine verify -DexcludedGroups=benchmark on the engine module at 9d02b02 is green.

[INFO] Tests run: 10432, Failures: 0, Errors: 0, Skipped: 23
[INFO] BUILD SUCCESS

1266 test classes, exit 0.

One note on method, since it matters for reading any earlier number I posted: an intermediate run of this same command reported 41 failures, and those were not real. 34 were NoClassDefFoundError for classes this PR does not touch (NodeIndexRangeScan, NodeByLabelDisjunctionScan, CountEdgesReturnStep$GroupKey, LongLongHashMap), caused by my own concurrent mvn compile/mvn test invocations rewriting target/classes underneath the running surefire JVM; the other 2 were EdgeAppendMergeRaceTest, the known-flaky race test from #5570. The run above was executed with nothing else touching the worktree, which is the only way this module's suite gives a trustworthy answer.

CI's own unit-tests, slow-unit-tests and integration-tests jobs have also passed on this branch.

The one red check, Meterian client scan, fails identically on main and this PR adds no dependencies, so it is not attributable here.