feat(grovedb,query): allow SizedQuery::limit on carrier AggregateCountOnRange - #664
Conversation
…tOnRange Split the SizedQuery-level size-constraint check by shape so the leaf form still rejects pagination (a leaf returns a single u64; pagination would silently change the answer) but the carrier form accepts `SizedQuery::limit` to cap the number of outer-key matches the walk returns. Each matched outer key still produces a complete leaf-ACOR u64 — the inner range isn't capped. `SizedQuery::offset` remains rejected on carriers pending a separate design pass on outer-dimension pagination semantics. Threaded the limit through the carrier verifier (`execute_carrier_layer_proof` now takes `outer_limit` so its merk walker stops at the same boundary the prover's truncation produced) and through the no-proof `query_aggregate_count_per_key` (shallow outer query carries the limit). Concrete upstream use case: dashpay/platform "Q8 with outer Range" (SELECT ... GROUP BY brand WHERE brand > X AND color > Y LIMIT 20), which maps to a carrier ACOR with `SizedQuery::limit = Some(20)`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements sized query limit pagination for carrier-shaped AggregateCountOnRange queries by distinguishing validation constraints (leaf vs carrier), threading limits through proof verification, constructing queries with those limits, and validating the behavior end-to-end. ChangesCarrier limit pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #664 +/- ##
========================================
Coverage 90.76% 90.77%
========================================
Files 189 189
Lines 56407 56466 +59
========================================
+ Hits 51196 51255 +59
Misses 5211 5211
🚀 New features to boost your workflow:
|
Add a direct unit test for the `query_validation_error_to_static_str` helper that exercises both the normal `InvalidOperation` projection and the defensive catch-all for other QueryError variants. The catch-all is unreachable from real `Query::validate_aggregate_count_on_range` results (its comment explicitly says so) but is kept as a defensive guard against future bugs; this test pins the projection behavior so the guard isn't silently broken later. Lifts patch coverage on this PR from 98.36% to 100%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both spots still said "pagination is rejected" but this PR makes carrier `SizedQuery::limit` valid and propagates it through the no-proof path and the proof verifier. The public `query_aggregate_count_per_key` doc and the internal `classify_aggregate_count_path_query` doc both now describe the shape-specific rules: leaf still rejects limit and offset; carrier accepts limit and rejects offset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…SumTree Pulls in PR #666 which adds the `Element::NotCountedOrSummed` wrapper — the third aggregation-suppression wrapper, suppressing BOTH count and sum contributions when inserted into a CountSumTree / ProvableCountSumTree parent — alongside PRs #663 / #664 (aggregate-count carrier improvements) already on develop. PR #666's contract specified four sum-bearing inner types (SumTree, BigSumTree, CountSumTree, ProvableCountSumTree). Because this branch adds `Element::ProvableSumTree` (the new fifth sum-bearing variant), the wrapper's allow-list is widened here to include it, matching the NotSummed allow-list extension we already did on the sum side. ## Conflict resolution ### Element / ElementType discriminants - `Element::ProvableSumTree` stays at variant 17 (our branch). - `Element::NotCountedOrSummed` slides from 17 (develop) to 18. - `NOT_COUNTED_OR_SUMMED_WRAPPER_DISCRIMINANT` becomes 18. - New synthetic twin `NotCountedOrSummedProvableSumTree = 193` (`0xC1`), hand-assigned because base 17 overflows the low-nibble formula — same reason `NotSummedProvableSumTree = 177` is hand-assigned at `0xB1`. Other four twins (196/197/199/202) keep the `0xC0 | base` formula slots. - `ElementType::base()` falls back to a per-variant match for ALL hand-assigned wrapper twins (NotSummed + NotCountedOrSummed). The develop-side `disc & NOT_*_BASE_MASK` paths are replaced because they'd collide with the per-variant slot assignments. ### Allow-list extensions `ProvableSumTree` joins the inner allow-list in every gate: - `Element::new_not_counted_or_summed` constructor. - `Element::validate_wrapper_invariants` (deserialize post-check). - `Element::serialize` pre-check. - `Element::deserialize` post-check. - All matching test data sets and rejection bytes. ### Batch propagation `GroveOp::InsertTreeWithRootHash` carries a new `not_counted_or_summed: bool` field (added by #666). Our branch's `Element::ProvableSumTree` build site was updated to set it from the upstream `not_counted_or_summed` local, matching the other tree variants. ### Test wrapper-byte fix The `deserialize_rejects_long_nested_wrapper_chain_without_recursion` test fills 1024 bytes with the wrapper discriminant. PR #666 used `17`; in this branch it must be `18` because slot 17 is `ProvableSumTree`. ## New tests - `direct_insert_not_counted_or_summed_provable_sum_tree_in_count_sum_tree_excludes_both_axes`: wrapped ProvableSumTree inside a CountSumTree parent — outer aggregate must read `(count=1, sum=7)` (only the unwrapped sum item contributes). - `direct_insert_not_counted_or_summed_provable_sum_tree_in_provable_count_sum_tree_excludes_both_axes`: same scenario but with a ProvableCountSumTree parent. - `constructor_invariants` extended to also accept `new_not_counted_or_summed(new_provable_sum_tree(None))`. ## Verification - `cargo build --workspace`: clean - `cargo clippy --workspace --all-features`: clean - `cargo test --workspace`: 3138 / 0 fail (3136 → 3138, +2 new end-to-end ProvableSumTree+NotCountedOrSummed tests; remaining delta from the develop merge bringing in #663/#664/#666 tests). - `cargo test -p grovedb-element --lib`: 99 / 0 fail - `cargo test -p grovedb --lib not_counted_or_summed`: 11 / 0 fail Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Issue being fixed or feature implemented
Follow-up to #663 (allow
AggregateCountOnRangeas carrier subquery). That change deliberately acceptedRange*outer items on carriers, but theSizedQuery::limit/offsetconstraint inherited from leaf-ACOR was still applied uniformly — which makes carrier ACOR withRange*outer items practically unusable above a certain dataset size (the proof grows linearly in the number of in-range outer keys, with no way for the caller to cap it).The concrete upstream use case from
dashpay/platformis a "Q8 with outer Range"GROUP BYshape:which maps to a carrier ACOR: outer item
RangeAfter("brand_050"..),subquery_path = ["color"], subqueryAggregateCountOnRange(RangeAfter("color_00000500"..)), capped at 20 outer matches viaSizedQuery::limit. Today thisPathQueryis rejected at validation withInvalidQuery("AggregateCountOnRange queries may not set SizedQuery::limit")even though the semantic is well-defined.What was done?
Split the SizedQuery-level size-constraint check by shape:
AggregateCountOnRange(_)item, no subqueries): keep rejecting bothlimitandoffset. A leaf returns a singleu64; pagination would silently change the answer.Key/Range*items routing to a leaf-ACOR subquery): acceptSizedQuery::limit— it caps the number of outer-key matches the carrier walks. Each matched outer key still produces a complete leaf-ACORu64(the inner range is not capped).SizedQuery::offsetis still rejected on carriers; the error message documents that skipping outer matches changes which(outer_key, u64)pairs end up in the proof and the use case for that hasn't been designed yet.Changes:
grovedb/src/query/mod.rs:SizedQuery::check_aggregate_count_size_constraintsis replaced bycheck_leaf_aggregate_count_size_constraints(strict) andcheck_carrier_aggregate_count_size_constraints(offset-only).validate_aggregate_count_on_rangeclassifies the inner query first and then dispatches the appropriate check.validate_leaf_aggregate_count_on_rangestill uses the leaf-strict check.grovedb/src/operations/proof/aggregate_count/{helpers,per_key}.rs: threadedSizedQuery::limitthrough to the carrier verifier so the merk walker'sexecute_proof(... limit, ...)stops at the same boundary the prover's truncation produced. Without this the verifier rejects truncated proofs with "Proof is missing data for query."grovedb/src/operations/get/query.rs:query_aggregate_count_per_key(no-proof path) propagatesSizedQuery::limitto the shallow outer-walk query.The merk-level hash composition is unchanged; the prover's existing
Merk::prove_unchecked_query_items(query, limit, ...)call already truncates correctly when the carrier merk'slimitarrives non-None.How Has This Been Tested?
cargo test --workspace --features 'minimal verify'passes (all 1543 grovedb tests + the rest of the workspace).Replaced
carrier_pagination_is_rejected_at_entrywithcarrier_aggregate_count_rejects_offset(limit is no longer rejected; offset still is). Added five new tests ingrovedb/src/tests/aggregate_count_query_tests.rs:leaf_aggregate_count_still_rejects_limit— the leaf strict path is byte-identical to before this PR.carrier_keys_outer_with_limit_caps_results— outerKeys+limit=2against 4 brands returns exactly 2(brand, u64)pairs in lex-asc order, proof verifies.carrier_range_outer_with_limit_caps_results— outerRangeAfter+limit=2against 4 in-range brands behaves identically (the upstream Q8-with-Range shape).carrier_range_outer_with_limit_zero_returns_no_results—limit=0walks zero outer matches; exercised via the no-proof per-key API because the existingprove_query_non_serializedentry-point gate has a long-standing unconditional rejection oflimit == 0for proved queries ("proved path queries can not be for limit 0"), which is outside the scope of this PR.carrier_range_outer_with_limit_exceeding_available_walks_all—limit=100against 2 in-range brands returns all 2, matching the no-proof per-key result byte-for-byte.Also added two SizedQuery-level unit tests in
grovedb/src/query/mod.rs:sized_query_validate_leaf_acor_rejects_limit_and_offsetandsized_query_validate_carrier_acor_accepts_limit_rejects_offset.cargo clippy -p grovedb --lib --features minimal,verify -- -D warningsis clean.cargo fmt --checkis clean.Breaking Changes
None. Wire format is unchanged —
SizedQuery::limitwas already part of the serialized PathQuery and just rejected client-side at validation. Any previously-valid query keeps its byte-identical behavior. The relaxed rule applies unconditionally (no per-feature grove-version gate added); if maintainers want a version gate for the new carrier-with-limit acceptance so older verifiers continue to reject these queries, happy to add one — flagging here per the original spec.Out of scope (intentionally deferred):
SizedQuery::offseton carriers — the rationale is in the validator's error message; adding it later is a separate, well-scoped change.u64and pagination doesn't apply).dashpay/platform— a separate PR there consumes the new grovedb behavior.Checklist:
Summary by CodeRabbit
Bug Fixes
Enhancements