From 01d570eb360c859d28087790468e7e005be2e6a8 Mon Sep 17 00:00:00 2001 From: Jens Holdgaard Pedersen Date: Thu, 23 Jul 2026 05:57:28 +0200 Subject: [PATCH 1/2] =?UTF-8?q?feat(querier):=20group=20count=20by=20a=20p?= =?UTF-8?q?romoted=20attribute=20column=20(RFC=200037=20=C2=A73.3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the `count … by` group surface so a group term may be a promoted attribute path (`resource.` / `attr.`), 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 Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen --- crates/ourios-querier/src/compile.rs | 36 +++++-- crates/ourios-querier/src/dsl/parse.rs | 5 +- crates/ourios-querier/src/dsl/structured.rs | 76 ++++++++------- crates/ourios-querier/tests/it/rfc0002_dsl.rs | 95 +++++++++++++++++++ 4 files changed, 172 insertions(+), 40 deletions(-) diff --git a/crates/ourios-querier/src/compile.rs b/crates/ourios-querier/src/compile.rs index ba5244d0e..9584d4346 100644 --- a/crates/ourios-querier/src/compile.rs +++ b/crates/ourios-querier/src/compile.rs @@ -434,6 +434,25 @@ pub(crate) fn group_exprs(by: &[GroupTerm], df: &DataFrame) -> Result, .collect() } +/// Lower a group-by on a promoted attribute column (RFC 0037 §3.3). Groups on +/// the promoted `resource.` / `attr.` column when it is present in +/// the scanned union schema; otherwise rejects with a hint pointing at +/// promotion, so grouping never silently degrades to a single NULL bucket or +/// an unpruned JSON scan. +fn group_by_promoted(column: &str, key: &str, df: &DataFrame) -> Result { + let name = promoted_column_name(column, key); + if has_column(df, &name) { + Ok(Expr::Column(Column::new_unqualified(name))) + } else { + Err(QueryError::InvalidQuery { + detail: format!( + "grouping by '{name}' requires it to be a promoted attribute column present in \ + the queried range; promote the key via storage.promoted_attributes" + ), + }) + } +} + fn field_group_expr(field: &Field, df: &DataFrame) -> Result { match field { Field::Service => { @@ -445,13 +464,16 @@ fn field_group_expr(field: &Field, df: &DataFrame) -> Result { Ok(lit(ScalarValue::Utf8(None))) } } - // §7 confines `by`-list fields to the bare set, so these are only - // reachable from a hand-built IR — reject rather than group over the - // JSON-encoded attribute columns. - Field::Resource(_) | Field::Attr(_) => Err(QueryError::InvalidQuery { - detail: "grouping by resource/attr attributes is not supported in this query surface" - .to_string(), - }), + // RFC 0037 §3.3: group by a *promoted* attribute column. The column + // is present in the scanned union schema exactly when ≥ 1 scanned + // file promoted the key (DataFusion supplies per-file NULLs for any + // pre-promotion partitions within a mixed scan — the typed-NULL + // fallback happens for free). Absent from every scanned file, the key + // is not a usable group key here: reject with a promotion hint rather + // than collapse every row into one NULL bucket or group over an + // unpruned JSON scan (hazard #6). + Field::Resource(key) => group_by_promoted(columns::RESOURCE_ATTRIBUTES, key, df), + Field::Attr(key) => group_by_promoted(columns::ATTRIBUTES, key, df), _ => { let (column, optional) = column_of(field); if optional && !has_column(df, column) { diff --git a/crates/ourios-querier/src/dsl/parse.rs b/crates/ourios-querier/src/dsl/parse.rs index d25288172..6a0810993 100644 --- a/crates/ourios-querier/src/dsl/parse.rs +++ b/crates/ourios-querier/src/dsl/parse.rs @@ -1044,7 +1044,10 @@ impl<'a> Parser<'a> { self.expect(&Tok::RParen, "')' to close bucket(...)")?; Ok(GroupTerm::Bucket(width)) } - _ => Ok(GroupTerm::Field(self.parse_field()?)), + // A bare field, or (RFC 0037 §3.3) a `resource.`/`attr.` path — + // `parse_path` accepts both; the compiler gates the path forms on + // the key being a promoted column. + _ => Ok(GroupTerm::Field(self.parse_path()?)), } } diff --git a/crates/ourios-querier/src/dsl/structured.rs b/crates/ourios-querier/src/dsl/structured.rs index 1884c76a2..731be8aa4 100644 --- a/crates/ourios-querier/src/dsl/structured.rs +++ b/crates/ourios-querier/src/dsl/structured.rs @@ -467,19 +467,15 @@ impl RawGroupTerm { Self::Bucket(RawBucketTerm { bucket }) => { Ok(GroupTerm::Bucket(parse_duration_lexeme_pub(&bucket)?)) } - // The string DSL's `group_term = field` production (§7 v1.1) - // only accepts a bare top-level field — `resource.`/`attr.` - // paths are rejected there (see `parse_field`'s error message). - // A `{resource|attr}` object here would let the structured - // surface express a `by`-list the string grammar cannot, so it - // is rejected at the same boundary rather than reaching the - // planner (RFC0002.2). - Self::Field(RawField::Object(_)) => Err(DslError::new( - "a by-list field must be a bare field name (resource./attr. paths \ - are not allowed here)" - .to_string(), - )), - Self::Field(field @ RawField::Name(_)) => Ok(GroupTerm::Field(field.into_ir()?)), + // RFC 0037 §3.3: a `by`-list field is a bare top-level field or a + // `resource.`/`attr.` path (the `{resource|attr}` object form). + // Both surfaces admit the same set — the string grammar's + // `parse_group_term` now routes through `parse_path` — and the + // compiler gates the path forms on the key being a promoted + // column. + Self::Field(field @ (RawField::Object(_) | RawField::Name(_))) => { + Ok(GroupTerm::Field(field.into_ir()?)) + } } } } @@ -639,7 +635,7 @@ fn parse_time(s: &str) -> Result { #[cfg(test)] mod tests { use super::parse_structured; - use crate::dsl::ir::{Call, CmpOp, Field, OrdOp, Predicate, Stage, Value}; + use crate::dsl::ir::{Call, CmpOp, Field, GroupTerm, OrdOp, Predicate, Stage, Value}; #[test] fn parses_comparison_with_attr_object() { @@ -826,25 +822,41 @@ mod tests { } #[test] - fn rejects_resource_and_attr_group_terms() { - // The string DSL's `group_term = field` production (§7 v1.1) is - // bare-field-only; a structured `{resource|attr}` by-element would - // let this surface express a query the string grammar cannot - // (RFC0002.2), so it is rejected here rather than reaching the - // planner. - for req in [ - r#"{"predicate":{"const":true},"stages":[{"count":{"by":[{"resource":"k8s.pod.name"}]}}]}"#, - r#"{"predicate":{"const":true},"stages":[{"count":{"by":[{"attr":"http.status_code"}]}}]}"#, - r#"{"predicate":{"const":true},"stages":[{"avg":"confidence","by":[{"attr":"k"}]}]}"#, - ] { - let err = parse_structured(req).unwrap_err(); - assert!( - err.message().contains("bare field"), - "{}: {}", - req, - err.message() - ); + fn accepts_resource_and_attr_group_terms() { + // RFC 0037 §3.3 extends the by-list to `resource.`/`attr.` paths on + // both surfaces — the string grammar's `parse_group_term` now routes + // through `parse_path`, so the two surfaces still admit the same set. + // The compiler gates the path forms on the key being a promoted + // column; parsing accepts them. (Previously this surface rejected them + // to stay within the older bare-field-only string grammar.) + let count_cases: &[(&str, GroupTerm)] = &[ + ( + r#"{"predicate":{"const":true},"stages":[{"count":{"by":[{"resource":"k8s.pod.name"}]}}]}"#, + GroupTerm::Field(Field::Resource("k8s.pod.name".into())), + ), + ( + r#"{"predicate":{"const":true},"stages":[{"count":{"by":[{"attr":"http.status_code"}]}}]}"#, + GroupTerm::Field(Field::Attr("http.status_code".into())), + ), + ]; + for (req, expected) in count_cases { + let query = parse_structured(req).expect("resource/attr count group terms now parse"); + let Some(Stage::Count { by }) = query.stages.first() else { + panic!("{req}: expected a count stage"); + }; + assert_eq!(by.len(), 1, "{req}: one group term"); + assert_eq!(&by[0], expected, "{req}"); } + + // The relaxation applies to any grouped aggregate (they share + // `group_terms_to_ir`), e.g. `avg … by attr.k`. + assert!( + parse_structured( + r#"{"predicate":{"const":true},"stages":[{"avg":"confidence","by":[{"attr":"k"}]}]}"# + ) + .is_ok(), + "avg grouped by a resource/attr path also parses now" + ); } #[test] diff --git a/crates/ourios-querier/tests/it/rfc0002_dsl.rs b/crates/ourios-querier/tests/it/rfc0002_dsl.rs index 9fc97fe04..18db35868 100644 --- a/crates/ourios-querier/tests/it/rfc0002_dsl.rs +++ b/crates/ourios-querier/tests/it/rfc0002_dsl.rs @@ -1775,6 +1775,101 @@ async fn rfc0002_13_count_by_param_bucket_grouped_map() { ); } +/// Scenario RFC0037.4 — `count … by` a *promoted* attribute column +/// (`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. See `docs/rfcs/0037-genai-structured-log-events.md` +/// §3.3. +#[tokio::test] +async fn rfc0037_4_count_by_promoted_attribute() { + use std::collections::BTreeMap; + + use crate::common::{ + DEFAULT_WINDOW_NS, NOW, TS0, kv, no_aliases, rec_with_attrs, write_all, + write_all_with_promoted, + }; + use ourios_core::tenant::TenantId; + use ourios_parquet::PromotedAttributes; + + const SECOND_NS: u64 = 1_000_000_000; + const WIDTH_NS: u64 = 300 * SECOND_NS; // 5m + + // Rows carrying a `gen_ai.request.model` log attribute across two models + // and two 5-minute windows. + let rows: &[(&str, u64)] = &[ + ("gpt-4", TS0), + ("gpt-4", TS0 + 100 * SECOND_NS), + ("claude", TS0 + 150 * SECOND_NS), + ("gpt-4", TS0 + 400 * SECOND_NS), + ]; + let recs: Vec<_> = rows + .iter() + .map(|(model, ts)| { + rec_with_attrs( + "a", + *ts, + vec![kv("service.name", "checkout")], + vec![kv("gen_ai.request.model", model)], + ) + }) + .collect(); + + // Promote the log key so it gets a dedicated, groupable column. + let promoted = PromotedAttributes::new( + Vec::::new(), + vec!["gen_ai.request.model".to_string()], + ); + + let bucket = tempfile::TempDir::new().expect("temp"); + write_all_with_promoted(bucket.path(), &recs, &promoted); + + // Brute-force oracle: (model, bucket_key) → count. + let bucket_key = |ts: u64| { + let start = ts / WIDTH_NS * WIDTH_NS; + chrono::DateTime::from_timestamp_nanos(i64::try_from(start).expect("fixture ns")) + .to_rfc3339_opts(chrono::SecondsFormat::AutoSi, true) + }; + let mut oracle: BTreeMap, u64> = BTreeMap::new(); + for (model, ts) in rows { + *oracle + .entry(vec![(*model).to_string(), bucket_key(*ts)]) + .or_default() += 1; + } + + let result = run_dsl( + bucket.path(), + "template_id == 1 | range(2026-04-02T10:00:00Z, 2026-04-02T12:00:00Z) \ + | count by attr.gen_ai.request.model, bucket(5m)", + ) + .await; + assert_eq!( + group_map(&result), + oracle, + "grouped count over the promoted attribute matches the brute-force map" + ); + + // Rejection: the identical grouping over a store that did NOT promote the + // key is rejected with a promotion hint — never a silent unpruned scan. + let unpromoted = tempfile::TempDir::new().expect("temp"); + write_all(unpromoted.path(), &recs); + let query = ourios_querier::dsl::parse("template_id == 1 | count by attr.gen_ai.request.model") + .expect("parse"); + let err = ourios_querier::Querier::new(unpromoted.path()) + .run_query( + &query, + &TenantId::new("a"), + NOW, + DEFAULT_WINDOW_NS, + Some(&no_aliases()), + ) + .await + .expect_err("grouping by a non-promoted attribute must be rejected"); + assert!( + err.to_string().contains("promote"), + "the rejection hints at promotion; got: {err}" + ); +} + /// Scenario RFC0002.14 — `param(n)` misuse is a specific compile-time /// error. See `docs/rfcs/0002-query-dsl.md` §5 (amendment 2026-07-15). #[tokio::test] From c708933723ea6b5296ee572cc6c7bca2911789a0 Mon Sep 17 00:00:00 2001 From: Jens Holdgaard Pedersen Date: Thu, 23 Jul 2026 06:08:09 +0200 Subject: [PATCH 2/2] fix(querier): name the raw config key + sublist in the group-promotion hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen --- crates/ourios-querier/src/compile.rs | 12 ++++++++++-- crates/ourios-querier/tests/it/rfc0002_dsl.rs | 11 +++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/crates/ourios-querier/src/compile.rs b/crates/ourios-querier/src/compile.rs index 9584d4346..909d3c0c8 100644 --- a/crates/ourios-querier/src/compile.rs +++ b/crates/ourios-querier/src/compile.rs @@ -444,10 +444,18 @@ fn group_by_promoted(column: &str, key: &str, df: &DataFrame) -> Result