feat(sdk): implement document sum/average aggregation FFI (DOC-13/14)#3935
feat(sdk): implement document sum/average aggregation FFI (DOC-13/14)#3935QuantumExplorer wants to merge 5 commits into
Conversation
`dash_sdk_document_sum` and `dash_sdk_document_average` were stubs
returning NotImplemented, citing "blocked on grovedb PR 670". That
block is stale — the pinned grovedb (fc81498) already exposes
verify_aggregate_sum_query / verify_aggregate_count_and_sum_query, and
the rs-drive sum/average query + verifiers and the rs-sdk
DocumentSplitSums / DocumentSplitAverages `Fetch` impls are complete.
Implement both FFI functions as mirrors of the working
`dash_sdk_document_count`: build the DocumentQuery from the JSON
params, select `SelectProjection::sum(field)` / `avg(field)`, fetch via
DocumentSplitSums / DocumentSplitAverages, fold with the overflow-safe
`try_into_flat_map`, and serialize:
- sum -> {"sums":{"<hex-key>":<i64>}}
- average -> {"averages":{"<hex-key>":{"count":<u64>,"sum":<i64>}}}
Empty `sum_property` is rejected with InvalidParameter (the rs-sdk
select assertions require a non-empty field). Three where/order/limit
parsing helpers in count.rs are promoted to `pub(super)` and reused to
keep cross-mode parsing identical. C ABI unchanged (signatures already
existed); only doc comments changed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "blocked on grovedb PR 670" note was stale. With dash_sdk_document_sum / _average now implemented, DOC-13/14 are FFI-available (no app UI yet), so reclassify 🚫 → 🔌 and record the result shapes + the summable-index fixture requirement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 5 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTwo previously stubbed FFI endpoints, ChangesDocument Aggregation FFI Implementation
Sequence Diagram(s)sequenceDiagram
participant iOS as iOS Caller
participant FFI as dash_sdk_document_sum / dash_sdk_document_average
participant Helpers as count.rs (build_base_query, decode_ffi_limit, parse_group_by_json)
participant SDK as DocumentSplitSums / DocumentSplitAverages
iOS->>FFI: FFI call (sum_property, group_by_json, limit, ...)
FFI->>FFI: null-pointer validation
FFI->>Helpers: build_base_query(...)
Helpers-->>FFI: DocumentQuery
FFI->>FFI: validate_aggregation_property(sum_property)
FFI->>Helpers: decode_ffi_limit(limit)
FFI->>Helpers: parse_group_by_json(group_by_json)
FFI->>SDK: fetch(query)
SDK-->>FFI: split results map
FFI->>FFI: flatten → hex-encode keys → serialize JSON → CString
FFI-->>iOS: DashSDKResult (heap-allocated JSON CString)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit d723757) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean FFI implementation of dash_sdk_document_sum / dash_sdk_document_average mirroring the existing dash_sdk_document_count pattern: shared parameter-parsing helpers are promoted to pub(super) and reused verbatim, null-pointer guards and UTF-8 validation are consistent, and the (count, sum) result shape is correctly handled via try_into_flat_map. No blocking correctness, safety, FFI ABI, or consensus issues found. The only worthwhile suggestions are a typed-SDK-error preservation tweak and a small unit-test gap for the new entry points.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 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-sdk-ffi/src/document/queries/sum.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/document/queries/sum.rs:138-144: Preserve typed SDK errors from the sum/average fetch instead of collapsing into InternalError
DocumentSplitSums::fetch and DocumentSplitAverages::fetch return dash_sdk::Error. packages/rs-sdk-ffi/src/error.rs:102-188 already classifies that variant into NetworkError (DapiClientError/NoAvailableAddressesToRetry), Timeout (TimeoutReached), DriveInternalError, NotFound, ProtocolError, etc. The current `map_err(|e| FFIError::InternalError(format!("Failed to fetch sum: {}", e)))` (sum.rs:140 and average.rs:157) discards that classification, so every transient transport failure or proof error surfaces to Swift as code 99 (InternalError) instead of the typed code that Swift can act on. Using `.map_err(FFIError::from)?` lets the existing `From<dash_sdk::Error> for FFIError` (#[from] on SDKError) feed the existing classifier. Note the existing count entry point has the same shortcut — aligning the three call sites together (or in a small follow-up) avoids future divergence.
- [SUGGESTION] packages/rs-sdk-ffi/src/document/queries/sum.rs:112-117: Add small unit tests for the new sum/average FFI surface (empty sum_property, JSON shape, hex keys)
The PR turns two stable NotImplemented stubs into a real C ABI surface but adds no rs-sdk-ffi tests for the new code. The empty-`sum_property` rejection (sum.rs:112-117 and average.rs:129-134) is pure FFI logic — no SDK / runtime / contract needed — and is the only new validation step; a unit test that constructs an empty NUL-terminated string and asserts FFIError::InvalidParameter (mirroring the style of the existing decode_ffi_limit tests at the bottom of count.rs) would lock the contract in cheaply. The DocumentSumResult / DocumentAverageResult JSON shapes (`{"sums":{...}}`, `{"averages":{"<hex>":{"count":..,"sum":..}}}`) and the hex-key encoding are also pure data-shaping and equally cheap to round-trip. End-to-end proof-verified coverage via the planned `summable` index fixture is reasonably deferred; the FFI-shape tests are not.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3935 +/- ##
=============================================
- Coverage 72.78% 52.54% -20.25%
=============================================
Files 22 11 -11
Lines 3054 1707 -1347
=============================================
- Hits 2223 897 -1326
+ Misses 831 810 -21
🚀 New features to boost your workflow:
|
Address thepastaclaw review on the sum/average aggregation FFI: - Route the `.fetch(...)` errors through `FFIError::from` (the existing `SDKError(#[from] dash_sdk::Error)` classifier) instead of collapsing to InternalError, so Swift sees NetworkError/Timeout/etc. Applied to sum, average, AND count (aligning all three to avoid divergence). The `try_into_flat_map` overflow error stays InternalError (a data error, not transport). - Extract `validate_aggregation_property` (pure, testable) for the empty-field guard, used by both sum and average, and add unit tests: empty-field rejection + DocumentSumResult / DocumentAverageResult JSON shape round-trips (mirrors count.rs's decode_ffi_limit test style). - Drop the redundant "Same contract as […]" Safety prose, keeping the per-parameter bullets (cbindgen headers don't follow intra-doc links). C ABI unchanged. cargo build/test/clippy/fmt -p rs-sdk-ffi all clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/swift-sdk/SwiftExampleApp/TEST_PLAN.md`:
- Around line 218-219: The TEST_PLAN.md document has outdated cross-references
that contradict the updated DOC-13 and DOC-14 rows. Update line 395 in Appendix
A (Document RPC section) to remove the statement that "SUM/AVG are
upstream-blocked (grovedb PR 670)" and instead reflect that these functions are
now 🔌 FFI available. Additionally, remove the DOC-13 and DOC-14 entries from
lines 481-482 in Appendix B's "Not implemented anywhere" section entirely, since
these aggregation functions are now implemented and should no longer appear in
that unimplemented catalog.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45d6d783-aa20-414c-975b-b1c6e0c25280
📒 Files selected for processing (5)
packages/rs-sdk-ffi/src/document/queries/average.rspackages/rs-sdk-ffi/src/document/queries/count.rspackages/rs-sdk-ffi/src/document/queries/mod.rspackages/rs-sdk-ffi/src/document/queries/sum.rspackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
After reclassifying DOC-13/14 🚫→🔌, two cross-references still said "blocked on grovedb PR 670": - Appendix A getDocuments RPC row: status ✅/🧪/🚫 → ✅/🧪/🔌, and "SUM/AVG are upstream-blocked (grovedb PR 670)" → "FFI-available, no app UI yet". - Appendix B: moved DOC-13/14 out of "🚫 Not implemented anywhere" into "🔌 SDK-only (FFI/wrapper exists, no UI)" (GRP-04 stays under 🚫). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All three prior reviewer findings (typed-error preservation, FFI unit tests, duplicated # Safety prose) are resolved in the current head. One valid CodeRabbit doc inconsistency remains: TEST_PLAN.md's Appendix A and Appendix B still describe DOC-13/DOC-14 as upstream-blocked stubs, contradicting the §4.7 rows and the now-implemented FFI surface this PR ships.
Review coverage note: the following reviewer lanes failed or produced unparseable output and were not included in validated findings: claude-ffi-engineer, claude-general, claude-rust-quality, claude-security-auditor. In this run the Claude ACP lanes failed with the current account limit resetting at 8am America/Chicago; available Codex lanes were still verified.
Reviewed commit 762f68d0 with cumulative reconciliation against prior commit 074c91bc.
Findings
-
[suggestion] Appendix A still says SUM/AVG are upstream-blocked; Appendix B still lists them as NotImplemented stubs —
packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md:394
The §4.7 catalog rows for DOC-13/DOC-14 (lines 218–219) correctly mark sum/average as 🔌 withdash_sdk_document_sum/dash_sdk_document_averageimplemented, but two cross-references elsewhere in the same document still describe them as upstream-blocked stubs: -
Line 394 (Appendix A — gRPC read-RPC coverage, Document section):
SUM/AVG are upstream-blocked (grovedb PR 670) — DOC-13/14. -
Lines 481–482 (Appendix B — Theoretically possible but not runnable in-app):
DOC-13 document SUM aggregation — FFI stub returns NotImplemented (blocked on grovedb PR 670)/DOC-14 document AVERAGE aggregation — FFI stub returns NotImplemented (blocked on grovedb PR 670).
Those references contradict the rows this PR just updated and falsely advertise the stub behavior the PR removed. Update Appendix A to reflect that SUM/AVG are now implemented (FFI-only, no in-app UI) and remove DOC-13/DOC-14 from Appendix B (or move them to the SDK-implemented / no-UI bucket).
Resolved prior findings
- Preserve typed SDK errors from the sum/average fetch instead of collapsing into InternalError — Fixed at sum.rs:151–153 and average.rs:149–151 — both now use
.map_err(FFIError::from)?on the SDKfetchcall, preserving the existingFrom<dash_sdk::Error>classification (NetworkError / Timeout / DriveInternalError / NotFound / ProtocolError). The adjacent count.rs path was aligned in the same commit. - Add small unit tests for the new sum/average FFI surface (empty sum_property, JSON shape, hex keys) — Fixed by extracting the empty-property rejection into
validate_aggregation_property(sum.rs:38–47) and adding pure-FFI tests:validate_aggregation_property_rejects_empty_accepts_named,document_sum_result_serializes_to_expected_shape(covers empty + hex keys, signed value), anddocument_average_result_serializes_to_expected_shape(count+sum pair shape) at sum.rs:180–220 and average.rs:180–201. # Safetysection states the contract twice (intra-doc reference + inline bullets) — Fixed — sum.rs:93–97 and average.rs:91–95 now keep only the four-bullet per-parameter contract; theSame contract as …intra-doc reference prose was removed.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
Heads-up for reviewers/mergers: the one open finding (Appendix A/B still describing DOC-13/14 as grovedb-blocked stubs) was reviewed against |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior Appendix A/B inconsistency finding is FIXED in the current head (line 394 + Appendix B reclassification). New cumulative review found two convergent correctness issues in the sum/average FFI: ungrouped requests are routed through DocumentSplitSums/Averages, which the rs-sdk source documents as the group-by path — for In-based and carrier-aggregate where clauses this returns per-key entries instead of the documented single empty-key total. Other agent findings (error-code classification in shared parse helpers, panic-safety) are pre-existing or out of scope for this PR.
🔴 2 blocking
🤖 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-sdk-ffi/src/document/queries/sum.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/document/queries/sum.rs:144-157: Ungrouped SUM queries don't always return the single-empty-key aggregate the doc promises
The FFI doc-comments (lines 53-55 and 66-67) state that for empty/null `group_by_json` the result is a one-entry map with an empty key (`sums[""]` is the total). However the path always calls `DocumentSplitSums::fetch`, which `rs-sdk/src/platform/documents/document_split_sums.rs:5-8` explicitly scopes to `select=SUM, group_by=[...]` and points aggregate callers at `DocumentSum`. Per `sum_proof_helpers.rs:204-292`, only `DocumentSumMode::RangeProof` (range, no In, no distinct) emits the single-empty-key entry the doc promises; `PointLookupProof` (point/`in`) and `RangeAggregateCarrierProof` (`in + range`) emit one entry per matched key, and `try_into_flat_map` preserves that shape. So an iOS caller passing `where field in [...]` with no `group_by_json` gets `{sums:{<hexKey1>:v1,<hexKey2>:v2,...}}` (or `{}` for no matches) instead of the documented `{sums:{"":<total>}}`. Either route ungrouped queries through `dash_sdk::platform::documents::document_sum::DocumentSum::fetch` (which folds all verified entries into a single `i64` with checked-add overflow handling and returns 0 for empty aggregates), or correct the doc-comment to describe the actual per-key wire shape.
In `packages/rs-sdk-ffi/src/document/queries/average.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/document/queries/average.rs:149-157: Ungrouped AVG queries don't always return the single-empty-key aggregate the doc promises
Symmetric to the sum issue: the FFI documents ungrouped average results as a single `averages[""]` `(count, sum)` pair, but the implementation always calls `DocumentSplitAverages::fetch` and flattens via `try_into_flat_map`. `rs-sdk`'s `DocumentSplitAverages` is the group-by surface; aggregate callers should use `DocumentAverage::fetch` (which folds all verified entries into one `(count, sum)` pair and returns `(0, 0)` for empty aggregates). Under `PointLookupProof` and `RangeAggregateCarrierProof` shapes, an ungrouped average request returns one `(count, sum)` pair per matched key (or `{}`), so callers computing `sum/count` from the documented `averages[""]` get the wrong number or a missing key. Either route ungrouped queries through `DocumentAverage::fetch` or correct the doc-comment to describe the actual per-key wire shape.
thepastaclaw (blocking): the FFI promised a single empty-key total for
ungrouped sum/average, but always called DocumentSplitSums/Averages —
the per-group surface (SplitSumEntry is one entry per matched group).
For an `in`/range-carrier `where` with no group_by, that returns
per-key entries, not the documented `{sums:{"":total}}`.
Branch on group_by:
- empty → DocumentSum::fetch / DocumentAverage::fetch (fold all
verified entries to one total) → {"sums":{"":<i64>}} /
{"averages":{"":{"count","sum"}}}; absent aggregate → 0 / (0,0).
- non-empty → DocumentSplitSums/Averages::fetch + try_into_flat_map →
per-group hex keys (unchanged).
Aggregate fetch also uses .map_err(FFIError::from). Doc-comments now
describe both shapes; 4 ungrouped-shape unit tests added. count.rs
left as-is (its analogous behavior is pre-existing / out of scope).
C ABI unchanged. cargo build/test (293)/clippy/fmt -p rs-sdk-ffi clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both prior blocking findings (ungrouped SUM/AVG routed through the per-group DocumentSplitSums/Averages type instead of the aggregate type) are FIXED in the current head. Verified at sum.rs:179-194 and average.rs:170-186 — the ungrouped path now branches on group_by.is_empty() and routes through DocumentSum::fetch / DocumentAverage::fetch, returning the documented single empty-key total with zero fallback. Grouped queries retain DocumentSplitSums/Averages with try_into_flat_map. No new in-scope blocking, suggestion, or nitpick findings from any reviewer lane; CodeRabbit reported zero inline findings (coverage was rate-limited and degraded).
2 participants
Issue being fixed or feature implemented
dash_sdk_document_sumanddash_sdk_document_averagewere stubs returningNotImplemented, citing "blocked upstream on grovedb PR 670". That block is stale. The pinned grovedb (fc81498) already exposesverify_aggregate_sum_query/verify_aggregate_count_and_sum_query[_per_key], and the entire Rust stack below the FFI is complete:drive_document_sum_query/drive_document_count_and_sum_querydispatchers + executors, andverify/document_sum/proof verifiers.impl Fetch for DocumentSplitSums/DocumentSplitAverages.This left the two FFI entry points (DOC-13 sum / DOC-14 average) as the only gap.
What was done?
Implemented both FFI functions as mirrors of the already-working
dash_sdk_document_count:DocumentQueryfrom the same JSON params (where/order_by/group_by/limit), selectingSelectProjection::sum(field)/avg(field)(the projection carries the field — there is no separate*_starcall).DocumentSplitSums::fetch/DocumentSplitAverages::fetch, fold with the overflow-safetry_into_flat_map, hex-encode keys, and serialize:{"sums":{"<hex-key>":<i64>}}{"averages":{"<hex-key>":{"count":<u64>,"sum":<i64>}}}(caller divides)sum_propertyis rejected up front withInvalidParameter(rs-sdk's select assertions require a non-empty field — fail fast for a clearer error).where/order/limitparsing helpers incount.rspromoted topub(super)and reused, so cross-mode parsing stays identical across count/sum/average.DOC-13/DOC-14🚫 → 🔌(FFI-available, no app UI yet).How Has This Been Tested?
cargo build -p rs-sdk-ffi— clean.cargo clippy -p rs-sdk-ffi --all-targets— no new warnings.cargo fmt --all— no-op (already formatted).Follow-up (separate, build-gated): a SwiftUI Sum/Average view (mirroring
CountDocumentsView) + asummable-index QA fixture to drive DOC-13/14 in-app (🔌 → 🧪).Breaking Changes
None — fills two previously-
NotImplementedFFI bodies; ABI unchanged.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation