fix(engine) #5589: verify the partition properties before pruning an index lookup to one bucket - #5591
Conversation
…index lookup to one bucket A type using `partitioned(...)` places a record in the bucket its PARTITION key hashes to, but TypeIndex.getIndexesByKeys pruned every lookup to the bucket the LOOKUP key hashed to. Those coincide only for the partition index itself; for any other index of the type they are unrelated, so the pruned search read a bucket the record was not in. The lookup silently returned nothing, and since the commit-time duplicate check reads the same path, a secondary UNIQUE index stopped rejecting duplicates. Rather than guard the one call site, the bucket-selection contract now carries the property names the key values belong to, so the mismatch cannot be expressed: getBucketIdByKeys(List, Object[], boolean) lets a partitioning strategy check the lookup covers exactly its own properties and return -1 otherwise, which callers already read as "search every bucket". That also covers a partial key on a composite partition, which hashes fewer values than placement did. The single-argument overloads are deprecated and resolve to the unverifiable case, so they never prune. Pruning on the partition key is unchanged, composite keys included, and both planner pruning rules now pass their partition properties through the checked path. Tests: PartitionedSecondaryIndexLookupTest pins the secondary-index lookup, the secondary UNIQUE constraint, the round-robin control, the surviving single-bucket pruning on the partition key, the fan-out on a non-partition key, and the composite partition key.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage 96.55% diff coverage · -7.55% coverage variation
Metric Results Coverage variation ✅ -7.55% coverage variation Diff coverage ✅ 96.55% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (c60958f) 147717 111689 75.61% Head commit (10249c4) 179725 (+32008) 122321 (+10632) 68.06% (-7.55%) 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 (#5591) 29 28 96.55% 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.
Review: #5589 verify partition properties before pruning an index lookupSolid fix with an excellent write-up. I traced the type contract across every call site and implementer and it holds together. Summary: approve, with a couple of minor/optional notes. What I verified
Minor / optional
On the abstract-vs-default question you raisedMaking the 3-arg method TestsCoverage is strong: secondary-index lookup, secondary Nice work - the "make the mismatch unrepresentable" framing is a cleaner fix than guarding the single call site. |
…quality Review follow-up on #5591. coversPartitionProperties tested that every lookup property was also a partition property, which accepts a repeated property standing in for a missing one: [a, a] passed against a partition of [a, b] even though it sums a different pair of values than placement did, so it would have pruned to the wrong bucket. Unreachable while no index declares a repeated property, but this method is the guard the fix rests on, so it now compares occurrence counts and enforces the invariant rather than assuming callers uphold it. Still allocation-free: partition keys hold one to three properties. Also dropped the propertyNames == null branch. Both constructors assign a non-null list (the list constructor NPEs on null inside Collections.unmodifiableList), so the fallback to super could never execute. Test: aRepeatedLookupPropertyDoesNotPassForACompositePartition drives the strategy directly, since no query can produce a repeated lookup property - covering the partition set, a permutation of it, the repeated property, and the unverifiable null.
|
Review: fix(engine) #5589 - verify partition properties before pruning an index lookup Reviewed the diff, the surrounding code (TypeIndex.getIndexesByKeys, the strategy hierarchy, both planner call sites), and the new test. This is a clean, well-reasoned fix with excellent documentation. Recommendation: approve. Correctness - the root-cause framing is right. Widening Things I specifically checked and confirmed hold:
Test coverage. Strong. The suite pins the real symptom (secondary-index lookup returning 0), the more serious one (secondary UNIQUE no longer enforcing), a round-robin control, the surviving single-bucket optimization on the partition key, the fan-out on a non-partition key, and the composite-partition cases including direct multiset/null assertions against the strategy. Three failing on main is exactly what you want. Minor notes (non-blocking):
Nice work - the failure analysis in the description (the 6-accepted/32-rejected duplicate table especially) makes the severity and the fix easy to trust. I could not execute the test suite in this environment (Maven is sandboxed), so the pass/fail claims above rest on code reading plus the results reported in the PR description. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5591 +/- ##
============================================
+ Coverage 65.90% 65.93% +0.03%
- Complexity 1114 1116 +2
============================================
Files 1751 1751
Lines 147670 147733 +63
Branches 31561 31577 +16
============================================
+ Hits 97325 97414 +89
+ Misses 37353 37327 -26
Partials 12992 12992 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Fixes #5589.
A type using
partitioned(...)bucket selection places each record in the bucket its partition key hashes to, butTypeIndex.getIndexesByKeyspruned every lookup to the bucket the lookup key hashes to. Those coincide only for the partition index itself; for any other index of the type they are unrelated, so the pruned search read a bucket the record was not in.Two silent consequences, both measured on
main:tenant_id, UNIQUE oncode)code, viaTypeIndex.getand via SQLcodeDuplicatedKeyExceptionThe second one matters most: the commit-time duplicate check reads through the same pruned path, so a secondary
UNIQUEindex stopped enforcing its constraint.Approach
The issue proposed guarding the single call site. This does something a bit broader instead, because the call site was not really the defect:
getBucketIdByKeys(Object[] keyValues, boolean async)took an untyped key array with no statement of which properties those values described, so neither the caller nor the strategy could tell a partition key from an unrelated index's key.The property names now go into the contract:
A strategy whose placement depends on the key verifies the lookup covers exactly its own properties and returns
-1otherwise, which every caller already reads as "search every bucket" - correct, only slower. A strategy whose placement does not depend on the key (the plannedTenantBucketSelectionStrategyindocs/multitenant.md§6.3 routes by session context) ignores the parameter and can still answer for any index. The mismatch becomes unrepresentable rather than guarded in one place.Property matching is deliberately order-insensitive, since the hash both sides compute is a commutative sum over the per-value hash codes, and it is done with nested scans rather than a
Setto stay allocation-free on a path that runs per query.This also covers a case the issue did not mention: a partial key on a composite partition hashes fewer values than placement used. It cannot reach the strategy through
get()today because the index contract rejects it first, but the guard covers it regardless.Compatibility
SelectExecutionPlanner,PartitionPruning) now pass their partition properties through the checked path.getBucketIdByKeys(Object[], boolean)andDocumentType.getBucketIndexByKeys(Object[], boolean)are deprecated and resolve to the unverifiable case, so they never prune.defaultreturning-1plus a one-time warning if reviewers prefer no build break at all.partitioned(...)on a type with more than one index may already hold duplicates in a secondaryUNIQUEindex, admitted while the check read the wrong bucket. The constraint is enforced again from this change, but existing rows are not retro-validated; the release note tells operators to scan andREBUILD INDEX.Tests
New
PartitionedSecondaryIndexLookupTest(6 tests) pins the secondary-index lookup, the secondaryUNIQUEconstraint, the round-robin control, the surviving single-bucket pruning on the partition key, the fan-out on a non-partition key, and the composite partition key. Three of them fail onmain.Also run green: 939 tests in
com.arcadedb.index.**, 319 incom.arcadedb.schema.**, and the SQL/Cypher partition-pruning suites, plus a full-reactorcompileandtest-compile.