fix(engine) #5595: normalise a partition lookup key to the declared type before hashing it

[lvca]

Closes #5595.

The defect

PartitionedBucketSelectionStrategy derives the bucket from Object.hashCode() on both sides, but the two sides saw differently boxed objects:

• Placement (getBucketIdByRecord) hashes the value after the schema coerced it to the declared property type. Every write path funnels through MutableDocument.convertValueToSchemaType, so a LONG property always holds a Long.
• Lookup (getBucketIdByKeys) hashed whatever the caller passed. TypeIndex hands over the raw keys, and the index's own convertKeys runs much later inside LSMTreeIndex.get.

Long.hashCode(v) is (int) (v ^ (v >>> 32)) while Integer.hashCode(v) is v, so the two agree only for positive values below 2^31. On a LONG partition key every negative value, and every value outside the int range, pruned to a bucket the record had never been placed in and the lookup silently found nothing.

This is not confined to the Java index API. A plain SELECT FROM T WHERE id = -5 hits it too: a SQL integer literal that fits an int arrives as an Integer, and SelectExecutionPlanner prunes buckets through the same strategy. That is the more visible symptom, and the issue did not mention it.

The fix

The lookup key is converted to the declared property type - the very coercion the write path applies - before being hashed, so both sides hash the same object. Placement is untouched, so no existing database needs a repartition.

Rather than inlining the coercion into the strategy, the mapping from a declared type to the Java class it is stored as now lives in one place, Type.getJavaImplementation(Database), and MutableDocument routes through it as well. The failure mode here was precisely two places independently deciding what a value is stored as; duplicating the DATE/DATETIME special-casing into the strategy would have recreated it.

I did not take the issue's stronger alternative (hashing serializeKeyForHashing bytes), for the reason the issue itself gives: it changes the placement function, so every existing partitioned database would need a repartition that REBUILD TYPE ... WITH repartition = true refuses on graph types.

Where it declines to prune

Returning -1 makes the caller fan out over every bucket: correct, only slower.

• the partition property is not declared in the schema, so the record kept whatever Java type the writer used and there is no conversion target;
• the key does not coerce to the declared type at all;
• the partition index declares COLLATE CI - a sibling defect found while fixing this one, see below.

The COLLATE CI case

Case folding is an index-level normalisation that placement never applied, so 'Hello' and 'hello' are a single index key living in two different buckets. No lookup-side normalisation repairs that - only a change to placement could, which would force a repartition - so such a partition is no longer pruned.

Measured red/green with a 3-bucket type. Bucket counts that are a power of two up to 32 hide it by arithmetic accident: flipping the case of an ASCII letter shifts the Java string hash by a multiple of 32, so the low 5 bits survive.

Testing

