feat(drive): per-prefix ranked aggregates on compound indexes - #4393
Open
QuantumExplorer wants to merge 12 commits into
Open
feat(drive): per-prefix ranked aggregates on compound indexes#4393QuantumExplorer wants to merge 12 commits into
QuantumExplorer wants to merge 12 commits into
Conversation
Serve a grouped aggregate carrying exactly one HAVING clause on the selected aggregate (GROUP BY p HAVING <agg> <op> <value> LIMIT n) as a value-bounded range read of the covering ranked index's axis secondary — the same grovedb trees the PV14 ranked top-k surface walks — with a completeness-attesting proof. - rs-drive: drive_document_having_query (versioned grammar, bounds translation, executors) + document_having verifier; prover and verifier share one bounds-to-Merk-query translation and path builder - rs-drive-abci: compute_aggregate_mode_and_check_limit v2 routes the shape to dispatch_having_v1; response reuses RankedEntries with skipped unset, so zero proto changes - rs-platform-version: PV14 selects DRIVE_ABCI_QUERY_VERSIONS_V3; detect_having_mode / verify_having_range_proof slots dormant at 0 in all tables; v13 and earlier keep rejecting every non-empty HAVING - rs-drive-proof-verifier / rs-sdk: DocumentHavingEntries with FromProof/Fetch, binding the proof to the quorum-signed app hash Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins the worked example — SELECT AVG(grade) GROUP BY identityId HAVING AVG(grade) > 80 — against a contract whose group key is a 32-byte identifier rather than a string: strict-bound exclusion of an exactly-at-threshold average, inclusion of a fractional average just above it, byte-exact identifier keys in both walk directions, and proof verification against the live root hash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GROUP BY identityId, class HAVING AVG(grade) > 80 must be rejected, not misserved: ranked axes live on single-property indexes (a ranked flag on a compound index is already rejected at contract-parse time, covered by dpp's test_index_try_from_ranked_on_compound_index_rejected). Pins the drive grammar rejection and that it surfaces through the abci wire path as InvalidArgument naming the single-property rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lift the single-property restriction on ranked aggregate index flags: a compound index like [identityId, class] may now declare rankedCountable / rankedSummable / rankedAverageable, with per-prefix semantics — the ranked flags land on the terminal property-name level, one ordered secondary per prefix value, ranking only that prefix's trailing-property groups. No global cross-prefix ordering exists. rs-dpp keeps the one structurally impossible shape rejected, now as a cross-index check where the doctype's full index set is visible (validate_no_ranked_prefix_overlap): a countable/summable index terminating at exactly the compound's leading prefix would demand the NonCounted/NotSummed shell grovedb rejects around indexed trees. The check runs on validating and structural parses alike; drive's INDEXED_INNER_UNWRAPPABLE guard remains the fail-closed backstop. The write path needed no walker changes: the v2 walkers are arity-generic, and the pinned grovedb rev supports creating an indexed primary and populating it in the same batch, so the lazily-created per-prefix terminal trees maintain their secondaries through the ordinary document write path (verified end to end by the new integration suites; stale comments claiming otherwise corrected). Both query surfaces gain equality-prefix routing, v1 equality-only: every leading index property must be pinned by an == where clause (IN and range operators on a prefix are rejected loudly; multi-IN branching can layer on later), group_by names the trailing property. Resolution — covering-index pick plus pin encoding via serialize_value_for_key into prefix path segments — is one shared function per surface (resolve_ranked_query_for_mode / resolve_having_query_for_mode), called by the server executors and the SDK proof helpers, and the shared path builder gained the prefix-value segments, so prover and verifier cannot drift on which subtree a proof is about. abci needs no routing changes: the v2 aggregate-mode helper already routes grouped having / aggregate-ordered shapes regardless of where clauses, and both dispatchers forward where clauses to drive untouched. Protocol version 13 keeps rejecting every new shape before any contract fetch (pinned by wire-level tests). Everything sits in PV14-only modules (meta-schema v3 grammar, DRIVE_ABCI_QUERY_VERSIONS_V3), and PV14 is unreleased, so no version slots were added or renumbered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds equality-pinned compound ranked queries and protocol v14 grouped ChangesCompound ranked-index contracts
Equality-pinned ranked queries
HAVING range execution
Protocol routing
Proof verification and SDK results
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🔵 Low · up to This change enables compound-index ranked aggregate queries behind unreleased protocol version 14. It is mergeable with owner awareness, but the protocol-version table change should be explicitly confirmed as compatible with frozen deployed-version behavior, and misleading maintenance documentation should be corrected. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
⛔ Blockers found — Opus deferred (commit 9ec210b) |
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-08-13T04:46:52.077Z |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4393 +/- ##
============================================
- Coverage 87.49% 86.98% -0.51%
============================================
Files 2672 2684 +12
Lines 340400 344652 +4252
============================================
+ Hits 297819 299800 +1981
- Misses 42581 44852 +2271
🚀 New features to boost your workflow:
|
…nuation contract Review fixes for the having-range surface: - Float AVG operands now translate through the exact IEEE-754 value with operator-aware floor/ceiling (scaled_avg_operand + avg_bounds_for_operator) instead of f64-multiply-and-truncate, which lost sub-tick precision at the 10^19 scale and could move an inclusive bound by one tick — including the sign-dependent cases around zero. An equality bound between ticks is rejected loudly instead of silently becoming a point lookup on the truncated tick. - The continuation-by-bound story is stated honestly everywhere: a page cut at the limit continues past distinct aggregate values only; a cut inside a tie cannot be continued without a composite-key cursor (future capability), so callers size the limit above the widest expected tie. - The abci empty-axis mapping keeps its typed InvalidArgument but now describes both ranking and HAVING-range shapes, and the having dispatcher's comment no longer claims the path is unreachable (the empty-secondary prove failure is pinned by test). - Unexpected getDocuments result variants are reported by variant name only (shared result_variant_name helper) so error strings and logs cannot grow with — or leak — an untrusted response payload; the shared single-property path error now names both query surfaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lity-5b3db9' into claude/gracious-mahavira-673f37 # Conflicts: # packages/rs-drive/src/query/drive_document_ranked_query/path.rs
The canonical platform.proto comments still described the pre-PV14 behavior (every non-empty having rejected at every protocol version, having cannot combine with an aggregate ORDER BY). They now document the served single-clause COUNT/SUM/AVG range shape, the required ranked-axis index and limit, the absence of offset and cursor pagination with the distinct-value continuation and its tie limitation, and the unchanged rejection on v13 and earlier. Clients regenerated (only the Objective-C header embeds comments). The having-range route also gets its own OFFSET rejection message: the legacy one recommends `start_after` / `start_at`, which that surface rejects too, so it now explains continuation-by-bound instead. The legacy message stays byte-identical on every other route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lity-5b3db9' into claude/gracious-mahavira-673f37 # Conflicts: # packages/rs-drive-abci/src/query/document_query/v1/tests.rs
The having-range and ranked mode tables inherited "no where" wording from the base branch; on this branch a compound ranked index requires exactly one EQUAL pin per leading property, so the supported and rejected shape bullets now say that. Objective-C client regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ling `ORDER BY <agg>` read as an explicit OrderClause.aggregate target, which the wire rejects; the accepted spelling is the field name for SUM/AVG and the $count sentinel for COUNT(*), same as ranked mode. Objective-C client regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lity-5b3db9' into claude/gracious-mahavira-673f37
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs (1)
844-845: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the test to use the required
should_prefix.
compound_ranked_index_contract_parses_unless_its_prefix_aggregatesdoes not begin withshould. Rename it toshould_parse_compound_ranked_index_unless_its_prefix_aggregates.Proposed rename
-fn compound_ranked_index_contract_parses_unless_its_prefix_aggregates() { +fn should_parse_compound_ranked_index_unless_its_prefix_aggregates() {As per coding guidelines: “Unit and integration tests should … use descriptive names beginning with ‘should …’.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs` around lines 844 - 845, Rename the test function compound_ranked_index_contract_parses_unless_its_prefix_aggregates to should_parse_compound_ranked_index_unless_its_prefix_aggregates, preserving its test body and behavior.Source: Coding guidelines
packages/rs-drive/src/query/drive_document_ranked_query/tests.rs (1)
2295-2402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd pin-order and grouped-property-pin cases to
pinned_prefix.Every test in this module pins exactly one leading property, and the pin order already matches the index property order. Two behaviors of the new contract stay untested:
- Pin-order independence.
encode_equality_prefix_valuesre-orders pins by index property name. A regression there would swap prefix path segments and read a different subtree. The proof round trip cannot detect it, becauseclient_side_querycalls the same resolver. A fixture with two leading properties, with thewhereclauses supplied in reverse index order, would pin this.- Rejection of a pin on the grouped property. The module documentation in
mod.rsstates that awhereclause on the grouped (terminal) property is rejected. No test asserts the resulting error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/query/drive_document_ranked_query/tests.rs` around lines 2295 - 2402, Extend the pinned_prefix tests with a fixture whose two leading index properties are pinned through where clauses supplied in reverse index order, then verify reads still target the correct subtree and ranking results. Add a separate case that pins the grouped terminal property and assert it is rejected with the documented query syntax error, using the existing setup and error-matching patterns.packages/rs-drive/src/query/drive_document_having_query/tests.rs (2)
1944-1958: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error class for the unknown-prefix case.
Every other rejection test in this file matches the concrete
Error::Query(QuerySyntaxError::…)variant or the message text. This test accepts any error. A regression that turns the missing-prefix read into an internal/corruption error instead of a query-level rejection still passes here, and the doc comment claims the abci layer maps these to a client-visible rejection.♻️ Proposed tightening
let unknown = pin([9u8; 32]); - assert!( - run(&drive, &contract, &unknown, &[], false).is_err(), - "reading a never-written prefix value tree must error" - ); - assert!( - run(&drive, &contract, &unknown, &[], true).is_err(), - "proving a never-written prefix value tree must error" - ); + for prove in [false, true] { + let error = run(&drive, &contract, &unknown, &[], prove) + .expect_err("a never-written prefix value tree must error"); + // Pin the class the abci layer maps, so a change to an + // internal-error shape fails here rather than downstream. + assert!( + matches!(error, Error::GroveDB(_)), + "expected the grovedb path-not-found class (prove = {prove}), got {error:?}" + ); + } }Replace the matched variant with whichever class the executor actually returns today.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/query/drive_document_having_query/tests.rs` around lines 1944 - 1958, Update unknown_prefix_value_errors_rather_than_fabricating_an_empty_page to assert the concrete query-level error class returned by run for both read and proof paths, matching the existing Error::Query(QuerySyntaxError::…) or message-based assertions used in this file. Preserve the test’s coverage of both false and true modes while rejecting internal or corruption errors.
1393-1462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the three near-identical proof round-trip helpers.
client_side_queryandassert_proof_round_tripsnow exist in three copies (execution,identifier_group_keys,pinned_prefix), and the copies differ only in which inputs they thread through (where_clauses,order_by). The root-hash assertion is duplicated verbatim in all three. A change to the verifier signature or to the root-hash read must be applied three times.One generic helper that takes the mode inputs plus the document-type name would remove the duplication without weakening any assertion. Optional for this PR.
Also applies to: 1692-1765
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/query/drive_document_having_query/tests.rs` around lines 1393 - 1462, The proof round-trip helpers are duplicated across the execution, identifier_group_keys, and pinned_prefix tests. Consolidate client_side_query and assert_proof_round_trips into shared generic helpers that accept the varying mode inputs, where_clauses, order_by, and document-type name, while preserving proof verification, expected-entry comparison, and root-hash assertions.packages/rs-drive-abci/src/query/document_query/v1/mod.rs (1)
1481-1506: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the contract-fetch and document-type resolution block.
This 26-line block (identifier conversion,
get_contract_with_fetch_info_and_fee,DataContractNotFound,document_type_for_name) is now identical indispatch_sum_v1,dispatch_average_v1,dispatch_count_v1,dispatch_ranked_v1, anddispatch_having_v1. Each copy repeats the same three error messages, so a wording or fetch-policy change must be applied five times.A helper that returns the
Arc<DataContractFetchInfo>would remove most of it.document_typeborrows from the fetch info, so the helper should return the fetch info and let each caller resolve the document type from it. Optional for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/query/document_query/v1/mod.rs` around lines 1481 - 1506, Extract the duplicated contract lookup logic from the dispatch_sum_v1, dispatch_average_v1, dispatch_count_v1, dispatch_ranked_v1, and dispatch_having_v1 flows into a shared helper that converts the identifier, calls get_contract_with_fetch_info_and_fee, and applies the existing errors, returning the Arc<DataContractFetchInfo>. Update each caller to resolve document_type via document_type_for_name on the returned fetch info, preserving the current error messages and borrow behavior.packages/rs-drive-abci/src/query/document_query/v1/tests.rs (1)
3164-3211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the empty-axis proof mapping on the having path.
dispatch_having_v1routes drive errors throughempty_ranking_proof_rejection, and its comment states this branch is "genuinely reachable here" because the range prover has no empty-range shape. The ranked counterpart is no longer reachable —proving_an_empty_ranking_succeedsasserts an empty ranking now proves. So the broadened rejection message and the mapping call on the having path currently have no test in this suite.A test that proves a having request against a freshly registered contract, and asserts
QueryError::InvalidArgumentwith the "cannot be proved" wording, would pin the newly added branch.Do you want me to generate that test?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/query/document_query/v1/tests.rs` around lines 3164 - 3211, Add a test alongside a_having_request_with_prove_returns_a_proof that submits a prove-enabled having query against a freshly registered contract with no documents, then assert the result reports QueryError::InvalidArgument and its message contains “cannot be proved.” Exercise the dispatch_having_v1 empty-axis proof mapping while preserving the existing successful proof test for non-empty data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v3.rs`:
- Around line 19-21: Update the mixed-network rationale comment near the v3
query-version definitions to name the shipped tables accurately: state that
protocol versions 1–11 use DRIVE_ABCI_QUERY_VERSIONS_V0 and versions 12–13 use
DRIVE_ABCI_QUERY_VERSIONS_V1, both with helper 0 rejecting ranked and HAVING
queries; do not imply any shipped version selects DRIVE_ABCI_QUERY_VERSIONS_V2.
In
`@packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rs`:
- Line 23: Remove detect_having_mode from DRIVE_DOCUMENT_METHOD_VERSIONS_V2 and
keep that frozen table byte-for-byte unchanged; define the feature in a
later-version configuration or registry layer instead.
In `@packages/rs-sdk/src/platform/documents/document_having_entries.rs`:
- Around line 313-325: Replace the doc comment above
limit_is_required_and_capped_client_side with a concise description of the
client-side limit contract it verifies: limits must be within the inclusive
range 1..=100, and values outside that range are rejected rather than clamped.
---
Nitpick comments:
In `@packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 1481-1506: Extract the duplicated contract lookup logic from the
dispatch_sum_v1, dispatch_average_v1, dispatch_count_v1, dispatch_ranked_v1, and
dispatch_having_v1 flows into a shared helper that converts the identifier,
calls get_contract_with_fetch_info_and_fee, and applies the existing errors,
returning the Arc<DataContractFetchInfo>. Update each caller to resolve
document_type via document_type_for_name on the returned fetch info, preserving
the current error messages and borrow behavior.
In `@packages/rs-drive-abci/src/query/document_query/v1/tests.rs`:
- Around line 3164-3211: Add a test alongside
a_having_request_with_prove_returns_a_proof that submits a prove-enabled having
query against a freshly registered contract with no documents, then assert the
result reports QueryError::InvalidArgument and its message contains “cannot be
proved.” Exercise the dispatch_having_v1 empty-axis proof mapping while
preserving the existing successful proof test for non-empty data.
In
`@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rs`:
- Around line 844-845: Rename the test function
compound_ranked_index_contract_parses_unless_its_prefix_aggregates to
should_parse_compound_ranked_index_unless_its_prefix_aggregates, preserving its
test body and behavior.
In `@packages/rs-drive/src/query/drive_document_having_query/tests.rs`:
- Around line 1944-1958: Update
unknown_prefix_value_errors_rather_than_fabricating_an_empty_page to assert the
concrete query-level error class returned by run for both read and proof paths,
matching the existing Error::Query(QuerySyntaxError::…) or message-based
assertions used in this file. Preserve the test’s coverage of both false and
true modes while rejecting internal or corruption errors.
- Around line 1393-1462: The proof round-trip helpers are duplicated across the
execution, identifier_group_keys, and pinned_prefix tests. Consolidate
client_side_query and assert_proof_round_trips into shared generic helpers that
accept the varying mode inputs, where_clauses, order_by, and document-type name,
while preserving proof verification, expected-entry comparison, and root-hash
assertions.
In `@packages/rs-drive/src/query/drive_document_ranked_query/tests.rs`:
- Around line 2295-2402: Extend the pinned_prefix tests with a fixture whose two
leading index properties are pinned through where clauses supplied in reverse
index order, then verify reads still target the correct subtree and ranking
results. Add a separate case that pins the grouped terminal property and assert
it is rejected with the documented query syntax error, using the existing setup
and error-matching patterns.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5feee696-944f-4014-b836-a70e61b4b7e6
📒 Files selected for processing (55)
book/src/drive/document-ranked-trees.mdpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v3/mod.rspackages/rs-dpp/src/data_contract/document_type/index/mod.rspackages/rs-drive-abci/src/query/document_query/v1/compute_aggregate_mode_and_check_limit/mod.rspackages/rs-drive-abci/src/query/document_query/v1/compute_aggregate_mode_and_check_limit/v2/mod.rspackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rspackages/rs-drive-proof-verifier/src/lib.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/proof/document_having.rspackages/rs-drive-proof-verifier/src/proof/document_ranked.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/tests/ranked_index_e2e_tests.rspackages/rs-drive/src/fees/op.rspackages/rs-drive/src/query/drive_document_having_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_having_query/execute_range.rspackages/rs-drive/src/query/drive_document_having_query/executors.rspackages/rs-drive/src/query/drive_document_having_query/mod.rspackages/rs-drive/src/query/drive_document_having_query/mode_detection.rspackages/rs-drive/src/query/drive_document_having_query/tests.rspackages/rs-drive/src/query/drive_document_ranked_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_ranked_query/executors/mod.rspackages/rs-drive/src/query/drive_document_ranked_query/executors/top_k_no_proof.rspackages/rs-drive/src/query/drive_document_ranked_query/executors/top_k_proof.rspackages/rs-drive/src/query/drive_document_ranked_query/index_picker.rspackages/rs-drive/src/query/drive_document_ranked_query/mod.rspackages/rs-drive/src/query/drive_document_ranked_query/mode_detection.rspackages/rs-drive/src/query/drive_document_ranked_query/path.rspackages/rs-drive/src/query/drive_document_ranked_query/tests.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/src/verify/document_having/mod.rspackages/rs-drive/src/verify/document_having/verify_having_range_proof/mod.rspackages/rs-drive/src/verify/document_having/verify_having_range_proof/v0/mod.rspackages/rs-drive/src/verify/mod.rspackages/rs-drive/tests/supporting_files/contract/grades/grades-compound-ranked-contract.jsonpackages/rs-drive/tests/supporting_files/contract/grades/grades-ranked-contract.jsonpackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v4.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rspackages/rs-platform-version/src/version/v14.rspackages/rs-sdk/src/mock/requests.rspackages/rs-sdk/src/platform/documents/document_having_entries.rspackages/rs-sdk/src/platform/documents/document_query.rspackages/rs-sdk/src/platform/documents/document_ranked_entries.rspackages/rs-sdk/src/platform/documents/having_proof_helpers.rspackages/rs-sdk/src/platform/documents/mod.rspackages/rs-sdk/src/platform/documents/ranked_proof_helpers.rs
Comment on lines
+19
to
+21
| /// Same mixed-network rationale as the v1 → v2 flip: earlier protocol | ||
| /// versions keep the v2 table and keep rejecting the shape, so nodes | ||
| /// agree until the upgrade carries. The wire surface is unchanged — |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Map protocol version tables to their aggregate-routing helper version,
# and check that the struct-update expression does not trip clippy::needless_update.
set -euo pipefail
echo '--- which protocol version selects which query table ---'
rg -nP --type=rust -C4 'DRIVE_ABCI_QUERY_VERSIONS_V[0-9]' packages/rs-platform-version/src/version
echo '--- helper value per table ---'
rg -nP --type=rust -C3 'compute_aggregate_mode_and_check_limit\s*:' packages/rs-platform-version/src
echo '--- field count of DriveAbciQueryVersions (needless_update risk if only one field) ---'
ast-grep run --lang rust --pattern 'pub struct DriveAbciQueryVersions { $$$ }' packages/rs-platform-version/srcRepository: dashpay/platform
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("packages/rs-platform-version/src/version")
for path in sorted(root.glob("v[0-9]*.rs"), key=lambda p: int(re.search(r"v(\d+)", p.name).group(1))):
text = path.read_text()
match = re.search(r"query:\s*DRIVE_ABCI_QUERY_VERSIONS_V(\d+)", text)
if match:
print(f"{path.name}: query V{match.group(1)}")
PY
echo '--- query table helper values and struct fields ---'
sed -n '1,45p' packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v0.rs
sed -n '1,38p' packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs
sed -n '1,38p' packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v2.rs
sed -n '1,42p' packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v3.rs
sed -n '1,32p' packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs
echo '--- protocol-13 routing tests and v3 documentation ---'
rg -n -C5 'protocol_version_13_(does_not_route_ranked|still_rejects_having)|DRIVE_ABCI_QUERY_VERSIONS_V[0-9]|earlier protocol' packages --glob '*.rs'Repository: dashpay/platform
Length of output: 50372
Correct the mixed-network sentence to name the shipped tables. Protocol versions 1–11 select DRIVE_ABCI_QUERY_VERSIONS_V0, and versions 12–13 select DRIVE_ABCI_QUERY_VERSIONS_V1; both use helper 0 and reject ranked and HAVING queries. No shipped protocol version selects DRIVE_ABCI_QUERY_VERSIONS_V2.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v3.rs`
around lines 19 - 21, Update the mixed-network rationale comment near the v3
query-version definitions to name the shipped tables accurately: state that
protocol versions 1–11 use DRIVE_ABCI_QUERY_VERSIONS_V0 and versions 12–13 use
DRIVE_ABCI_QUERY_VERSIONS_V1, both with helper 0 rejecting ranked and HAVING
queries; do not imply any shipped version selects DRIVE_ABCI_QUERY_VERSIONS_V2.
Source: Coding guidelines
Comment on lines
+313
to
+325
| /// The generic FromProof guard in drive-proof-verifier must not be | ||
| /// reachable from the SDK path: this impl (on `DocumentQuery`) is | ||
| /// the one `fetch` resolves, and it runs the real verification. | ||
| #[test] | ||
| fn limit_is_required_and_capped_client_side() { | ||
| for limit in [0u32, 101] { | ||
| let query = hashtags_over_100().with_limit(limit); | ||
| assert!( | ||
| assert_having_shape(&query, platform_version()).is_err(), | ||
| "LIMIT {limit} is outside 1..=100 and must be rejected, not clamped" | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the doc comment on limit_is_required_and_capped_client_side.
The doc comment describes the generic FromProof guard. The test asserts that LIMIT 0 and LIMIT 101 are rejected client side. Replace the comment with the limit contract it actually tests.
📝 Proposed doc fix
- /// The generic FromProof guard in drive-proof-verifier must not be
- /// reachable from the SDK path: this impl (on `DocumentQuery`) is
- /// the one `fetch` resolves, and it runs the real verification.
+ /// The limit is a hard range, not a clamp: `0` (the unset
+ /// sentinel) and any value above `MAX_HAVING_LIMIT` must be
+ /// rejected client side, because the limit is echoed in the proof
+ /// envelope and re-checked by the verifier.
#[test]
fn limit_is_required_and_capped_client_side() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// The generic FromProof guard in drive-proof-verifier must not be | |
| /// reachable from the SDK path: this impl (on `DocumentQuery`) is | |
| /// the one `fetch` resolves, and it runs the real verification. | |
| #[test] | |
| fn limit_is_required_and_capped_client_side() { | |
| for limit in [0u32, 101] { | |
| let query = hashtags_over_100().with_limit(limit); | |
| assert!( | |
| assert_having_shape(&query, platform_version()).is_err(), | |
| "LIMIT {limit} is outside 1..=100 and must be rejected, not clamped" | |
| ); | |
| } | |
| } | |
| /// The limit is a hard range, not a clamp: `0` (the unset | |
| /// sentinel) and any value above `MAX_HAVING_LIMIT` must be | |
| /// rejected client side, because the limit is echoed in the proof | |
| /// envelope and re-checked by the verifier. | |
| #[test] | |
| fn limit_is_required_and_capped_client_side() { | |
| for limit in [0u32, 101] { | |
| let query = hashtags_over_100().with_limit(limit); | |
| assert!( | |
| assert_having_shape(&query, platform_version()).is_err(), | |
| "LIMIT {limit} is outside 1..=100 and must be rejected, not clamped" | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-sdk/src/platform/documents/document_having_entries.rs` around
lines 313 - 325, Replace the doc comment above
limit_is_required_and_capped_client_side with a concise description of the
client-side limit contract it verifies: limits must be within the inclusive
range 1..=100, and values outside that range are rejected rather than clamped.
…helper Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The compound-ranked implementation has two correctness blockers: contract validation applies the ranked item-key limit to leading prefix properties, and null equality pins for optional system-property prefixes do not reproduce the write path's empty key encoding. The remaining findings correct inaccurate public SDK and maintenance documentation.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v3/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v3/mod.rs:113-115: Apply the ranked key-length ceiling only to the terminal property
`validate_index_properties` calls this check for every property in the index, but only the terminal property's encoded value becomes the indexed primary's item key and is concatenated with the ranked secondary's 8- or 16-byte sort key. Leading property values remain ordinary GroveDB path keys and should retain the generic 63-character or 255-byte limit. As written, a valid average-ranked index such as `[region, class]`, with `region.maxLength = 60` and a short terminal `class`, is rejected because the leading prefix exceeds the 59-character ranked limit even though it is never part of the average secondary key. Restrict this check to the terminal property and add a boundary test with a long leading prefix.
In `packages/rs-drive/src/query/drive_document_ranked_query/index_picker.rs`:
- [BLOCKING] packages/rs-drive/src/query/drive_document_ranked_query/index_picker.rs:256-264: Encode null prefix pins as empty path segments
Optional system properties such as `$updatedAt`, `$transferredAt`, and `$creatorId` can legally be leading properties of a compound ranked index. When one is absent, the write walker turns `get_raw_for_document_type(...).unwrap_or_default()` into an empty path segment. This query path instead sends `Value::Null` through `serialize_value_for_key`; the system-property branches attempt identifier or integer conversion and fail before reaching the user-property null encoding. Consequently, a valid query such as `WHERE $updatedAt = null GROUP BY ...` cannot address the empty-key prefix subtree populated by the write path. Handle null before system-property serialization so server execution and SDK proof verification reconstruct the stored path exactly, and cover this case in ranked and HAVING proof tests.
In `packages/rs-sdk/src/platform/documents/document_query.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/documents/document_query.rs:293-299: Document the PV14 HAVING support on the public builder
The public `DocumentQuery::with_having` documentation still states that every non-empty value is rejected and that the builder only exists ahead of server support. This contradicts both the updated `having` field documentation and the PV14 having-range API introduced by this PR, so SDK users are told that the new supported request is unusable.
In `packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v3.rs`:
- [NITPICK] packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v3.rs:18-20: Name the query tables actually selected before PV14
The mixed-network rationale says earlier protocol versions keep the V2 query table, but no shipped protocol version selects it: protocol versions 1–11 select `DRIVE_ABCI_QUERY_VERSIONS_V0`, and versions 12–13 select `DRIVE_ABCI_QUERY_VERSIONS_V1`. Both use aggregate helper version 0 and reject ranked and HAVING requests, so the rationale is sound but the table attribution is inaccurate.
In `packages/rs-sdk/src/platform/documents/document_having_entries.rs`:
- [NITPICK] packages/rs-sdk/src/platform/documents/document_having_entries.rs:313-315: Describe the limit contract tested by the HAVING test
The comment discusses `FromProof` implementation resolution, but the test directly calls `assert_having_shape` and only verifies rejection of limits 0 and 101. Describe the inclusive client-side limit range and rejection behavior that the test actually exercises.
Comment on lines
+256
to
+264
| document_type | ||
| .serialize_value_for_key(&property.name, value, platform_version) | ||
| .map_err(|e| { | ||
| Error::Query(QuerySyntaxError::InvalidParameter(format!( | ||
| "the equality pin on `{}` does not encode as that property's \ | ||
| index key: {e}", | ||
| property.name | ||
| ))) | ||
| }) |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Encode null prefix pins as empty path segments
Optional system properties such as $updatedAt, $transferredAt, and $creatorId can legally be leading properties of a compound ranked index. When one is absent, the write walker turns get_raw_for_document_type(...).unwrap_or_default() into an empty path segment. This query path instead sends Value::Null through serialize_value_for_key; the system-property branches attempt identifier or integer conversion and fail before reaching the user-property null encoding. Consequently, a valid query such as WHERE $updatedAt = null GROUP BY ... cannot address the empty-key prefix subtree populated by the write path. Handle null before system-property serialization so server execution and SDK proof verification reconstruct the stored path exactly, and cover this case in ranked and HAVING proof tests.
Suggested change
| document_type | |
| .serialize_value_for_key(&property.name, value, platform_version) | |
| .map_err(|e| { | |
| Error::Query(QuerySyntaxError::InvalidParameter(format!( | |
| "the equality pin on `{}` does not encode as that property's \ | |
| index key: {e}", | |
| property.name | |
| ))) | |
| }) | |
| if value.is_null() { | |
| Ok(Vec::new()) | |
| } else { | |
| document_type | |
| .serialize_value_for_key(&property.name, value, platform_version) | |
| .map_err(|e| { | |
| Error::Query(QuerySyntaxError::InvalidParameter(format!( | |
| "the equality pin on `{}` does not encode as that property's \ | |
| index key: {e}", | |
| property.name | |
| ))) | |
| }) | |
| } |
source: ['codex']
Comment on lines
+18
to
+20
| /// | ||
| /// Same mixed-network rationale as the v1 → v2 flip: earlier protocol | ||
| /// versions keep the v2 table and keep rejecting the shape, so nodes |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Name the query tables actually selected before PV14
The mixed-network rationale says earlier protocol versions keep the V2 query table, but no shipped protocol version selects it: protocol versions 1–11 select DRIVE_ABCI_QUERY_VERSIONS_V0, and versions 12–13 select DRIVE_ABCI_QUERY_VERSIONS_V1. Both use aggregate helper version 0 and reject ranked and HAVING requests, so the rationale is sound but the table attribution is inaccurate.
Suggested change
| /// | |
| /// Same mixed-network rationale as the v1 → v2 flip: earlier protocol | |
| /// versions keep the v2 table and keep rejecting the shape, so nodes | |
| /// Mixed-network safety comes from the shipped tables: protocol versions | |
| /// 1–11 select `DRIVE_ABCI_QUERY_VERSIONS_V0`, and versions 12–13 select | |
| /// `DRIVE_ABCI_QUERY_VERSIONS_V1`. Both use helper version 0 and reject | |
| /// ranked and `HAVING` shapes, so nodes agree until the PV14 upgrade carries. | |
| /// The wire surface is unchanged — |
source: ['coderabbit']
Comment on lines
+313
to
+315
| /// The generic FromProof guard in drive-proof-verifier must not be | ||
| /// reachable from the SDK path: this impl (on `DocumentQuery`) is | ||
| /// the one `fetch` resolves, and it runs the real verification. |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Describe the limit contract tested by the HAVING test
The comment discusses FromProof implementation resolution, but the test directly calls assert_having_shape and only verifies rejection of limits 0 and 101. Describe the inclusive client-side limit range and rejection behavior that the test actually exercises.
Suggested change
| /// The generic FromProof guard in drive-proof-verifier must not be | |
| /// reachable from the SDK path: this impl (on `DocumentQuery`) is | |
| /// the one `fetch` resolves, and it runs the real verification. | |
| /// HAVING limits must be within the inclusive range `1..=100`; | |
| /// values outside that range are rejected client side rather than | |
| /// clamped. |
source: ['coderabbit']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Ranked aggregate index flags (
rankedCountable/rankedSummable/rankedAverageable) were restricted to single-property indexes. This lifts the restriction: a compound index like[identityId, class]can now declare ranked flags with per-prefix semantics — the ranked flags land on the index's terminal property-name level, so each prefix value (eachidentityId) gets its own ordered secondary ranking only that prefix'sclassgroups. There is deliberately no global cross-prefix ordering.Both query surfaces gain equality-prefix routing to serve it:
WHERE identityId = X GROUP BY class HAVING AVG(grade) > 80 LIMIT nWHERE identityId = X GROUP BY class ORDER BY AVG(grade) DESC LIMIT 5Everything is gated at protocol version 14 (unreleased): the ranked contract grammar is meta-schema v3 / parser generation 3, and both query surfaces route only through
DRIVE_ABCI_QUERY_VERSIONS_V3— all PV14-only and unreleased, so the relaxation is edited in place in those modules. PV13 keeps rejecting every new shape (pinned by tests). No version slots were added or renumbered, so the v14 gate tests are unchanged.Stacked on #4384 (the having-range surface) — that PR's three commits are included here; merge it first (or merge this one into it).
What was done?
rs-dpp
Index::try_from(generation-3 grammar, PV14-only). Flag-dependency rules (ranked*requires the matchingrange*), the unique-index rejection and thenullSearchable: falserejection apply to compound indexes unchanged.validate_no_ranked_prefix_overlapintry_from_schema::common, gated on the generation'sadmit_ranked): a compound ranked index whose full leading prefix also terminates a countable/summable index is refused — its aggregating value trees would demand theNonCounted/NotSummedshell grovedb structurally rejects around indexed trees (theINDEXED_INNER_UNWRAPPABLEfail-closed guard in drive remains as the backstop). Only the exactn-1prefix conflicts; shorter-prefix aggregating indexes and extensions past the ranked terminal are fine and covered by tests. The check runs on validating and non-validating parses alike, so check_tx/cache-warm paths cannot smuggle the shape past it.Write path (rs-drive)
No walker changes were needed: the v2 document index walkers are arity-generic and already emit the indexed tree type at a compound index's terminal level, and the pinned grovedb rev explicitly supports creating an indexed primary and populating it in the same batch (only overwrite-with-descendants is rejected) — so the per-prefix terminal trees, created lazily by the first document insert under each prefix, maintain their secondaries through the ordinary write path. This is verified end to end by the new integration tests (documents inserted through the real write path, per-prefix reads and proofs against the live root hash). Stale comments claiming grovedb rejects same-batch create+populate were corrected.
Query surfaces (rs-drive)
detect_ranked_mode_v0/detect_having_mode_v0, both PV14-only-reachable):whereclauses are now accepted as equality pins — each clause must be==on a distinct property (sharedequality_pins_from_where_clauses).INon a prefix is rejected loudly with its own not-yet message (one walk per element layers on future multi-INbranching); range operators and duplicate pins are rejected; multi-fieldgroup_bystays rejected, with the message steering to the pinned form.resolve_ranked_query_for_mode/resolve_having_query_for_mode), used by the server executors AND the SDK proof helpers: the covering-index picker matchespins + trailing group_byagainst the index's properties (exact cover — partial pins match nothing), and the pins are encoded into prefix path segments withDocumentType::serialize_value_for_key— the same encoding the write path used to key the prefix value trees.indexed_property_name_tree_path_for_index) extended with the prefix-value segments; prover and verifier both go through it (and through the shared resolver), so they cannot drift on which subtree a proof is about. Arity mismatches fail closed with a typed error.abci
No routing code changes: the v2
compute_aggregate_mode_and_check_limitalready routes any grouped single-clause having / aggregate-ordered shape regardless ofwhere, and both dispatchers forwardwhere_clausesto drive untouched — drive owns the grammar. PV13 (query table v1 / helper v0) never routes either surface, so it rejects all new shapes before any contract fetch.Clients (rs-sdk / rs-drive-proof-verifier)
The SDK's ranked and having proof helpers now call drive's shared resolvers instead of hand-building the query structs, so pinned requests verify against the same prefixed path the prover used. Docs updated (request-shape sections, book chapter
document-ranked-trees.md).How Has This Been Tested?
ranked*-requires-range*enforced on compound indexes.grades-compound-ranked-contract.jsonfixture ([identityId asc, class asc], averageable/rangeAverageable/rankedAverageable ongrade), all through the real write path with proof round-trips against the live grovedb root hash:>), fractional average just above (80, 81 → 80.5 included), byte-exact string class keys, ascending and descending walks, proof round-trips for each — and isolation: identity Y's qualifying classes never leak into X's result, including a class name collision (mathfails X's bound at avg 80 but passes Y's at 92.5).INprefix (not-yet message), wrong-property pin, duplicate pins, range-operator pins, unknown prefix value (error, not empty page).ResultData.ranked,skippedunset, isolation asserted) + proved variant; unpinned two-fieldgroup_bystill rejected (flipped from the old single-property pin, now asserting the steering message); the pinned shape still rejected wholesale at protocol version 13.cargo check --workspace --all-targets✓,drivelib (3361 tests) ✓,drive-abcilib ✓,dpp(3873) ✓,drive-proof-verifier✓,dash-sdklib (208) ✓,platform-version(16) ✓, clippy clean on drive/dpp,cargo fmt --all.Breaking Changes
None for deployed networks — everything activates at protocol version 14, which is unreleased. Contracts that were invalid at PV14-in-development (compound ranked) become valid; the newly-rejected prefix-overlap shape was never registrable (it was covered by the broader single-property rejection).
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
GROUP BY … HAVINGrange queries for COUNT, SUM, and AVG with bounded results, ordering, limits, and proof support.Bug Fixes