feat(drive): boolean HAVING range queries on ranked index axes - #4384
Open
QuantumExplorer wants to merge 6 commits into
Open
feat(drive): boolean HAVING range queries on ranked index axes#4384QuantumExplorer wants to merge 6 commits into
QuantumExplorer wants to merge 6 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>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds protocol-versioned ChangesHAVING range query support
Estimated code review effort: 5 (Critical) | ~90 minutes Mergeability Score: 🔵 Low · up to The new HAVING query behavior is mergeable, but the protocol documentation should clarify the valid ORDER BY keys for aggregate queries to avoid client integration errors; this is a bounded follow-up for the owner. Sequence Diagram(s)sequenceDiagram
participant SDK
participant ABCI
participant Drive
participant GroveDB
participant Verifier
SDK->>ABCI: Submit grouped aggregate with HAVING range
ABCI->>Drive: Dispatch HAVING request
Drive->>Drive: Validate mode and resolve ranked index
Drive->>GroveDB: Execute range or generate proof
GroveDB-->>Drive: Return entries or range proof
Drive-->>ABCI: Return HAVING response
ABCI-->>SDK: Return entries and metadata
SDK->>Verifier: Verify HAVING proof
Verifier-->>SDK: Return verified entries
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
✅ Final review complete — no blockers (commit 39e14e5) |
![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: 4
🧹 Nitpick comments (2)
packages/rs-drive-abci/src/query/document_query/v1/mod.rs (1)
1463-1488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated contract and document-type resolution.
These 26 lines are the fifth copy of the same block.
dispatch_count_v1,dispatch_sum_v1,dispatch_average_v1, anddispatch_ranked_v1contain identical logic, including the two error strings. A future change to the "document type not found" or "contract not found" wording must now be applied in five places.Extract one private helper that returns
(Arc<DataContractFetchInfo>, Identifier)and call it from each dispatcher.🤖 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 1463 - 1488, The repeated contract and document-type resolution should be centralized in one private helper returning (Arc<DataContractFetchInfo>, Identifier). Extract the shared validation and lookup logic from the current block, including both existing error messages, then update dispatch_count_v1, dispatch_sum_v1, dispatch_average_v1, dispatch_ranked_v1, and the current dispatcher to call that helper and use its returned contract information and identifier.packages/rs-drive/src/query/drive_document_having_query/tests.rs (1)
296-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd grammar tests for the remaining rejection branches.
The suite covers most of the v0 grammar. Three rejection branches in
detect_having_mode_v0have no test: a non-emptywhere_clauses, more than oneorder_byclause, and an empty select field forSUM/AVG. The unknown-method-version arm ofdetect_having_modeis also untested. Each branch is a one-call test, and each guards a documented request-shape contract.🤖 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 296 - 366, Add focused one-call rejection tests alongside limit_is_required_and_capped and offset_and_start_at_are_rejected for non-empty where_clauses, multiple order_by clauses, and empty select fields with SUM or AVG, asserting detect_having_mode_v0 returns an error. Also add a test for detect_having_mode with an unsupported method version, asserting the unknown-version error path.
🤖 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-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 1513-1524: Update the error-handling block in the having-query
proof path to remove the “should be unreachable” empty-tree comment and replace
the ranking-specific empty_ranking_proof_rejection handling with
having-query-appropriate empty-secondary rejection wording. Preserve the
existing conversion of unrelated errors through Err(e.into()) and align the
message with an_empty_match_set_reads_empty_and_proves_empty.
In `@packages/rs-drive-proof-verifier/src/proof/document_having.rs`:
- Around line 126-136: Update the `other` arm in the response decoding logic to
report only the received result variant, not the full `other` payload. Replace
`{other:?}` with a variant-name representation that avoids serializing document
contents while preserving the existing routing guidance and error type.
Apply the same fix in
`@packages/rs-drive/src/query/drive_document_ranked_query/path.rs` around lines 32
- 40: The shared helper emits ranked-only wording when called by having-range
queries.
In `@packages/rs-drive/src/query/drive_document_having_query/mode_detection.rs`:
- Around line 497-527: Update avg_operand to reject finite float operands whose
value multiplied by AVG_FIXED_POINT_SCALE is not an exact integer, instead of
silently truncating through the f64-to-i128 conversion. Return the existing
InvalidParameter error for inexact scaled values, while preserving accepted
exact values and the current range validation.
In
`@packages/rs-drive/src/verify/document_having/verify_having_range_proof/mod.rs`:
- Around line 37-50: Update the platform-version verification definitions to add
a dedicated verify.document_having group and move verify_having_range_proof out
of document_ranked into it. In the dispatcher using platform_version, update the
match to read document_having.verify_having_range_proof while preserving the
existing version handling and error metadata.
---
Nitpick comments:
In `@packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 1463-1488: The repeated contract and document-type resolution
should be centralized in one private helper returning
(Arc<DataContractFetchInfo>, Identifier). Extract the shared validation and
lookup logic from the current block, including both existing error messages,
then update dispatch_count_v1, dispatch_sum_v1, dispatch_average_v1,
dispatch_ranked_v1, and the current dispatcher to call that helper and use its
returned contract information and identifier.
In `@packages/rs-drive/src/query/drive_document_having_query/tests.rs`:
- Around line 296-366: Add focused one-call rejection tests alongside
limit_is_required_and_capped and offset_and_start_at_are_rejected for non-empty
where_clauses, multiple order_by clauses, and empty select fields with SUM or
AVG, asserting detect_having_mode_v0 returns an error. Also add a test for
detect_having_mode with an unsupported method version, asserting the
unknown-version error path.
🪄 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: 4d767716-5f83-4a98-9a0f-454a3e7a6e92
📒 Files selected for processing (35)
packages/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/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/path.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-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/having_proof_helpers.rspackages/rs-sdk/src/platform/documents/mod.rs
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>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4384 +/- ##
============================================
- Coverage 87.49% 86.28% -1.22%
============================================
Files 2672 2685 +13
Lines 340400 346465 +6065
============================================
+ Hits 297819 298933 +1114
- Misses 42581 47532 +4951
🚀 New features to boost your workflow:
|
thepastaclaw
suggested changes
Aug 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The HAVING range implementation follows the indexed-axis proof architecture, but two core API semantics are incorrect: floating-point AVG predicates can resolve to the wrong fixed-point range, and the documented bound-only pagination cannot continue through aggregate ties. The canonical protobuf documentation and several new error paths also need smaller corrections.
Source: reviewer backend gpt-5.6-sol; final verifier backend 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 | 🟡 3 suggestion(s)
1 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-drive/src/query/drive_document_having_query/mode_detection.rs`:
- [BLOCKING] packages/rs-drive/src/query/drive_document_having_query/mode_detection.rs:517-526: Translate floating AVG bounds without truncating
`avg_operand` casts the scaled floating threshold to `i128` before `bounds_for_operator` applies inclusive or exclusive semantics. The cast truncates toward zero, so it discards information needed to choose the correct integer boundary. If the scaled threshold is `0.5`, `AVG >= threshold` must start at `1`, but this code starts at `0`; if it is `-0.5`, `AVG > threshold` must start at `0`, but this code starts at `1`. A non-integral equality threshold also cannot be converted into an equality lookup on the truncated integer. The conversion must compute operator-specific floor or ceiling bounds from the exact IEEE-754 value, or reject floating operands that cannot be represented with defined fixed-point semantics. A simple `scaled.fract()` check is insufficient at the current 10^19 scale because large `f64` products have already lost sub-integer precision.
In `packages/rs-drive/src/query/drive_document_having_query/mod.rs`:
- [BLOCKING] packages/rs-drive/src/query/drive_document_having_query/mod.rs:254-256: Bound-only pagination loses groups tied at the page boundary
The secondary is ordered by `(sort_key || group_key)`, but callers can adjust only the aggregate-value bound. If a page ends inside a tie, moving the threshold past the last aggregate excludes every remaining group with that value, while keeping the threshold returns the same first page. For example, with 101 groups at the same count and `LIMIT 100`, the final group cannot be retrieved; if more than `MAX_HAVING_LIMIT` groups tie, increasing the limit cannot help either. This contradicts the PR and SDK contract that callers can paginate by tightening the bound. Add a continuation cursor containing the composite secondary key/group key, or remove the pagination claim and explicitly document that result sets cut inside a tie cannot be fully enumerated.
In `packages/dapi-grpc/protos/platform/v0/platform.proto`:
- [SUGGESTION] packages/dapi-grpc/protos/platform/v0/platform.proto:706-711: Update the canonical wire documentation for the new HAVING grammar
The protobuf documentation still says HAVING cannot be combined with aggregate `ORDER BY`, even though PV14 accepts that clause to select the range-walk direction. The request documentation at lines 852-857, 886-888, and 1087-1092 also says every non-empty HAVING is rejected at every protocol version. These comments feed generated API documentation and now contradict the server behavior introduced by this PR. Document the supported PV14 single-clause range shape, its index and pagination restrictions, and the continued rejection on PV13 and earlier.
In `packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/document_query/v1/mod.rs:1513-1524: Use HAVING-specific empty-axis proof diagnostics
The comment says the empty-secondary failure should be unreachable, but `an_empty_match_set_reads_empty_and_proves_empty` explicitly verifies that the HAVING range prover still returns `Cannot create proof for empty tree` for a completely empty secondary. The error is then mapped through `empty_ranking_proof_rejection`, whose user-facing text calls the request a ranking. Keep the typed `InvalidArgument` mapping and retry guidance, but describe this as an empty HAVING axis/range and remove the inaccurate unreachable-path explanation.
In `packages/rs-drive-proof-verifier/src/proof/document_having.rs`:
- [SUGGESTION] packages/rs-drive-proof-verifier/src/proof/document_having.rs:126-136: Keep unexpected HAVING response diagnostics payload-independent
Formatting `{other:?}` serializes the entire unexpected response into the error, including every document byte array when a node returns `ResultData.documents`. This makes diagnostic allocation and logging proportional to an untrusted response payload and can expose returned document contents in logs. Report only whether the result was absent, proof, or the received `ResultData` variant. The shared path helper in `drive_document_ranked_query/path.rs` should likewise use surface-neutral wording because it is now called by both ranked and HAVING queries rather than saying only that “ranked queries” require a single-property index.
6 tasks
…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>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head revalidation confirms that the floating-point AVG translation, tie-boundary contract, empty-axis diagnostics, and payload-independent response diagnostics have been corrected. One prior documentation suggestion remains valid, and the HAVING-specific offset rejection still directs callers to unsupported cursor pagination; no blocking issue remains.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(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/dapi-grpc/protos/platform/v0/platform.proto`:
- [SUGGESTION] packages/dapi-grpc/protos/platform/v0/platform.proto:706-711: Update the canonical wire documentation for the new HAVING grammar
The canonical protobuf comments still contradict the PV14 behavior added by this PR. This section says HAVING cannot be combined with aggregate `ORDER BY`, although `detect_having_mode_v0` accepts that combination to choose the range-walk direction. The request documentation at lines 852-857, 886-888, and 1087-1092 also says every non-empty HAVING is rejected at every protocol version. Update the generated-API comments to document the supported PV14 single-clause COUNT/SUM/AVG range shape, the required ranked-axis index and limit, the lack of offset or start cursors, the inability to continue a page cut inside an aggregate tie, and the continued rejection on PV13 and earlier.
In `packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/document_query/v1/mod.rs:419-423: Do not recommend an unsupported cursor for HAVING offsets
A PV14 HAVING request carrying `offset` is first routed to `RoutingDecision::HavingRange` and then rejected by this shared gate. The returned message directs the caller to `start_after` or `start_at`, but `detect_having_mode_v0` rejects both cursor forms because document-ID cursors do not address the aggregate-sorted secondary. Preserve the load-bearing legacy message for other non-ranked routes, but return a HAVING-specific message for `RoutingDecision::HavingRange` explaining that this surface has neither offset nor cursor pagination and can continue only by tightening past a distinct aggregate value, subject to the documented tie limitation.
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>
Member
Author
|
Both final-validation suggestions addressed in 0c80416:
🤖 Addressed by Claude Code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dapi-grpc/protos/platform/v0/platform.proto`:
- Around line 706-717: The having-range documentation in GetDocumentsRequestV1
must use accepted aggregate order-key spellings: document ORDER BY f for SUM(f)
and AVG(f), and ORDER BY $count for COUNT(*), instead of ORDER BY <agg>. Update
the corresponding generated documentation in
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h at lines
2651-2662 after changing packages/dapi-grpc/protos/platform/v0/platform.proto at
lines 706-717; no other sites require changes.
🪄 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: 7557446b-7838-478f-a47a-dc8197cb35ba
📒 Files selected for processing (4)
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive-abci/src/query/document_query/v1/tests.rs
…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>
thepastaclaw
approved these changes
Aug 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head 39e14e5, the HAVING-range implementation has no remaining in-scope blocking issues, suggestions, or nitpicks from the supplied Codex and CodeRabbit evidence. Both prior verified findings are fixed: the canonical wire documentation now describes the PV14 grammar and limitations accurately, and HAVING offsets receive surface-specific continuation guidance.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
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
Grouped aggregate queries could rank groups by their aggregate (
GROUP BY p ORDER BY <agg> LIMIT n, the PV14 ranked surface) but could not filter groups by an aggregate bound — e.g. "hashtags with more than 100 posts". Any non-emptyhavingon the wire was rejected at every protocol version.What was done?
Implements a boolean-
HAVINGrange query surface end to end, served as a value-bounded range read of the covering ranked index's axis secondary (the same grovedb trees the ranked top-k surface walks), so a having read isO(log n + k)with a completeness-attesting proof.Query shape (v1 grammar,
detect_having_mode_v0in drive):SELECT COUNT(*)/SUM(f)/AVG(f) ... GROUP BY p HAVING <the selected aggregate> <op> <value> LIMIT n=,>,>=,<,<=and the fourBETWEENvariants translate to inclusive per-axis bounds (!=/INrejected — non-contiguous)order_byoptional (may name the selected aggregate to flip walk direction); limit required,1..=100; no offset/start_at — pagination is "tighten the bound past the last value seen"ANDand cross-aggregate predicates keep thenot_yet_implementedcontractLayer by layer:
query/drive_document_having_query/module (versioned grammar, bounds translation, executors over grovedb'sindexed_*_range/prove_indexed_*_query) andverify/document_having/verifier. Prover and verifier share one bounds→Merk-query translation (AxisRangeBounds::merk_query) and one path builder, so they cannot drift.compute_aggregate_mode_and_check_limitv2 routes a grouped single-clausehavingto the newdispatch_having_v1. The response reuses the existingRankedEntrieswire message withskippedunset — zero proto changes, zero client regeneration.DRIVE_ABCI_QUERY_VERSIONS_V3; drive slotsdetect_having_mode/verify_having_range_proofexist at 0 in all tables (same dormancy pattern asdetect_ranked_mode). v13 and earlier keep rejecting the shape, so mixed-version networks agree across the upgrade.DocumentHavingEntrieswithFromProof/Fetch— client-side verification binds the proof's reconstructed root hash to the quorum-signed app hash, and Merk range boundaries attest completeness (an in-range group the node omitted fails verification).No grovedb changes — the pinned revision already ships
indexed_{count,sum,avg}_rangeandprove/verify_indexed_*_query.How Has This Been Tested?
103 tests across the stack, all passing:
cargo check --workspace --all-targetsclean, clippy clean on all touched crates,cargo fmtapplied. rs-drive additionally checked--no-default-featuresand--all-features --all-targets.Known limitation: proving against a completely empty axis secondary (fresh contract) fails at the grovedb layer ("Cannot create proof for empty tree"); drive-abci maps this to a clear InvalidArgument suggesting
prove=false, the same class as the existing ranked-surface limitation. Populated-tree absence proofs work fine.Breaking Changes
None for deployed networks: the routing gate lives in PV14, which is unreleased. PV13 and earlier nodes keep rejecting every non-empty
havingexactly as before.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
HAVINGrange forCOUNT,SUM, orAVG.HAVINGresults, including empty responses.Documentation
Bug Fixes