New engine/src/test/java/com/arcadedb/partitioning/PartitionedBoxedKeyLookupTest.java, 7 tests. 3 were red on main (which already carries #5591); the round-robin control was green throughout. Each of the three declining paths above was verified red with the guard disabled and green with it.

Full engine module suite: 10351 tests, 0 failures, 23 skipped.

Two honest notes

• The duplicate-admission half of the issue does not reproduce for a declared property. Both sides of the commit-time check (TransactionIndexContext.checkUniqueIndexKeys -> TypeIndex.get) read schema-coerced values, so they agree. The issue said it was inferred from the shared code path and not reproduced directly; that inference does not hold here. aUniquePartitionIndexRejectsADuplicateBoxedDifferently is kept as a regression guard, but it passed before the fix as well.
• MutableDocument.convertValueToSchemaType is a very hot write path. The refactor is behaviour-preserving - for DATE/DATETIME the helper returns the same configured implementation class, which is never Document.class, so hoisting the embedded-document check out of the else is equivalent - and the full suite was run rather than just the partitioning tests because of it.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[claude]

Review: #5595 - normalise a partition lookup key to the declared type before hashing

Overall this is a careful, high-quality fix with an excellent write-up. The root cause (placement hashing the schema-coerced value while lookup hashed the caller's raw box) is correctly diagnosed, and fixing the lookup side rather than placement, so existing databases don't need a repartition, is the right call. The three "decline to prune" paths (undeclared property, non-coercible key, COLLATE CI) all fail safe by fanning out. I verified the referenced methods (hasAnyCaseInsensitive, getPolymorphicPropertyIfExists, getMetadata, Type.convert) exist with the expected signatures.

Correctness

• MutableDocument refactor is behaviour-preserving. For DATE/DATETIME, getJavaImplementation(database) returns the configured impl class, which is never Document.class, so hoisting the embedded-document check out of the old else branch is equivalent, as the PR claims. Type.convert is still called with the same javaImplementation, so the write path is unchanged.
• Shared helper is the right structural move. Collapsing the "declared type -> stored Java class" mapping into Type.getJavaImplementation removes exactly the duplication that caused the bug; both sides now provably agree.
• Alignment is correct. toStoredForm(lookupProperties.get(i), keyValues[i]) uses the lookup property name, not propertyNames.get(i), so it stays correct under the permuted-key case that coversPartitionProperties deliberately allows.

Test coverage - one real gap worth addressing

The 7 tests all assert records are found (via index.get(...).hasNext() or SQL). But fan-out (return -1) also finds every record. So these tests would still pass if the fix regressed to "always return -1" and never pruned at all - they prove correctness but not that pruning still selects the right single bucket. The only test that exercises -1 on purpose is the round-robin control.

Consider adding a direct assertion that, for a partitioned type, getBucketIdByKeys(props, {intKey}) returns the same bucket as getBucketIdByRecord(record) for the numerically-equal stored value (and that it is not -1). That pins the behaviour the fix actually delivers - pruning to the correct bucket - and guards against a silent regression into permanent fan-out, which is a performance cliff no functional test here would catch.

Minor test nit: in aLongKeyLookedUpAsAnIntegerFindsTheRecord, incrementing foundAsInteger for out-of-range values "to keep the tallies comparable" is a bit of a fudge; asserting per-value would read more clearly. Cosmetic.

Smaller notes (non-blocking)

• toStoredForm catches Exception broadly. Coercion failure legitimately means "can't reproduce, fan out", but a swallowed RuntimeException from an unrelated bug would silently degrade to fan-out and hide the defect. On a correctness-critical path, narrowing the catch (or a debug-level log) would keep a future latent bug observable. Defensive as-is, so optional.
• Per-query cost. partitionKeyIsCaseInsensitive() + toStoredForm() add a schema/index resolution and a Type.convert per getBucketIdByKeys call. Since this runs at prune time (~once per query, not per row) and Type.convert short-circuits on same-type values, the overhead is negligible and well justified. The Javadoc's reasoning for the deliberate no-cache choice on the CI check is nicely done.
• Docs and comments are exemplary and make the invariant easy to maintain.

Verdict

Solid, well-reasoned, minimal-blast-radius fix. The main actionable item is strengthening the tests to assert pruning to the correct bucket rather than only record found, since the current suite cannot distinguish the fix from a regression to unconditional fan-out.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 9 complexity

Metric	Results
Complexity	9

View in Codacy

🟢 Coverage 60.00% diff coverage · -6.70% coverage variation

Metric	Results
Coverage variation	✅ -6.70% coverage variation
Diff coverage	✅ 60.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (f0f341d)	147814	110511	74.76%
Head commit (0a58a2c)	179828 (+32014)	122397 (+11886)	68.06% (-6.70%)

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 (#5598)	40	24	60.00%

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 diff against the fix's stated design. This is a well-scoped, well-reasoned fix, and the write-up on the PR is genuinely excellent - the rationale (why normalise the lookup key rather than change placement) and the honest "does not reproduce" note on the duplicate-admission half are exactly right.

Correctness

• The core fix is sound: getBucketIdByRecord hashes the schema-coerced stored value, and getBucketIdByKeys now replays that same coercion via toStoredForm before hashing, so both sides hash the same object. The UNKNOWN_STORED_FORM sentinel (distinct object, identity-compared, since null is a legitimate conversion result) is the right call.
• Pairing is correct: coversPartitionProperties allows a permuted property set (sum-of-hashes is commutative), and the loop uses lookupProperties.get(i) with keyValues[i], so each value is coerced against its own property name even under permutation.
• The MutableDocument.convertValueToSchemaType refactor is behaviour-preserving. The embedded-document check was previously only in the else branch, but hoisting it is safe because getJavaImplementation returns the date/datetime impl for DATE/DATETIME, which is never Document.class, so the check is a no-op for those types - same as before.
• Centralising the DATE/DATETIME mapping in Type.getJavaImplementation is the right anti-duplication move given the failure mode was "two places independently deciding the stored form."
• The COLLATE CI decline is a genuine sibling bug caught here, and declining (fan-out) is the only correct answer since only a repartition could reconcile it. Nice catch, and the deliberately non-power-of-two bucket count in the test that actually exposes it is a thoughtful detail.

Minor points (non-blocking)

1. getJavaImplementation silent fallback for null/non-DatabaseInternal: when database isn't a DatabaseInternal, a DATE/DATETIME type silently returns javaDefaultType instead of the configured impl. Every current caller passes a DatabaseInternal (in MutableDocument the field is already DatabaseInternal, which is why the original direct getSerializer() call worked), so this is fine today and the javadoc documents it. Worth keeping in mind that a future caller passing null for a DATE property would get a divergent stored form with no error.

2. Per-lookup cost: getBucketIdByKeys now does a getPolymorphicIndexByProperties map lookup plus, per key, a getPolymorphicPropertyIfExists + getSchema().getEmbedded().getDatabase() + Type.convert, on a hot query-pruning path. All individually cheap (HashMap lookups, idempotent Long->Long conversion), and correctness clearly justifies it - the PR acknowledges this. The database resolution inside toStoredForm could be hoisted out of the per-key loop, but the gain is negligible.

3. Broad catch (Exception) at Level.FINE: correct defensive posture (never turn a conversion failure into a wrong bucket), and logging so a latent bug degrades visibly rather than silently. Just noting that if conversion started failing systematically, every query would silently fall back to a full fan-out and only whisper at FINE - the PR calls this out honestly.

Tests

Excellent coverage. The standout is anIntegerLookupKeyResolvesTheBucketPlacementChose and the direct -1 assertion in the CI test: they pin the real invariant (lookup resolves the same bucket placement chose, and still narrows to one sub-index) rather than "the record was found," which would pass even if the strategy regressed to unconditionally declining and never pruning again. The round-robin control and the INTEGER-property mirror case round it out well. Fast unit tests, no @Tag needed.

Overall: LGTM. The fix is minimal, correct, leaves placement untouched (so no repartition), and is backed by tests that guard the performance invariant and not just the visible symptom.

[claude]

Review: fix(engine) #5595 - normalise a partition lookup key to the declared type before hashing

Thorough, well-reasoned fix. The diagnosis (Long.hashCode vs Integer.hashCode diverging outside the positive-int range, placement hashing the schema-coerced value while lookup hashed the raw caller key) is correct, and the chosen remedy - normalise the lookup side and leave placement untouched so no repartition is forced - is the right trade-off. The PR body and Javadoc are exemplary; they explain not just what but why, including the declined alternatives.

Correctness - verified

• MutableDocument.convertValueToSchemaType refactor is behaviour-preserving. Hoisting the javaImplementation.equals(Document.class) check out of the old else branch is safe: for DATE/DATETIME, getJavaImplementation(database) returns the configured date impl class, which is never Document.class, so the embedded-document branch is skipped exactly as before. Good call running the full suite given this is a hot write path.
• Type.getJavaImplementation(Database) correctly centralises the DATE/DATETIME special-casing, and the null/!DatabaseInternal fallback to javaDefaultType is sound. In toStoredForm the database comes from type.getSchema().getEmbedded().getDatabase(), which is a DatabaseInternal at runtime, so the date path resolves correctly.
• getMetadata() can return null (empty indexesOnBuckets); partitionKeyIsCaseInsensitive() guards for it. Good.
• The UNKNOWN_STORED_FORM sentinel (distinct object vs null, since null is a legitimate conversion result) is the right pattern.

Test coverage

Excellent. anIntegerLookupKeyResolvesTheBucketPlacementChose is the standout: it pins the real invariant (lookup bucket == placement bucket, and pruning still narrows to one sub-index) rather than just "record found", which would mask a regression to unconditional -1. The COLLATE CI test deliberately using a non-power-of-two bucket count (3) to avoid the arithmetic accident that hides the defect at 8/16/32 shows real care. The out-of-int-range guards in the loops are correct.

Minor points / questions (non-blocking)

1. Placement is not normalised. getBucketIdByRecord still hashes the raw record.get(prop) value. This is correct as long as every write funnels through convertValueToSchemaType (as the PR states). If any path can populate a declared partition property while bypassing that coercion (bulk import, direct deserialization of externally-written data), a record could be placed with a differently-boxed hash than a fresh lookup now computes, reintroducing a miss. Worth a sentence in the Javadoc noting placement correctness depends on that write-path invariant.
2. Per-lookup cost. getBucketIdByKeys now does more work per call (index resolution in partitionKeyIsCaseInsensitive(), database resolution, per-key property lookup + Type.convert) versus the old bare hashCode(). This is the query-planning path, not per-record, and the Javadoc already justifies re-resolving rather than caching, so acceptable - just flagging it is a real (small) shift.
3. Broad catch (Exception) in toStoredForm logs at FINE and fans out. Correct and defensively reasonable; the rationale comment is appreciated. FINE means a genuine conversion regression would be near-invisible in production - a rate-limited WARNING might surface it better, though it risks log spam on legitimately-uncoercible keys, so FINE is defensible.

Style / conventions

Consistent with the codebase - final on params/locals, single-statement if without braces, imports (no FQNs), assertThat(...).isTrue() test style, and Apache headers all present.

Overall: correct, well-tested, and conservative in exactly the way this class of bug demands. LGTM.

[codecov]

Codecov Report

❌ Patch coverage is 45.00000% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.92%. Comparing base (f0f341d) to head (0a58a2c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...onstrategy/PartitionedBucketSelectionStrategy.java	50.00%	7 Missing and 4 partials ⚠️
...in/java/com/arcadedb/database/MutableDocument.java	16.66%	9 Missing and 1 partial ⚠️
engine/src/main/java/com/arcadedb/schema/Type.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5598      +/-   ##
============================================
- Coverage     65.93%   65.92%   -0.01%     
+ Complexity     1113     1112       -1     
============================================
  Files          1751     1751              
  Lines        147814   147836      +22     
  Branches      31605    31611       +6     
============================================
+ Hits          97457    97467      +10     
- Misses        37347    37354       +7     
- Partials      13010    13015       +5     

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