fix(engine) #5637: persist the partition strategy, and report the index change that undoes it

[lvca]

Closes #5637.

Two loose ends of the partitioned-strategy series (#5589, #5595, #5603), both about what happens to a type after it has been configured.

1. ALTER TYPE ... BucketSelectionStrategy is persisted

The strategy was set in memory and nothing wrote it out, so a type partitioned by that DDL alone came back round-robin after a restart - unless some later, unrelated schema mutation happened to flush the configuration first. This hits correctly configured types, which is what makes it worse than the states #5603 refuses: new records were placed round-robin among rows the partition hash had placed, every partition-aware lookup silently fanned out, and nothing warned.

It is the one schema mutator that left the write to somebody else - every sibling either calls saveConfiguration() directly or goes through recordFileChanges, which does - and the flag about the partitioning (needsRepartition, in the same class) saved itself while the partitioning did not. It now saves, skipped while the schema is being read back so a partitioned database does not rewrite an identical schema.json on every open.

2. An index created on an already-partitioned type is diagnosed when it is created

#5603 refuses an unsuitable partition at assignment time, but the same state was reachable by reordering the DDL:

ALTER TYPE T BucketSelectionStrategy `partitioned('name')`;   -- accepted
DROP INDEX `T[name]`;
CREATE INDEX ON T (name COLLATE CI) UNIQUE;                   -- used to be silent

Correctness always held - the strategy declines to prune, so lookups fan out and UNIQUE stays global - but between the CREATE INDEX and the next restart nothing said the partitioning had stopped doing anything. TypeIndexBuilder.create() now re-runs the same check, once per CREATE INDEX rather than once per bucket (which is why the hook is not in addIndexInternal, and why it does not fire during schema reload).

It never refuses: at assignment time the strategy is what was asked for and a blocked one is pure cost, whereas here the index is what was asked for and it is useful, so the partitioning is what gives way. An index change that leaves the partition exactly as suitable as it was stays quiet.

3. Persisting the strategy made a pre-existing fault reachable

Binding the strategy demanded the unique automatic index on the partition properties, and did so on every rebind - including the one the schema loader performs on every open. With the strategy now reaching schema.json, a DROP INDEX on the partition key made the next open throw from inside the loader, which aborts everything it has not reached yet: the remaining types' strategies, the triggers, the function libraries, the extensions, and the compaction file-migration map WAL recovery redirects through - surfacing only as Error on loading schema. The schema will be reset.

Binding no longer validates anything. The requirement moved into checkSuitability(), which still refuses it at assignment time and now warns about it on load, so the type keeps its partitioned placement and records written after the index was dropped still land where a lookup would look for them.

The refusal is not byte-for-byte what it was, and anything matching on the old form should know it: it used to be an IllegalArgumentException thrown straight out of setType, and it is now a SchemaException raised by the suitability check and wrapped in the usual "Cannot use the partitioned bucket selection strategy on ... for type ..." sentence. What is unchanged is what callers actually depend on - the cannot find a unique automatic index on the partition properties [...] substring, and the CommandSQLParsingException -> HTTP 400 classification, since AlterTypeStatement already caught both exception types. LocalSchema additionally isolates a per-type strategy failure, so any other unusable strategy - an implementation class that no longer resolves, say - costs that one type its pruning instead of the rest of the schema.

Suitability.canPrune() is renamed isUsable(): a missing partition index does not stop the strategy computing a bucket, it removes the index lookup that would have been pruned, so the old name no longer described the whole set.

Verification

New PartitionedStrategyLifecycleTest (9 tests). Four were red before the fix (persistence to schema.json, survival of a reopen, and both index-change reports), and the two load-path guards were mutation-checked individually - reinstating the binding validation reddens aPartitionedTypeWhoseIndexWasDroppedStillOpens, removing the loader's per-type catch reddens anUnresolvableStrategyDoesNotCostTheRestOfTheSchema. Three controls keep them honest: an unchanged index re-creation stays quiet, a round-robin type is never diagnosed, and reverting to round-robin persists as the absence of the entry.

The explicit saveConfiguration() workaround in PartitionedStrategySuitabilityTest.theFanOutWarningIsNotRepeatedOnEveryReopen, added in #5605 with a comment saying why, comes out.

Full arcadedb-engine suite green (10545 tests, 0 failures), plus PartitionedStrategyRefusalHttpTest in arcadedb-server.

🤖 Generated with Claude Code

https://claude.ai/code/session_017dDUKvQXJGBBoXiEQgtc2v

[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 7 complexity

Metric	Results
Complexity	7

View in Codacy

🟢 Coverage 94.74% diff coverage · -6.21% coverage variation

Metric	Results
Coverage variation	✅ -6.21% coverage variation
Diff coverage	✅ 94.74% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (7081057)	149675	112546	75.19%
Head commit (2550afe)	174679 (+25004)	120508 (+7962)	68.99% (-6.21%)

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 (#5644)	38	36	94.74%

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: #5644 - persist the partition strategy, and report the index change that undoes it

Thorough, well-reasoned change. I traced all five source hunks against PartitionedBucketSelectionStrategy, LocalDocumentType, LocalSchema.readConfiguration, and TypeIndexBuilder.create, and the design holds up. Notes below, from most to least important - none are blockers.

Correctness (verified, looks right)

• Moving the unique-index requirement out of setType() into checkSuitability() is the right call. getBucketIdByKeys()/getBucketIdByRecord() compute the bucket purely from the property hash and never touch the partition index, so a dropped index leaves placement correct and only removes the pruned lookup - the rename canPrune() -> isUsable() describes that accurately. The reload path binds without throwing, so a partitioned DB whose index was dropped now opens (confirmed by aPartitionedTypeWhoseIndexWasDroppedStillOpens).
• The PartitionReport three-way split (ASSIGNMENT throws / RELOAD warns-only / SCHEMA_CHANGE reports-all-refuses-nothing) is clean, and the single checkSuitability() source keeps the three reactions from drifting.
• Placing reportPartitionSuitabilityAfterSchemaChange() outside the try/catch in TypeIndexBuilder.create() is correct - a diagnostic must not be able to trip the index-cleanup arm - and hooking it there rather than in addIndexInternal avoids the per-bucket multiplication and the reload firing.
• The LocalSchema per-type catch correctly isolates one bad strategy from the rest of the load; since the block sits near the end of readConfiguration, this genuinely protects triggers/functions/extensions.

Minor: redundant re-reporting on unrelated CREATE INDEX

reportPartitionSuitabilityAfterSchemaChange() re-runs the full checkSuitability() on every CREATE INDEX against a partitioned type, so pre-existing warnings get re-logged even when the new index is unrelated. Example: a type partitioned on k that already has a fan-out index on code will re-emit the code advisory every time any further index is created. Correct, just noisier than "report what this DDL changed." Worth a comment acknowledging it, since the surrounding Javadoc emphasises "one line at the moment the shape is chosen."

Minor: saveConfiguration() is now unconditional

The new if (!schema.isReadingFromFile()) schema.saveConfiguration(); fires on every setBucketSelectionStrategy call, including:

• setting the same strategy a type already has (idempotent DDL now always rewrites schema.json), and
• the inherited-strategy copy at addSuperType (line ~1840), which already runs inside recordFileChanges and thus saves again - a double write of schema.json during super-type wiring.

Both are cold-path DDL, so no real cost, but you could skip the save when selectionStrategy is equivalent to previous. Optional.

Tests

Coverage is strong - 9 tests including the three honest controls (unchanged re-create stays quiet, round-robin never diagnosed, revert-to-default persists as absence), and the two load-path guards being individually mutation-checked is exactly the right rigor. One small thing: captureWarnings() swaps the process-global LogManager.instance().setLogger(), which assumes single-threaded execution within the JVM; fine for the current sequential engine suite, but a landmine if these ever run under parallel test execution. The try/finally restore is correct.

Nits

• The comment block around TypeIndexBuilder.java:422 uses an HTML <p> tag inside a // line comment (copied from Javadoc style) - harmless but out of place in a non-Javadoc comment.

Nice cleanup removing the saveConfiguration() workaround in PartitionedStrategySuitabilityTest now that the root cause is fixed. LGTM once the above are considered.

Note: I could not run mvn in this sandbox, so I relied on static analysis plus the PR's stated green run (arcadedb-engine 10545 tests + PartitionedStrategyRefusalHttpTest).

[claude]

Review of PR #5644 — persist the partition strategy, and report the index change that undoes it

Overall this is a high-quality, carefully reasoned fix. The three faults (strategy not persisted, later CREATE INDEX silently undoing suitability, and the reachable loader crash) are well diagnosed, the reaction matrix (ASSIGNMENT / RELOAD / SCHEMA_CHANGE) is a clean model, and the fault isolation added to the loader is a genuine robustness improvement. The tests are strong: four were red before the fix, the two load-path guards are mutation-checked individually, and there are honest controls (unchanged index re-creation stays quiet, round-robin never diagnosed). Comment density matches the surrounding code and explains the why, not just the what.

I verified the key claims against the source rather than the description:

• The renamed Suitability.canPrune() -> isUsable() has no stale references left in engine/src.
• Index restore on load goes through componentFactory/file registration, not TypeIndexBuilder.create(), so the new reportPartitionSuitabilityAfterSchemaChange() hook genuinely does not fire during reload — the "once per CREATE INDEX, not once per bucket, not on open" claim holds.
• The unresolvable-class path throws in the String overload before this.bucketSelectionStrategy is reassigned, so the loader's new catch correctly leaves the type on round-robin — matching anUnresolvableStrategyDoesNotCostTheRestOfTheSchema.
• Moving the throw out of setType() into checkSuitability()/reportPartitionSuitability(ASSIGNMENT) preserves the refusal at assignment time while letting rebinds (add-bucket, add-index, loader) no longer throw. Correct.

A few minor, non-blocking observations:

1. Redundant saveConfiguration() in one call. In the populated partition-shape-change branch, setNeedsRepartition(true) already calls schema.saveConfiguration() (LocalDocumentType.java:701-702), and then the new unconditional saveConfiguration() at line 800-801 writes schema.json a second time within the same setBucketSelectionStrategy invocation. Not a correctness issue, just a redundant full-file rewrite on that path. The explicit save could be skipped when the flag flip already fired — this is a different (same-call) redundancy, not the operator-repair case the comment defends.

2. The explicit save now also fires on the inheritance path. addSuperType inherits the super type's strategy via setBucketSelectionStrategy(...) (LocalDocumentType.java:1857), which now triggers saveConfiguration() inside an already-saving recordFileChanges block — another redundant write. Cosmetic given the frequency, but worth being aware of.

3. Coverage gap: inheriting a partitioned strategy. Removing the throw from setType() means a subtype that inherits a partitioned strategy but lacks the required unique index now routes through reportPartitionSuitability(ASSIGNMENT) (throwing SchemaException) instead of the old IllegalArgumentException from setType. The behavior is arguably improved, but no partitioning test exercises addSuperType with a partitioned parent. A small regression test (inherit with, and without, a suitable index on the subtype) would lock this down.

4. Transaction/rollback of saveConfiguration(). It writes schema.json eagerly; if the enclosing DDL transaction later rolls back, disk carries the new strategy while in-memory state is reverted. This matches every sibling mutator (setAliases, LocalProperty setters), so it is not a regression introduced here — noting only for completeness.

Nice touches: extracting WarningCapture out of the duplicated test helper and documenting that the logger swap is process-global (assumes the engine suite's sequential execution); the SCHEMA_CHANGE "whole current picture, not the delta" rationale; and keeping reportPartitionSuitabilityAfterSchemaChange() outside the try/catch in TypeIndexBuilder.create() so a diagnostic can never fall into the index-cleanup arm.

No blocking concerns. The minor points above are optional polish.

[claude]

Review: persist the partition strategy, and report the index change that undoes it

Thorough, well-reasoned change. I traced the four moving parts (setType no longer validating, checkSuitability() absorbing the index requirement, the saveConfiguration() on assignment, and the per-type try/catch in the loader) and they fit together correctly. Notes below, mostly minor.

Strengths

• The root fix is the right one and minimal. ALTER TYPE ... BucketSelectionStrategy was the only schema mutator not persisting its own change; adding the explicit saveConfiguration() (skipped under isReadingFromFile()) matches how the siblings behave.
• Moving the unique-index requirement out of setType() into checkSuitability() is a genuine correctness fix, not just a refactor. Binding runs on every rebind including the loader's, so a throw there aborted the rest of readConfiguration() (triggers, function libraries, extensions, the compaction migration map). The query path was already defensive about a missing index (partitionKeyIsCaseInsensitive() and getBucketIdByKeys() both tolerate a null TypeIndex), so making binding permissive does not open a new NPE surface. aPartitionedTypeWhoseIndexWasDroppedStillOpens pins exactly this.
• ASSIGNMENT/RELOAD/SCHEMA_CHANGE cleanly separates diagnosis from reaction with a single source of truth in checkSuitability(), and putting the SCHEMA_CHANGE hook in TypeIndexBuilder.create() (once per CREATE INDEX) rather than addIndexInternal (once per bucket, and fires on reload) is correct and well justified in the comment.
• Test coverage is strong. The controls (anIndexChangeThatChangesNothingStaysQuiet, aRoundRobinTypeIsNeverDiagnosedOnIndexCreation, revertingToRoundRobinIsPersistedAsWell asserting absence) keep the positive assertions honest, and anUnresolvableStrategyDoesNotCostTheRestOfTheSchema pins the loader isolation via an extension read strictly after the strategy block. Good removal of the saveConfiguration() workaround in PartitionedStrategySuitabilityTest, and extracting WarningCapture to a shared class is a clean deduplication.
• The .codacy.yml src/test/* -> src/test/** fix is a nice incidental correctness catch.

Minor points / questions

• Redundant saveConfiguration() on flows already wrapped in recordFileChanges. The new unconditional save is exactly right for the direct ALTER TYPE path (the whole point of the fix). But setBucketSelectionStrategy is also called from the supertype-inheritance path (LocalDocumentType.java:1857), which runs inside a recordFileChanges(...) block that itself calls saveConfiguration() at the end. That path now writes schema.json twice per addSuperType. Harmless and off the hot path, but worth a mention.

• catch (final Exception e) in the loader is deliberately broad. It gives the fault isolation the issue needs, but it will also swallow a genuine programming error (e.g. an NPE from unrelated future changes in the bind path) into a WARNING and a silent round-robin fallback. Acceptable given the stated intent, but a narrower catch (or logging non-SchemaException causes at SEVERE) would make a real regression here easier to spot.

• Blocker warnings now repeat on every open, forever, for a dropped-index database. Intentional per the RELOAD doc ("blockers do repeat... until somebody acts on it") and correct, but combined with persistence it means such a database logs a WARNING every startup with no throttle, unlike the needsRepartition advisory next door which does throttle. Just confirming that is the intended trade.

Nothing blocking. The reasoning in the inline comments is unusually complete and made this easy to verify. Nice work.

[claude]

Review: PR #5644 - persist partition strategy & report index changes

Solid, well-scoped fix. I traced each of the three sub-changes through the schema mutation, reload, and index-creation paths. Overall this is high quality: the diagnosis is centralized in checkSuitability(), the assignment/reload/schema-change reactions are cleanly separated by the PartitionReport enum, and the test suite is genuinely rigorous (red-before controls, negative cases like anIndexChangeThatChangesNothingStaysQuiet, and the mutation-checked load-path guards). The comments carry the intent well.

A few observations, mostly minor.

1. Diagnostic outside the try can fail a successfully-created index (minor robustness). TypeIndexBuilder.create() (line ~429) calls type.reportPartitionSuitabilityAfterSchemaChange() after the index has been built and committed via recordFileChanges, and deliberately outside the try/catch. The reasoning (a diagnostic must not fall into the cleanup arm and undo a good index) is sound. But the flip side: if that diagnostic ever throws an unexpected RuntimeException (e.g. an NPE in checkSuitability() or the logging path), the CREATE INDEX command fails even though the index was already persisted - leaving the user with an error plus an index that now exists, so a retry fails with 'already exists'. In SCHEMA_CHANGE mode it cannot throw SchemaException, so the practical risk is low, but since the whole reason it sits outside the try is that it must never affect the built index, consider making that explicit by wrapping the call in a try/catch that logs at WARNING. A diagnostic should never be able to fail an operation that already succeeded.

2. Broad catch(Exception) in the loader (intentional, worth a note). LocalSchema.readConfiguration() (~line 1891) now isolates a per-type strategy failure with catch (final Exception e). Right call for fault isolation, and the comment explains it well. Two small things: (a) it also swallows genuine programming errors (a stray NPE, etc.) and silently downgrades the type to round-robin - acceptable trade for 'the database still opens', but the WARNING is the only signal, so ensure that message is not filtered out at the deploy default log level; (b) e.getMessage() can be null (some NPEs), rendering as '...: null'. Not worth special-casing, just flagging.

3. Unconditional saveConfiguration() now also persists non-partitioned strategies. The new if (!schema.isReadingFromFile() && !alreadySaved) schema.saveConfiguration() (LocalDocumentType.java ~811) fires for any strategy assignment, including thread. That is actually a latent improvement (previously thread also relied on a later mutation to flush), and toJSON() correctly omits round-robin so revert-to-default persists as absence. Confirmed consistent - just calling it out since the PR framing is about partitioned; the behavior change is slightly broader than the title suggests.

4. alreadySaved optimization - verified correct. The setNeedsRepartition(true) -> boolean change and the alreadySaved short-circuit are correct: the strategy field is assigned before the flag flip, so the flag's save carries the strategy, and only a real CAS transition returns true; every other path (flag already set, no shape change) falls through to the explicit save. Not guarding on 'strategy unchanged' (so a re-issued ALTER TYPE can repair an in-memory/on-disk divergence after a transient IOException) is a nice touch and correctly reasoned. All setNeedsRepartition callers ignore the return value, so the signature change is safe.

Nits. .codacy.yml **/src/test/* -> **/src/test/** is a good catch and unrelated-but-welcome; the inline comment explains why. The SCHEMA_CHANGE 'report the whole picture, not the delta' choice means repeated CREATE INDEX on a partitioned type re-logs prior advisories each time - documented as intentional and bounded by index count, reasonable, though a type with many indexes emits several lines per DDL. Fine as-is.

Nothing here is blocking. Item 1 is the only one I would suggest acting on before merge; the rest are notes. Nice work - the test coverage in particular is a model for this kind of lifecycle fix.

[claude]

Review of #5644 - persist the partition strategy, and report the index change that undoes it

Thorough, well-scoped fix with excellent inline documentation and strong test coverage. The design decisions (moving validation out of the binding path, the three-way PartitionReport reaction split, and per-type fault isolation on load) are sound and address a real correctness gap: a correctly-partitioned type silently coming back round-robin after a restart. Nice work. I verified the change against the surrounding code rather than just the diff.

What holds up well

• setType no longer validates. Moving the unique-index requirement from setType() into checkSuitability() is the right call: setType runs on every rebind (per-bucket, per-bucket-add, and the loader own rebind on every open), so a throw there was a landmine that aborted the rest of readConfiguration. Binding being total-and-never-throwing is a much cleaner contract.
• alreadySaved double-save avoidance is correct. setNeedsRepartition(true) only returns true on a real CAS transition, and by then the strategy field (assigned at the top of the method) is already in place, so its saveConfiguration() serializes the strategy too - the second save is genuinely redundant and correctly skipped. The isReadingFromFile guard covers both writes.
• setNeedsRepartition return-type change is safe. All callers use it in statement position, and it is not declared on the DocumentType interface, so widening void to boolean breaks nothing.
• TypeIndexBuilder.create() hook placement. Calling reportPartitionSuitabilityAfterSchemaChange() outside the try/catch (so a diagnostic can never fall into the cleanup arm and undo a successfully built index) and once per CREATE INDEX rather than per bucket in addIndexInternal is the correct seam. Confirmed reload restores indexes via index.setMetadata(...), not TypeIndexBuilder.create(), so it will not fire on open.
• Test coverage is genuinely strong. The lifecycle suite pins persistence, reopen survival, both index-change reports, and the three controls (unchanged re-create stays quiet, round-robin never diagnosed, revert-to-default persisted as absence). checkSuitabilityNamesEveryBlockerAndWarning still holds (Diag has the unique index on k, so no extra blocker), and anUndeclaredPartitionPropertyCannotBeIndexedAndThereforeCannotBePartitionedOn still matches on "unique automatic index" since that substring now lands in the SchemaException wrapper.

Minor observations (non-blocking)

1. catch (final Exception e) in readConfiguration is broad. The isolation goal is right, but catching bare Exception also swallows genuine programming errors (e.g. a future NPE) and silently degrades that type to round-robin. It does log at WARNING with the throwable, so it is recoverable operationally - just worth being aware a real bug here reads as "one type lost its strategy" rather than surfacing loudly.
2. Exception type/wording nuance. The missing-index refusal now travels as a SchemaException wrapper rather than the old IllegalArgumentException from setType. The PR body "same wording" is approximate - the substring is preserved (so tests pass) but the surrounding message and exception type differ. Fine in practice; noting it for anyone matching on the old form.
3. .codacy.yml and the release-notes entry are unrelated to the issue but harmless and correctly justified (the src/test glob fix is a legitimate drive-by).

Not verified

I could not run mvn verify in the review sandbox (permission-gated), so I am relying on the stated "engine suite green (10545 tests) + PartitionedStrategyRefusalHttpTest" for the dynamic result. Everything checks out statically.

Overall: clean, careful, and the reasoning in the comments is unusually good. LGTM pending CI.

[claude]

Review: #5644 - persist the partition strategy, and report the index change that undoes it

Reviewed the full diff against the current tree. This is a high-quality, well-iterated change: the failure modes are real, the fixes are minimal and correctly placed, the reasoning is captured in the code (not just the PR body), and the test coverage is genuinely adversarial (red-before/green-after plus the "stays quiet" controls that keep the reports from becoming noise). The canPrune() -> isUsable() rename is applied consistently, and the load-path fault isolation in LocalSchema with the WARNING/SEVERE split is a nice touch.

A few observations, none of them blocking:

1. A bare DROP INDEX on the partition key is still silent until restart (minor / follow-up)

The new diagnosis hook lives in TypeIndexBuilder.create(), so it only fires on CREATE INDEX. The moment that actually makes a partition unusable - dropping the unique partition index and not recreating it - draws nothing until the next open, where RELOAD logs the blocker. That is a strictly smaller gap than before this PR and correctness is unaffected (lookups fan out), but given the PR's framing is "say what a schema change cost the partitioning", the DROP INDEX half is worth a note or a follow-up issue. anIndexChangeThatChangesNothingStaysQuiet exercises drop-then-recreate, which papers over the drop-only case.

2. The post-build diagnostic can fail a CREATE INDEX that already succeeded (low)

type.reportPartitionSuitabilityAfterSchemaChange() at TypeIndexBuilder.java:427 runs after created is resolved and deliberately outside the try/catch - the comment explains this well (a diagnostic must not fall into the cleanup arm and undo a built index). But the flip side is that if checkSuitability() ever throws an unexpected RuntimeException (it dereferences getEmbedded().getDatabase(), walks getAllIndexes(true), and resolves the serializer's read-back class), that exception propagates out of create() after the index was built and committed - so the caller sees "Error on creating index" for an index that actually exists. On SCHEMA_CHANGE the suitability path itself never throws by design, so this is only reachable via an internal bug, but since the whole point of putting the call outside the try/catch is "a diagnostic must not affect the index", wrapping the call in a catch-and-log would complete that guarantee: a broken diagnostic then costs a missing warning instead of a spuriously-failed DDL.

3. saveConfiguration() on every re-issued ALTER TYPE ... BucketSelectionStrategy (noted, agreed)

The comment at LocalDocumentType.java:815 already defends not guarding on "strategy unchanged" (re-issuing is the operator's way to repair a schema.json that a prior transient IOException left round-robin). Agreed with the reasoning; the cost is one full schema.json rewrite per redundant DDL statement, which is negligible on a per-statement path. Flagging only so it's a conscious trade-off on the record.

Nits

• .codacy.yml src/test/* -> src/test/** is a correct fix (the helper WarningCapture.java does not end in Test.java/IT.java and was being analysed as production code); good catch, sensible to fold in.
• WarningCapture swaps a process-global logger and only holds under single-threaded execution; that is documented in its Javadoc and matches the rest of the engine suite, so fine as-is - just a landmine if this module ever goes parallel.

Nothing here needs to block the merge; #2 is the only one I would consider addressing in this PR, and even that is defensive.

[claude]

Review: #5644 - persist the partition strategy, and report the index change that undoes it

Thoroughly reviewed the diff, the surrounding LocalDocumentType/LocalSchema/TypeIndexBuilder/PartitionedBucketSelectionStrategy code, and the new tests. This is a careful, high-quality change. The three-part framing (persistence, later CREATE INDEX re-check, load-path fault isolation) is coherent, and the reasoning is unusually well captured in the code comments.

What holds up well

• The persistence fix is correct and complete. setBucketSelectionStrategy now saves on its own, skips the redundant write during reload (isReadingFromFile()), and avoids double-writing both when setNeedsRepartition already flushed in the same call (alreadySaved) and when an enclosing recordFileChanges will save (persistOnItsOwn). The alreadySaved = setNeedsRepartition(true) interplay is subtle but sound: the strategy field is assigned before the flag flip, so that write already carries it; and when the flag was already true the setter returns false, so the explicit save still fires. Reverting to round-robin correctly persists as absence of the entry.
• Moving the unique-index requirement out of setType() into checkSuitability() is the right call. A bind that runs on every open (loader, addBucketInternal, per-bucket addIndexInternal) must not throw; making it a blocker that is refused at assignment but warned at reload keeps a degraded database openable, which is exactly the load-path abort the PR set out to remove.
• TypeIndexBuilder.create() placement is right - once per CREATE INDEX rather than per bucket, outside the try/catch so a diagnostic can't fall into the cleanup arm and undo a committed index, and with its own inner catch so a diagnosis fault can't fail a command over an already-registered index (which would then fail retry with "already exists").
• Fault isolation in LocalSchema.readConfiguration() with the WARNING-vs-SEVERE split (expected refusal vs. real bind fault) and toString() fallback for message-less exceptions is a genuinely nice touch, and it is mutation-checked in the tests.
• Test coverage is strong: 9 tests with red-before/green-after and per-guard mutation checks, honest controls (unchanged re-create stays quiet, round-robin never diagnosed), the WarningCapture extraction is clean, and removing the saveConfiguration() workaround from PartitionedStrategySuitabilityTest closes the loop.

Observations (non-blocking)

1. Runtime DROP INDEX of the partition key is not diagnosed until the next reopen. Only TypeIndexBuilder.create() calls reportPartitionSuitabilityAfterSchemaChange(); LocalSchema.dropIndex() has no equivalent hook. So the symmetric case to the one this PR fixes - an operator who drops the partition index rather than recollating it - gets no immediate signal and only sees the RELOAD warning at the next restart. The CREATE INDEX case is arguably the more important one (it silently turns a working partition unprunable), and the correctness invariant holds either way, but the asymmetry is worth a follow-up issue given how much of this PR is about closing exactly that "nothing said so until restart" gap.

2. WarningCapture swaps the process-global logger. This is documented in the class Javadoc and relies on the engine suite running sequentially. That is fine today, but since these tests aren't tagged, if the module ever moves to parallel test execution these will produce cross-talk false assertions. Worth keeping in mind if parallelization is ever considered for arcadedb-engine.

3. Inheritance path (addSuperType, createIndexes = false) runs the ASSIGNMENT reaction, which can throw SchemaException on a blocker. This matches prior behavior (old setType() threw IllegalArgumentException at the same spot), so it is not a regression, and the new inheritance tests cover the re-attach case. Just flagging that a subtype whose polymorphic partition config is a blocker will fail the addSuperType/re-attach rather than degrade - which is the intended assignment-time contract, but a reviewer of a future change here should be aware the throw is reachable without any explicit ALTER TYPE.

Nits

• The comment volume is high (deliberately, and it is good context), but a few blocks in setBucketSelectionStrategy restate the same rationale two or three times; a future reader might appreciate one consolidated note. Not worth churning the diff for.

Overall: correct, well-tested, and the documentation makes the intent auditable. LGTM modulo the DROP INDEX follow-up in (1).

[claude]

Review of PR 5644 - persist the partition strategy, and report the index change that undoes it

I read the full diff (all 8 commits) against the surrounding code in LocalDocumentType, LocalSchema, TypeIndexBuilder, and PartitionedBucketSelectionStrategy. This is a careful, well-reasoned fix and the test coverage is genuinely strong.

What holds up well

• The core fix is correct. The strategy field is assigned (this.bucketSelectionStrategy = selectionStrategy) before both setNeedsRepartition(true) and the explicit saveConfiguration(), so whichever write fires serializes the new strategy. Reverting to round-robin still persists as the absence of the entry, and the reopen path confirms it.
• The alreadySaved / persistOnItsOwn de-duplication is sound. setNeedsRepartition now returns whether it actually transitioned (CAS), so it only reports true when it really wrote; the enclosing-recordFileChanges case (addSuperType) correctly opts out with persistOnItsOwn=false. No double rewrite, no missed write.
• Moving validation out of setType() into checkSuitability() is the right call. Binding runs on every rebind (per-bucket, per-open), so a throw there was exactly what made a DROP-INDEX-ed partition key unopenable. RELOAD mode logging instead of throwing, plus the per-type try/catch in readConfiguration, restores the "database opens and says why" guarantee. I confirmed the unresolvable-class path throws SchemaException (LocalDocumentType:1014), so it lands in the expected -> WARNING branch the test asserts.
• canPrune() -> isUsable() rename matches the widened meaning and the test call site is updated.
• Test suite is thorough and honest: red-before-fix cases, the three controls (unchanged re-create stays quiet, round-robin never diagnosed, revert persists as absence), the two individually mutation-checked load-path guards, and the WARNING-vs-SEVERE split asserted rather than assumed. The follow-ups (5645 / 5646) are documented right where the limitation lives.

Points worth a look (none blocking)

1. Exception-type change on the inheritance path. With binding no longer validating, addSuperType -> setBucketSelectionStrategy(copy, false) now reaches the ASSIGNMENT refusal, which throws SchemaException, where the old setType threw IllegalArgumentException. Both still throw for a genuinely unsuitable inherited partition, so CREATE ... EXTENDS still fails - but the type changed. The PR body covers this for the ALTER TYPE path (AlterTypeStatement catches both); worth confirming the inheritance/addSuperType callers are equally covered.

2. IllegalArgumentException in the readConfiguration "expected" classification. Now that binding never throws IllegalArgumentException, is that arm still reachable during reload, or is it now dead defensiveness? Harmless either way; a one-word note on why it is kept would save the next reader the question.

3. Global logger swap in WarningCapture is process-wide. Documented clearly and matches how the rest of the engine module asserts on logs under sequential execution. Just flagging that it quietly assumes no parallel test execution for this module - the finally restore keeps the blast radius to false assertions, not a leaked logger, so acceptable.

4. Comment-to-code ratio is very high in LocalDocumentType / TypeIndexBuilder. Consistent with the existing partitioned-strategy series, so I would keep it, but a couple of the "why not the alternative" paragraphs (e.g. the SCHEMA_CHANGE delta-vs-whole-picture rationale) could be compressed without losing the load-bearing content.

Style / conventions

Consistent with CLAUDE.md: final throughout, classes imported (no FQNs), assertThat(...).isTrue() test style, no System.out, cold-path allocation only. The .codacy.yml /src/test/ widening is a correct, self-contained fix for the non-recursive src/test/* glob.

Nice work - the fix is minimal where it matters and the tests pin the behavior tightly. My only real ask is confirming the inheritance-path exception-type change (point 1) does not surprise a caller.

[claude]

Code review — PR 5644

Reviewed the full diff (9 commits) against main, focusing on the engine schema/partitioning paths. This is a carefully reasoned change; the fixes are correct and the rationale is captured well in code comments.

What the PR does well

• Root cause is right. ALTER TYPE ... BucketSelectionStrategy was the one schema mutator not persisting on its own; adding schema.saveConfiguration() to setBucketSelectionStrategy (guarded by !isReadingFromFile()) is the correct place, and skipping it during reload avoids rewriting an identical schema.json on every open.
• Moving validation out of setType() into checkSuitability() is the right call. Binding running on every rebind (per-bucket, per-open) meant a DROP INDEX on the partition key made the loader throw and abort everything after it. Making binding side-effect-free and folding the unique-index requirement into the suitability check (refuse at assignment, warn on reload) is a clean separation of compute vs. diagnose.
• Defensive layering is solid. TypeIndexBuilder.create() re-runs the diagnosis outside the build try/catch (so a diagnostic cannot trigger index cleanup) and wraps the call in its own try/catch (so a fault in the check cannot fail a command over an already-committed index). Both reasons are documented.
• LocalSchema per-type isolation is a genuine robustness win: one type unusable strategy now degrades that type to round-robin with a WARNING instead of resetting the whole schema, and the WARNING/SEVERE level split (expected refusal vs. real bind fault) is a nice touch.
• Rename canPrune() to isUsable() better reflects that a missing partition index removes a lookup rather than blocking bucket computation.
• Test coverage is strong. 9 lifecycle tests covering persistence, reopen survival, both index-change reports, revert-to-default, and the two load-path guards, plus three controls. The PR notes 4 were red before the fix and the load-path guards were mutation-checked individually.

Observations / minor points

1. Global logger swap in WarningCapture is process-global. The helper swaps the LogManager logger, so the tests only hold under sequential execution. Explicitly documented and matches every other log-asserting test in the module - an accepted tradeoff, but worth remembering if the engine suite ever goes parallel (false assertions, not a leak, since finally restores).
2. saveConfiguration() deliberately not guarded on strategy-actually-changed. Justified in the comment (re-issuing the same ALTER TYPE is the operator repair after a transient IOException that logged SEVERE and returned). Reasonable; just flagging it is an intentional divergence from sibling mutators.
3. SCHEMA_CHANGE reports the whole current picture, not the delta, so a later unrelated CREATE INDEX re-emits an existing fan-out advisory. Documented and justified (bounded by index count, rare DDL). Acceptable; only reads as noise if a type accumulates many non-partition indexes.
4. Style/conventions all consistent with CLAUDE.md: imports over FQNs, final throughout, brace-less single-statement if, assertThat(...).isEqualTo(...). Fast functional tests, correctly not tagged slow/benchmark.

Not verified locally

Maven runs require approval in this environment, so I did not re-run the suite; the PR reports the full arcadedb-engine suite green (10545 tests, 0 failures) plus PartitionedStrategyRefusalHttpTest. The changed sources read as internally consistent.

Overall: correct, well-tested, and unusually well-documented. No blocking issues found.

[codecov]

Codecov Report

❌ Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.31%. Comparing base (3275298) to head (00f5eb4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/main/java/com/arcadedb/schema/LocalSchema.java	57.14%	0 Missing and 3 partials ⚠️
...ain/java/com/arcadedb/schema/TypeIndexBuilder.java	66.66%	2 Missing ⚠️
...onstrategy/PartitionedBucketSelectionStrategy.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5644      +/-   ##
============================================
+ Coverage     66.86%   67.31%   +0.44%     
============================================
  Files          1771     1771              
  Lines        149846   149871      +25     
  Branches      31768    31773       +5     
============================================
+ Hits         100197   100879     +682     
+ Misses        36522    35857     -665     
- Partials      13127    13135       +8     

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

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 7 complexity

Metric	Results
Complexity	7

View in Codacy

🟢 Coverage 94.74% diff coverage · -6.41% coverage variation

Metric	Results
Coverage variation	✅ -6.41% coverage variation
Diff coverage	✅ 94.74% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (3275298)	149846	113368	75.66%
Head commit (00f5eb4)	181863 (+32017)	125935 (+12567)	69.25% (-6.41%)

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 (#5644)	38	36	94.74%

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]

Code Review

Reviewed the full diff against main. This is a careful, well-reasoned fix and the inline documentation is genuinely excellent - the why is recorded exactly where a future reader needs it. Summary: looks correct and well-tested. Notes below are mostly confirmations plus a few small observations; no blocking issues found.

Correctness - the load-order dependency is the crux, and it holds. The whole fix hinges on checkSuitability() now reporting a missing partition index as a blocker. That would be a false positive on every healthy reopen if the strategy were bound before its index. It is not: LocalSchema.readConfiguration() binds strategies explicitly after the indexes are attached (the "SET THE BUCKET STRATEGY AFTER THE INDEXES" block), and inside the readingFromFile == true window, so it correctly takes PartitionReport.RELOAD (log-only, warnings suppressed). A stock partitioned DB stays quiet on reopen.

Persistence logic is sound. ALTER TYPE ... BucketSelectionStrategy routes through AlterTypeStatement -> setBucketSelectionStrategy(String) -> the public 1-arg setter (persistOnItsOwn = true), and that branch is not wrapped in recordFileChanges, so persisting on its own is the right call and will not double-write. The alreadySaved short-circuit against setNeedsRepartitions save is correct in every combination I traced (shape-changed + records, CAS-lost, revert-to-round-robin), because the strategy field is assigned before either save fires. The exception-safe rollback (restore previous, rethrow) keeps a refused ASSIGNMENT from leaving a half-bound type.

Fault isolation is a real robustness win. Moving validation out of setType() (binding must never throw) and wrapping the per-type strategy restore in LocalSchema so one bad strategy cannot abort the rest of the load (triggers, extensions, function libs, compaction migration map) is the right shape. The WARNING-vs-SEVERE split keyed on SchemaException | IllegalArgumentException is a nice touch, and anUnresolvableStrategyDoesNotCostTheRestOfTheSchema pins it via an extension read strictly after the strategy block - a well-chosen probe.

Test coverage. PartitionedStrategyLifecycleTest covers the reddened cases plus honest controls (unchanged re-create stays quiet, round-robin never diagnosed, revert persists as absence). Extracting WarningCapture from the suitability test and dropping the saveConfiguration() workaround are good cleanups. The WarningCapture javadoc correctly flags the process-global logger swap as safe only under serial execution - consistent with the rest of the module.

Minor observations (non-blocking):

• .codacy.yml (**/src/test/* -> **/src/test/**): technically unrelated to the issue, but it is what stops the new non-Test support class (WarningCapture.java) from being analysed as production code, so it earns its place. Worth a one-line mention in the PR body since it is a config change riding along.
• SCHEMA_CHANGE re-reports the whole picture, so an unrelated later CREATE INDEX re-emits a pre-existing fan-out advisory. Documented and deliberate, and CREATE INDEX is rare DDL - acceptable, just noting operators may see a repeated advisory line.
• DROP INDEX on the partition key stays silent until reopen - acknowledged in-code and tracked as a follow-up issue. Reasonable scope boundary.
• The known subtype-with-different-bucket-count crash is correctly left out of scope and documented in the test rather than pinned by a test that would fail for the wrong reason.

Build/verification. Author reports the full arcadedb-engine suite green (10545 tests) plus PartitionedStrategyRefusalHttpTest. I reviewed the logic paths by hand (could not run Maven in this sandbox); nothing I traced contradicts that.

Nice work - the change is minimal at the mutator, the fault isolation is the right instinct, and the tests target the actual regressions rather than the happy path.