fix(#10751, #10734, #10788): a type rename re-keys its indexes, DROP INDEX * is one statement, function options refuse a value of the wrong kind - #10816

Merge Risk: Moderate · previous review 7d085

Some unquoted map keys still fail to parse, and a type rename can make an existing index inaccessible by name. Fix the rename collision before merging.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Out of Scope Changes check		The PR includes behavior outside the directly linked objectives. FunctionOptions.getBoolean() changes validation for every SQL function option bag, including vector, geo, full-text, PromQL, movingAv…	Move the shared boolean validation and shortestPath maxDepth changes to a separately linked issue or pull request, or add an active linked issue that explicitly covers these compatibility changes. Keep only changes required to implement #…
Docstring Coverage		Docstring coverage is 40.98% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title names three fixes: a type rename re-keys its indexes (#10751), DROP INDEX * runs as one statement (#10734), and function options refuse a value of the wrong kind (#10788). The raw summary an…
Description check		The description follows the template sections: What does this PR do (covered by per-issue sections), Motivation (covered by the linked issue objectives), Related issues (closes #10751, #10734, #10788;…
Linked Issues check		The changes meet the coding requirements for #10751, #10734, and #10788. TypeIndex uses one rename path that re-keys the schema map and restores the old name on failure. Manual names persist through…

Full details: Title check

Explanation

The title names three fixes: a type rename re-keys its indexes (#10751), DROP INDEX * runs as one statement (#10734), and function options refuse a value of the wrong kind (#10788). The raw summary and objectives confirm each of these changes is present in the changeset (TypeIndex rename and rekeying, DropIndexStatement dropIndexes bulk call, FunctionOptions getBoolean and non-negative getters). The title covers the main changes, though it is long at 144 characters and omits the SQL map keyword fix and the rebuild fix.

Full details: Description check

Explanation

The description follows the template sections: What does this PR do (covered by per-issue sections), Motivation (covered by the linked issue objectives), Related issues (closes #10751, #10734, #10788; notes #10807 is deferred), Additional Notes (compatibility note), and Verification. It is detailed and explains the rename, DROP INDEX, rebuild, and function option changes. The template's Checklist section is missing: it does not state that mvn clean package was run or confirm that unit tests cover failure and success scenarios. The description does state tests fail on main and that engine sweeps are green, which partly covers the checklist intent, but it does not tick or address the checklist items explicitly. The description also does not give a separate Motivation heading, which is a minor deviation.

Resolution

Optionally add the template's Checklist section with the two items explicitly addressed (mvn clean package run; tests cover failure and success), and add a Motivation heading. The description is otherwise mostly complete.

Full details: Linked Issues check

Explanation

The changes meet the coding requirements for #10751, #10734, and #10788. TypeIndex uses one rename path that re-keys the schema map and restores the old name on failure. Manual names persist through renames and serialization. The added rename census and rollback tests cover registered index kinds and reopen behavior. DROP INDEX * collects top-level names and calls dropIndexes(...) once. The DDL census compares schema and index state before and after refusal, including reopen. FunctionOptions rejects fractional, overflowing, unparsable, negative, and wrong-kind values with the option name in the error. Astar and dijkstra use the validated depth reader.

Full details: Out of Scope Changes check

Explanation

The PR includes behavior outside the directly linked objectives. FunctionOptions.getBoolean() changes validation for every SQL function option bag, including vector, geo, full-text, PromQL, movingAvg, and bellmanFord consumers. SQLFunctionShortestPath also changes maxDepth semantics. #10788 is scoped to astar/dijkstra MAP options. The generic keyword-map parser supports the graph option syntax, but the shared boolean compatibility change and shortestPath change are not required by the linked issue.

Resolution

Move the shared boolean validation and shortestPath maxDepth changes to a separately linked issue or pull request, or add an active linked issue that explicitly covers these compatibility changes. Keep only changes required to implement #10751, #10734, and astar/dijkstra MAP-option validation in this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 40.98% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 61 functions across 16 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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

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

Overall a well-scoped, well-tested change. No security hazards found, so no label added. The three fixes (index re-key on rename, one-shot DROP INDEX *, strict option bag) look correct to me. Notes below.

Behavior change wider than the PR title says

• FunctionOptions.getBoolean() now throws for anything but a Boolean or "true"/"false". FunctionOptions is also used by the vector, geo, fulltext (MoreLikeThisConfig, searchIndexMore), PromQL, movingAvg and BellmanFord functions, not only astar/dijkstra/shortestPath. Queries that passed 1/0 or 'yes' to those would have run before and now fail. Please confirm that is intended, state it in the PR description or release notes, and check there is a test in at least one non-graph consumer.
• Should 1/0 Integer values be accepted as booleans? The tests assert emptyIfMaxDepth: 1 is refused, which is a defensible choice, but it is a compatibility break.

TypeIndex rename

• rename() writes logicName and calls indexRenamed(). logicName is not volatile and is read lock-free by queries. This is a pre-existing pattern, but indexMap re-keying and the field write are not atomic with respect to readers. A short note on the locking expected (schema write lock held by the caller?) would help.
• typeIndexFor treats a custom name equal to the derived name as not manual. A user who explicitly runs CREATE INDEX Foo[a] ON Foo (a) will see the name change on a type rename. That is an edge case and probably fine, but worth a comment.
• rename() dereferences type.getSchema().getEmbedded(). Confirm that is never null or a different schema impl during schema load (updateTypeName during load).

**DROP INDEX ***

• The result rows are built from the full getIndexes() snapshot, which includes bucket sub-indexes that were dropped with their parent, so rows are reported for them too (same as before, now with indexName). Fine, but bucketName duplicating indexName is a little confusing. A deprecation note would help.
• The snapshot is taken before dropIndexes, and a concurrent index creation between the two is simply not dropped. Acceptable.

Grammar

• Adding ~70 keywords to mapKeyword plus INDEX_COLON is nicely guarded by the lexer census test. Verify there is no ambiguity or perf regression for large map literals and {...} JSON in INSERT ... CONTENT (parser prediction cost). A quick run of the existing JSON/INSERT CONTENT tests would confirm.
• Minor: the mapKeyword branch keeps the spelled case while MAXDEPTH is normalized. That is documented, fine.

Style (CLAUDE.md)

• Code and tests follow the conventions (final, assertj, no System.out). Nice.
• The census tests are slow-ish by nature. If Issue10734...CensusTest or MapKeyKeywordsTest run for several seconds, consider @Tag("slow").

LGTM pending the getBoolean compatibility confirmation.

🤖 Generated with Claude Code

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.

Review

I read the main-source changes and skimmed the tests. I did not build or run anything. I found no security concerns: there are no new dependencies, no new I/O, and the grammar change only widens which words may be map keys.

What looks good

• TypeIndex.rename() is now the only place the name is assigned, and it re-keys the schema map in the same step. That removes the class of bug from A type rename renames every TypeIndex but never re-keys the schema's index map, so the index answers to a name nothing resolves: DROP and REBUILD INDEX fail, CREATE INDEX on a new type of the old name is refused, and a manual index name is lost #10751.
• Keeping manualName as a flag on the TypeIndex, instead of comparing against the derived name in toJSON, fixes the manual name being lost after a reopen.
• DROP INDEX * now makes one dropIndexes() call, and its result rows keep bucketName for compatibility.
• Both rebuild paths (RebuildIndexStatement and DatabaseChecker) now re-create the sub-index through the declaring type.
• The tests include a census over every registered index kind, and MapKeyKeywordsTest checks the lexer vocabulary against the map-key rules.

Points to consider

1. Behaviour change in FunctionOptions.getBoolean. It now throws for anything that is not a Boolean or the strings true/false. A numeric 1/0 used to be read as false by Boolean.parseBoolean, and now raises an error. That is probably what you want for astar/dijkstra option bag truncates numeric options and silently defaults an unparsable one (integerOrDefault/longOrDefault) #10788, but existing queries that pass such values will start failing. A line in the changelog or release notes would help. Also confirm the old non-string, non-Boolean fallback was not relied on anywhere else, such as the other FunctionOptions consumers.
2. maxDepth handling is inconsistent across the path functions. SQLFunctionShortestPath now treats a null maxDepth as no bound (opts.get(...) != null). getNonNegativeInt in bindOptions goes through getInt, which I did not trace for an out-of-int-range Long. It is worth a test for maxDepth: 3000000000.
3. mapKeyword is a long hand-maintained list. MapKeyKeywordsTest guards it, which is good. The grammar comment says a keyword missing from both rules fails that test, so keep that guarantee as the lexer grows. Check the key case too: visitMapEntry uses ctx.mapKeyword().getText() as spelled, so {TIMEOUT: 1} yields the key TIMEOUT, while MAXDEPTH is normalised to maxDepth. Option lookups are case-sensitive, so this may surprise people. Consider documenting it or adding a test.
4. DropIndexStatement all branch. The result rows list every index, including bucket sub-indexes, while only the top-level ones are passed to dropIndexes(). That is consistent with the old output, but the indexName and bucketName values for sub-index rows are bucket-level names. Make sure that is intentional, and consider a short comment saying so.
5. TypeIndex.logicName is now volatile. That is fine. The rename() sequence (set the field, then call indexRenamed) is not atomic for concurrent readers, so a lookup by the new name in that window could miss. The comment says it runs under the schema change of the type rename, so please confirm that the schema lock is held for the whole duration.
6. Minor style. The field alignment in the TypeIndex header is uneven, because private final boolean manualName; is misaligned against the neighbouring lines. The new Javadoc comments are long. Otherwise this matches the surrounding style.

Overall this looks solid. The main thing I'd like addressed is #1 (documenting the compatibility change), and #3 and #5 are worth quick confirmation.

Review

Overall this is a well-scoped, well-documented PR with strong regression coverage (a census over IndexFactory types, a census over the lexer vocabulary, refused-rename rollback). No security hazards found: no new dependencies, no new I/O or reflection, and the option parsing gets stricter, not looser.

Points worth a look

1. TypeIndex.rename() ordering (TypeIndex.java): logicName is assigned before indexRenamed(oldName, this) runs. If indexRenamed throws, the index keeps the new name while the map keeps the old key, which is the inconsistency A type rename renames every TypeIndex but never re-keys the schema's index map, so the index answers to a name nothing resolves: DROP and REBUILD INDEX fail, CREATE INDEX on a new type of the old name is refused, and a manual index name is lost #10751 is fixing. Either assign after the call succeeds (if indexRenamed doesn't read getName()), or restore logicName in a catch block.
2. Re-keying while iterating: updateTypeName now mutates indexMap through indexRenamed. Please confirm that the caller in LocalDocumentType.renameInternal (and the rollback path) doesn't iterate a live view of indexMap. Iterating a copy or a snapshot array is fine.
3. FunctionOptions.getBoolean is now strict: a numeric 1/0 or any non-true/false string throws. That is the intent, but it is a behavior change for every function using FunctionOptions, not just the path finders (your test covers search_index_more). A line in the release notes or docs would help.
4. mapKeyword is a hand-maintained list in the grammar. MapKeyKeywordsTest guards it, which is good. A comment next to the rule saying that the test is what keeps it complete would help whoever adds the next lexer token.
5. IndexFactory.getRegisteredTypes() is production API added only for a test census. It's harmless, just noting it.
6. DropIndexStatement result rows: the loop over indexes emits a row for every index, including bucket sub-indexes, while only the top-level ones are passed to dropIndexes. Please confirm that is intended. Previously the rows came from the same getIndexes() list, so it is probably consistent.
7. Style: in the DatabaseChecker and RebuildIndexStatement changes, the declaringTypeName computation is duplicated. A small shared helper would avoid the two copies drifting.

Nothing blocking from my side beyond points 1 and 2. I did not run the tests locally.

Review

I read the main-code changes in full. I skimmed the new tests and did not run them. This is a maintainer PR and I found no security hazards: no new dependencies, no unsafe parsing, no new external input paths.

What looks good

• TypeIndex.rename() is now the only place the name is assigned, and it re-keys indexMap in the same step, with rollback on failure. That is a cleaner design than capturing names in renameInternal.
• manualName is stored as a flag and toJSON persists it from that flag. This fixes the loss of a manual name after a reopen. logicName is now volatile, which suits the lock-free readers.
• DROP INDEX * goes through one dropIndexes() call and filters out bucket sub-indexes, so the all-or-nothing behaviour holds. Adding indexName while keeping bucketName is a sensible compatibility choice.
• The census over IndexFactory.getRegisteredTypes() is a good regression guard. Re-creating the sub-index through the declaring type in both RebuildIndexStatement and DatabaseChecker fixes a real bug.
• mapKeyword plus the MapKeyKeywordsTest vocabulary walk means a new lexer keyword cannot silently break map keys.

Points to consider

1. Behaviour change in getBoolean. Option values like 'yes' or 1 used to read as false and now throw CommandSQLParsingException. That is the right fix, but it is a breaking change for queries that worked by accident. A line in the release notes would help. Also check whether a numeric 0 or 1 should be accepted, since v instanceof String is the only non-Boolean branch.
2. DropIndexStatement result rows. The result loop still iterates every entry in indexes, including bucket sub-indexes that were not in toDrop. If that matches the old output, fine. If a client counts rows, the number of rows may not equal the number of dropped top-level indexes. A short comment or a test assertion would pin it down.
3. TypeIndex.rename() partial failure. If indexRenamed throws after it has partly re-keyed the map, restoring logicName may leave the map inconsistent. This probably cannot happen, since indexRenamed only does a map remove and put. Confirm that, or note it in a comment.
4. typeIndexFor. A user who explicitly writes CREATE INDEX \Foo[a]` ON Foo (a)` gets a derived (non-manual) name that follows a rename. The comment explains this and it seems reasonable, but it is a subtle edge case. One line in the docs would help.
5. Removed helpers. booleanOrDefault, integerOrDefault and the related protected methods in SQLFunctionHeuristicPathFinderAbstract are deleted. Check that nothing outside the module subclasses them (for example, user-supplied functions).

I have no blocking concerns.

Review of #10816 (index re-keying on type rename, single-statement DROP INDEX *, map-literal keyword keys, option-bag validation).

Overall: the changes hang together. I found no security hazards. Nothing here widens access or adds a dependency, and the new parsing paths take only the lexer's own tokens. I only read the diff and did not build or run the tests.

What looks good

• TypeIndex.rename() is now the only place the logic name is assigned. It re-keys indexMap in the same step and restores the old name if indexRenamed throws.
• manualName is stored as a flag instead of being inferred by comparing against the derived name. That fixes the manual name being lost after a reopen.
• LocalSchema.indexRenamed and renameInternal both reject a name collision, and renameInternal does it before anything changes.
• DROP INDEX * goes through one dropIndexes() call. That removes the partial-drop-then-"Nothing was changed" case.
• MapKeyKeywordsTest walks the lexer vocabulary, so a keyword added later fails the test instead of silently being refused as a map key.
• The tests cover the main paths, including the IndexFactory census.

Points to consider

1. Behavior change in shared FunctionOptions.getBoolean. It used to read anything that wasn't true as false. It now throws for 1, 'yes' and similar, for every function that uses it, not just the path finders. That is arguably right, but it can break existing user queries. Please call it out in the release notes. Also check that Boolean values and null are still handled before the new throw. The diff shows the branches only partly.
2. shortestPath maxDepth. A null value now means "no bound" (opts.get(...) != null), where before it was containsKey. That matches astar and dijkstra, but a test for shortestPath(..., {maxDepth: null}) and for a negative value would pin it down. I didn't see one.
3. DropIndexStatement result rows. Rows are still emitted for every index in getIndexes(), including bucket sub-indexes that were not named in the drop. Sub-indexes dropped together with their type index will therefore appear as rows too. Confirm that is intended, or emit rows only for toDrop.
4. visitMapEntry workaround. For tokens like bucket:1 it re-parses the substring with new SQLAntlrParser(null).parseExpression(...). It works, but it is a second parser instance on a per-entry path. Maps are small, so I expect no perf concern. A lexer-level fix would be cleaner if one is feasible.
5. Dead helpers. The removed booleanOrDefault, integerOrDefault and longOrDefault helpers were protected. Check that no subclass outside the module relied on them.
6. Regression coverage for the TypeIndex/schema change. Please confirm the existing HA/replication schema tests still pass, since indexRenamed now has a new failure mode (AlreadyExistsException) during a type rename.

Nice work overall.

Review

Overall this is a well-scoped, well-tested change. I found no security hazards, so I did not add the label.

What looks good

• TypeIndex.rename() is now the only place the name is assigned, and it re-keys indexMap in the same step, with rollback on failure. That removes the class of bug in A type rename renames every TypeIndex but never re-keys the schema's index map, so the index answers to a name nothing resolves: DROP and REBUILD INDEX fail, CREATE INDEX on a new type of the old name is refused, and a manual index name is lost #10751 rather than patching one call site.
• Storing manualName as a flag instead of comparing against the derived name fixes the loss of manual names after reopen.
• The pre-check in LocalDocumentType.renameInternal for name collisions happens before any mutation, and LocalSchema.indexRenamed also refuses to overwrite another index.
• The census tests (every registered index kind, every lexer keyword as a map key) are a good regression net.

Points to consider

1. Behavior change in FunctionOptions.getBoolean. It now throws for anything but a Boolean or a true/false string. Numbers like 1 were previously read as false silently and are now refused. That is the intent, but it applies to every function option bag (e.g. search_index_more), so it is worth a release-note line.
2. Parser allocation in SQLASTBuilder. The bucket:1 / schema:x path creates new SQLAntlrParser(null) per map entry. It is a rare path, but a shared helper or a comment saying it is rare would settle any performance question.
3. mapKeyword list maintenance. It is a hand-maintained list of tokens. The census test covers new keywords, which is good. Consider noting in the lexer file that new keywords must be added to mapKeyword, so people hit the hint before the test failure.
4. DROP INDEX * result rows. Rows are emitted for every index, including bucket sub-indexes, after dropping only the top-level ones. Please confirm that is intended (it looks consistent with the old output). Also confirm dropIndexes is atomic with respect to HA/replicated schema changes.
5. Upgrade path. For existing databases whose JSON has a custom name equal to the derived name, manualName becomes false, so a rename now moves it. That matches the comment in LocalDocumentType and seems right, but a one-line mention in the PR description would help.

Style conforms to CLAUDE.md (final usage, imports, assertj style). Please make sure the CI engine test lane is green, particularly the schema and SQL parser suites, since the grammar change touches a lot.

Codecov Report

❌ Patch coverage is 88.70968% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.00%. Comparing base (f8ae3a8) to head (02d2f09).
⚠️ Report is 12 commits behind head on main.

@@             Coverage Diff              @@
##               main   #10816      +/-   ##
============================================
+ Coverage     78.99%   79.00%   +0.01%     
- Complexity     4768     4770       +2     
============================================
  Files          2299     2299              
  Lines        251324   251442     +118     
  Branches      54835    54867      +32     
============================================
+ Hits         198529   198655     +126     
+ Misses        32567    32540      -27     
- Partials      20228    20247      +19     

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