feat(querier): group count by a promoted attribute column (RFC 0037 §3.3) - #601
Conversation
…3.3) Extend the `count … by` group surface so a group term may be a promoted attribute path (`resource.<key>` / `attr.<key>`), making the canonical GenAI aggregation — `count by attr.gen_ai.request.model, bucket(1h)` — expressible. Previously only `service`, `template_id`, `param(n)`, `bucket(w)`, and bare fields could group; `Field::Resource(_)`/`Field::Attr(_)` group keys were rejected outright. The compiler (`field_group_expr`) now lowers a promoted attribute group key to its column when that column is present in the scanned union schema — exactly `service`'s pattern, reusing `promoted_column_name` + `has_column`. A key whose column is absent from every scanned file is rejected with a promotion hint rather than collapsing every row into one NULL bucket or grouping over an unpruned JSON scan (hazard #6). DataFusion supplies per-file NULLs for any pre-promotion partitions within a mixed scan, so the typed-NULL fallback is free; no `PromotedAttributes` threading is needed — schema presence is the promotion signal. The DSL parsers are relaxed to accept the path forms as group terms: the string grammar's `parse_group_term` routes through `parse_path` (which already yields `Field::Resource`/`Field::Attr`), and the structured surface's `RawGroupTerm::into_ir` accepts the `{resource|attr}` object form. Both surfaces still admit the same set. This extends the RFC 0002 §7 `field_list` grammar per RFC 0037 §3.3 — additive (nothing that parsed before fails); the structured unit test that asserted the old bare-field-only rejection is flipped to assert the new acceptance (a sanctioned contract change, RFC 0037 §3.3). Acceptance: RFC0037.4 (rfc0002_dsl.rs) — grouped count over a promoted `attr.gen_ai.request.model` matches a brute-force `(bucket, model) → count` oracle; the same query against a non-promoted key is rejected with a promotion hint. Refs #546 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds DSL and compiler support for grouping by promoted ChangesPromoted attribute grouping
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DSLParser
participant QueryCompiler
participant UnionSchema
DSLParser->>QueryCompiler: produce resource/attr group term
QueryCompiler->>UnionSchema: check promoted column
UnionSchema-->>QueryCompiler: return column presence
QueryCompiler-->>DSLParser: compile grouping or promotion error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ourios-querier/src/compile.rs (1)
437-476: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd a property test for the promoted-grouping schema invariant.
This query-planner change has schema-dependent invariants beyond the fixed integration fixture. Add a nearby
proptestcovering resource and attr keys across promoted, absent, and mixed scan schemas.As per coding guidelines, “Unit tests must be next to the code and are mandatory for anything non-trivial” and “Use property tests (
proptest) for anything with an invariant: … the query planner.”🤖 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 `@crates/ourios-querier/src/compile.rs` around lines 437 - 476, Add a nearby proptest for field_group_expr/group_by_promoted that generates resource and attr keys with promoted, absent, and mixed scan schemas. Assert promoted keys resolve to their promoted columns, absent keys return the promotion-hint InvalidQuery, and mixed schemas preserve typed NULL behavior for unpromoted partitions. Keep the test focused on the schema invariant and use existing test helpers and proptest conventions.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/ourios-querier/src/compile.rs`:
- Around line 437-476: Add a nearby proptest for
field_group_expr/group_by_promoted that generates resource and attr keys with
promoted, absent, and mixed scan schemas. Assert promoted keys resolve to their
promoted columns, absent keys return the promotion-hint InvalidQuery, and mixed
schemas preserve typed NULL behavior for unpromoted partitions. Keep the test
focused on the schema invariant and use existing test helpers and proptest
conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8ddfec3-b0d1-437d-8f32-20293ac29590
📒 Files selected for processing (4)
crates/ourios-querier/src/compile.rscrates/ourios-querier/src/dsl/parse.rscrates/ourios-querier/src/dsl/structured.rscrates/ourios-querier/tests/it/rfc0002_dsl.rs
There was a problem hiding this comment.
Pull request overview
This PR completes RFC 0037 §3.3 on the querier side by allowing count … by to group on resource. / attr. paths when the corresponding attribute key is promoted to a dedicated column in the scanned union schema, while rejecting non-promoted group keys with a promotion hint to avoid silently degrading into NULL-bucket grouping or JSON scans.
Changes:
- Extend both string and structured DSL parsers to accept
resource.<k>/attr.<k>group terms inbylists. - Update the query compiler to lower
Field::Resource(_)/Field::Attr(_)group keys to their promoted column (when present) and otherwise reject with a promotion hint. - Add an integration test asserting grouped counts over a promoted
attr.gen_ai.request.modelmatch a brute-force oracle, and that the same query is rejected when the key is not promoted.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/ourios-querier/tests/it/rfc0002_dsl.rs | Adds RFC0037.4 integration coverage for grouping count … by a promoted attr.* column and rejecting unpromoted keys with a hint. |
| crates/ourios-querier/src/dsl/structured.rs | Aligns structured DSL by-list parsing with the string surface by accepting `{resource |
| crates/ourios-querier/src/dsl/parse.rs | Updates the string DSL group-term parser to route through parse_path, enabling resource./attr. paths in by lists. |
| crates/ourios-querier/src/compile.rs | Implements compiler lowering for promoted attribute group keys (presence-gated by the scanned union schema) and produces a promotion-hint rejection otherwise. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…n hint Review fix (Copilot): the rejection for grouping by a non-promoted attribute named the derived column (attr.gen_ai.request.model), but storage. promoted_attributes expects the raw key without the attr./resource. prefix. The hint now names the raw key and the correct sublist — "add 'gen_ai.request.model' to storage.promoted_attributes.log" — and RFC0037.4's rejection assertion pins both the raw key and the sublist string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
The three implementation slices landed (#599 §3.1, #600 §3.2, #601 §3.3). Flip status specified→green and map each §5 scenario to its greppable test: RFC0037.1/.3/.4 are the new slice tests; RFC0037.2 (structured-body reconstruction) is covered by the standing reconstruction property (RFC 0024 generates structured bodies) plus the miner/parquet Structured round-trips and the byte-for-byte retention unit; RFC0037.5 (absent-body parity) by rfc0025_absent_body.rs. `validated` waits on the v9 corpus (§3.4 calibration). Refs #546 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
* docs(rfc): flip RFC 0037 to green — all five §5 scenarios pass The three implementation slices landed (#599 §3.1, #600 §3.2, #601 §3.3). Flip status specified→green and map each §5 scenario to its greppable test: RFC0037.1/.3/.4 are the new slice tests; RFC0037.2 (structured-body reconstruction) is covered by the standing reconstruction property (RFC 0024 generates structured bodies) plus the miner/parquet Structured round-trips and the byte-for-byte retention unit; RFC0037.5 (absent-body parity) by rfc0025_absent_body.rs. `validated` waits on the v9 corpus (§3.4 calibration). Refs #546 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org> * docs(rfc): correct RFC0037.2 coverage wording — canonical-JSON equal, not byte-for-byte Review fix (Copilot): the RFC 0024 property tests assert structured bodies round-trip *canonical-JSON equal* (decode_any_value on the rebuilt bytes equals the original AnyValue — decoded value equality), not byte-for-byte. Byte identity of the stored canonical JSON is pinned separately by rfc0037_3_structured_body_retained_byte_for_byte. Reword §6 to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org> * docs(rfc): fix RFC0037.3/.4 §6 wording — attr requirement levels + key order Review fixes (CodeRabbit): ourios.service is recommended (emitted when a service is present), not required — reword RFC0037.3 to "required ourios.tenant plus recommended ourios.service present for this record". And RFC0037.4's result map is keyed in by-list order [model, bucket], not (bucket, model) — correct the §5 scenario and §6 wording to (model, bucket). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org> --------- Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
RFC 0037 slice C — group
count … bya promoted attribute column (§3.3)Third and final implementation slice (after A #599, B #600). This is the query-side change that makes the canonical GenAI aggregation expressible:
count by attr.gen_ai.request.model, bucket(1h).Before.
count … bycould group only byservice,template_id,param(n),bucket(w), and bare top-level fields.Field::Resource(_)/Field::Attr(_)group keys were rejected at compile, and the parsers rejectedresource./attr.paths in a by-list.Change.
field_group_expr): a promoted attribute group key lowers to its column when that column is present in the scanned union schema — exactlyservice's pattern, reusingpromoted_column_name+has_column(no new plumbing). A key whose column is absent from every scanned file is rejected with a promotion hint rather than collapsing every row into one NULL bucket or grouping over an unpruned JSON scan (hazard docs(rfc-0001): add §5 acceptance criteria (drafted → specified) #6). DataFusion supplies per-file NULLs for pre-promotion partitions within a mixed scan, so the typed-NULL fallback is free — schema presence is the promotion signal, so noPromotedAttributesthreading is needed.parse_group_termroutes throughparse_path(which already yieldsField::Resource/Field::Attr), and the structuredRawGroupTerm::into_iraccepts the{resource|attr}object form. Both surfaces still admit the same set.Invariants (CLAUDE.md §4)
ourios://query-schemaalready advertises promoted keys + cost tiers).Verification
rfc0002_dsl.rs: grouped count over a promotedattr.gen_ai.request.modelmatches a brute-force(bucket, model) → countoracle (reusing the RFC 0031 L4 comparison shape); the identical query against a store that did not promote the key is rejected with a promotion hint. ✅ourios-queriersuite green (116 lib + 95 integration); fmt + clippy clean. Verified no string-surface test asserted the old reject, andparse_fieldis unchanged for its other callers (select field-lists still correctly reject paths).Refs #546. With A + B + this, all §3.1–§3.3 deltas are implemented; a short follow-up flips RFC 0037 to
greenafter confirming RFC0037.2 (reconstruction) and RFC0037.5 (absent-body) against existing RFC 0005 / 0025 coverage.validatedwaits on the v9 corpus.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
resourceandattrattributes.Tests