refactor: PathQuery::classify() — one shared shape decision for query dispatch - #795
Conversation
…uery dispatch Every prove/verify/read entry point currently re-discovers what kind of query it was handed by calling the has_*/validate_* helpers in its own order. This adds PathQueryShape and PathQuery::classify() as the single named version of that decision — pure (no DB, so a verifier classifies identically to the prover), total (every query maps to one shape or a typed InvalidQuery), and mirroring prove_query_non_serialized's exact gate order so migrating call sites keep their error surface. Behavior- preserving: classify() is implemented entirely over the existing validators and nothing dispatches through it yet; it is the foundation for the unified-PathQuery work (axis reads, sum-budget reads) that follows. Also fixes a grovedbg drift: the debugger's mirrored Query type lacked add_parent_tree_on_subquery and the converter hardcoded it to false, so debugger-issued queries could never exercise the flag. The field is serde(default) so payloads from older frontends keep decoding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change adds pure ChangesQuery Shape Classification
Query Flag Propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to This change adds query-shape classification and preserves existing behavior, with no actionable merge-blocking risk remaining after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #795 +/- ##
===========================================
+ Coverage 92.21% 92.24% +0.02%
===========================================
Files 257 258 +1
Lines 78176 78471 +295
===========================================
+ Hits 72091 72382 +291
- Misses 6085 6089 +4
🚀 New features to boost your workflow:
|
…uery stack) Brings the branch across #795-#809: the aggregate_over_value_range rename, GroveVersion params on the indexed-axis family, the AggregateFold grammar, and the dual-aggregate (PCPS) count secondary. Adaptations beyond conflict resolution: - run_path_query's RankedPage dispatch consumes the new IndexedTopKPage return shape (.entries); surfacing the true 'skipped' through PathQueryRun is left as a unified-API follow-up. - lib.rs re-export conflict: both re-exports kept. Zero changes to the counted-descent core: its generic already handled ProvableCountAndSum aggregates (sum/avg secondaries were PCPS since its base, #791), so #809's count-axis flip lands on an already-supported flavor — the full workspace suite passes unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Context
This is PR 1 of the unified PathQuery effort: making
PathQueryexpress and serve every query shape (key selection, aggregates, count-offset pagination, and — in upcoming PRs — indexed-axis reads and sum-budget reads) through one classification shared by reader, prover, and verifier.Today each entry point re-discovers the shape of the query it was handed by calling the
has_*/validate_*helpers in its own order. Since prover and verifier must agree on what a query means (the proof does not carry the query), that discovery is consensus-relevant — and it currently exists in several copies.What this does
PathQuery::classify() -> Result<PathQueryShape, Error>(newgrovedb/src/query/shape.rs): pure (no DB access — a verifier holding only the query classifies identically to the prover), total (every query maps to exactly one shape or a typedInvalidQuery), and mirrorsprove_query_non_serialized's exact gate order (aggregate-count → aggregate-sum → combined → count-offset → key selection) so future call-site migrations keep their error surface byte-for-byte.KeySelection,CountOffsetPaginated,AggregateLeaf/AggregateCarrier×AggregateKind::{Count, Sum, CountAndSum}. Axis and sum-budget shapes arrive with the vocabulary PR.Querylackedadd_parent_tree_on_subqueryand the converter hardcodedfalse, so debugger-issued queries could never exercise the flag. Added withserde(default)so older frontend payloads keep decoding.Tests
classify()must return the exact error the corresponding validator returns.offset == Some(0)treated as no offset, and mixed-aggregate rejection.InvalidQuery.grovedb+grovedb-querysuites green;--no-default-features --features verifybuild green.Noted in passing (not fixed here)
While writing tests:
Query::insert_itemcollision-merges an aggregate wrapper with an overlapping plain item and silently degrades it to a plain range (AggregateCountAndSumOnRange(Range(a..z))+insert_key("extra")→items == [Range(a..z)]). Pre-existing; will be filed/fixed separately.🤖 Generated with Claude Code
Summary by CodeRabbit