feat(querier): parse the DSL string + structured surface to one IR (RFC0002.2/.7/.8) - #145
Merged
Merged
Conversation
…one IR Implements SLICE 1 of the RFC 0002 logs query DSL in `ourios-querier`: a new `dsl` module with two front-ends over one IR. - `dsl/ir.rs` — the typed query IR (Query / Predicate / Field / Stage / Value / …), mirroring the §7 grammar exactly. No datafusion/arrow/SQL type appears (hazard CLAUDE.md §4.6). - `dsl/parse.rs` — a hand-rolled tokenizer + recursive-descent parser for the surface-β string DSL implementing §7: severity-RHS restricted to name|number with ord ops (no regex), str-fn arity/string-operand checks, §7 string escapes with literal-newline/control rejection, field validation, dotted vs bracketed attr keys. - `dsl/structured.rs` — serde Deserialize for the §6.4 JSON surface, converting to the SAME IR; severity routed to the severity predicate so both surfaces agree. - `dsl/display.rs` — canonical single-line β serializer with parse(serialize(q)) == q. - `dsl/mod.rs` — the hand-rolled DslError (Display + Error), no thiserror; messages cite the offending token/clause and leak no engine/SQL term. Flips three RFC 0002 acceptance stubs green (the other 8 stay #[ignore]'d, pending the DataFusion compile / later slices): - RFC0002.2 — a β string and the structured JSON compile to the same IR. - RFC0002.7 — proptest: parse(serialize(q)) == q over generated well-formed queries. - RFC0002.8 — a table of malformed queries each return a specific DslError with no datafusion/arrow/SQL substring. Verified locally: cargo fmt --all --check, clippy -p ourios-querier --all-targets --all-features -D warnings, and cargo test -p ourios-querier (36 lib unit tests + 3 flipped acceptance tests pass; 8 stay ignored; existing querier tests unchanged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 46 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a complete query DSL: typed IR, string DSL tokenizer+parser, structured JSON parser, canonical single-line serializer, module wiring and Cargo/dev-deps, plus comprehensive RFC0002 tests including property-based round-trip and malformed-input checks. ChangesLogs Query DSL Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@crates/ourios-querier/src/dsl/display.rs`:
- Around line 63-84: write_join currently only adds parentheses around
lower-precedence children, which loses grouping for nested same-precedence joins
(e.g., And([a, And([b,c])]) or Or([a, Or([b,c])]). Update the wrap computation
in write_join (the match on BindLevel) so it also parenthesizes same-precedence
nested groups: for BindLevel::And wrap when the child is Predicate::Or(_) or
Predicate::And(_), and for BindLevel::Or wrap when the child is
Predicate::Or(_); then call write_predicate(out, term, wrap) as before so
serialization preserves nested grouping.
In `@crates/ourios-querier/src/dsl/parse.rs`:
- Around line 693-714: The parser currently accepts any field for string
functions (matches/contains/starts_with/ends_with); after parsing the field via
parse_path() and before consuming the comma/arg, validate that the field is
string-typed and return a DslError if not (e.g. "{name}(...) requires a string
first argument, found ..."). Implement this by invoking the same
operand/type-check helper used in crates/ourios-querier/src/dsl/structured.rs
(or extract that helper into a shared function) and call it from parse.rs (use
parse_path(), name, and the Call::Matches/Contains/StartsWith/EndsWith sites as
reference points) so the parser rejects non-string fields consistently for those
string functions.
- Around line 333-380: validate_rfc3339 currently accepts any tail starting with
'+' or '-' (e.g. "+", "+05") — update the ok_tail logic in validate_rfc3339 to
require a full ±hh:mm offset: allow tail equal to "Z"/"z", or if tail starts
with '+' or '-' require len == 6, byte positions 1-2 and 4-5 be ASCII digits and
position 3 be ':' and validate hour (00..=23) and minute (00..=59); for
fractional-second tails starting with '.' keep existing branch but when it
contains a '+' or '-' extract the offset substring after the last '+'/'-' and
validate it with the same ±hh:mm rules; refer to the tail variable and ok_tail
check and ensure parse_time_pub reuse still calls validate_rfc3339.
In `@crates/ourios-querier/src/dsl/structured.rs`:
- Around line 327-381: When converting a "sort" stage in RawStage::into_ir,
validate RawSort.key with the same grammar the β parser uses (the
single-identifier/field-selector grammar in parse.rs) instead of accepting any
string; call the existing parser/validator used elsewhere (the function that
parses field selectors / identifiers in parse.rs) on s.key and return a DslError
if it fails, then construct Stage::Sort with the validated identifier (or its
canonical form) so the structured IR matches the string parser's expectations
(refer to RawSort, into_ir, and Stage::Sort to locate the change).
In `@crates/ourios-querier/tests/rfc0002_dsl.rs`:
- Around line 410-441: The test in rfc0002_dsl.rs currently only asserts that
the parse error message is non-empty; update the loop that calls
ourios_querier::dsl::parse so it asserts the error message explicitly mentions
the offending construct (use either the query string or the descriptive `what`),
e.g. by checking err.message().contains(query) || err.message().contains(what)
(or assert err.to_string().contains(...)); keep the existing variables `cases`,
`query`, `what`, `err`, and `msg` and replace the non-empty check with a
specific contains assertion to enforce RFC0002.8.
🪄 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 Plus
Run ID: ebc31504-0b39-4eef-9df2-98287f12a03e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/ourios-querier/Cargo.tomlcrates/ourios-querier/src/dsl/display.rscrates/ourios-querier/src/dsl/ir.rscrates/ourios-querier/src/dsl/mod.rscrates/ourios-querier/src/dsl/parse.rscrates/ourios-querier/src/dsl/structured.rscrates/ourios-querier/src/lib.rscrates/ourios-querier/tests/rfc0002_dsl.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the RFC 0002 “surface β” query DSL to ourios-querier by introducing a shared typed IR and two front-ends (string DSL + structured JSON) plus a canonical serializer intended to round-trip. This lands the user-facing query language layer that sits in front of the already-existing RFC 0007 execution layer, while preserving the “no DataFusion/SQL leakage” hazard constraint.
Changes:
- Introduce
ourios_querier::dslmodule with IR (ir.rs), string parser/tokenizer (parse.rs), structured JSON parser (structured.rs), and canonical serializer (display.rs). - Expose the DSL publicly from
crates/ourios-querier/src/lib.rs. - Flip RFC0002.2/.7/.8 acceptance tests from ignored stubs to active tests; add
serde/serde_jsonandproptestdependencies.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-querier/tests/rfc0002_dsl.rs | Enables DSL acceptance tests and adds proptest generator for round-trip property testing. |
| crates/ourios-querier/src/lib.rs | Exposes the new dsl module as a public API surface. |
| crates/ourios-querier/src/dsl/mod.rs | Defines the DSL module API (parse, parse_structured, serialize) and DslError. |
| crates/ourios-querier/src/dsl/ir.rs | Adds the shared typed IR that both front-ends target. |
| crates/ourios-querier/src/dsl/parse.rs | Implements the string DSL tokenizer + recursive-descent parser. |
| crates/ourios-querier/src/dsl/structured.rs | Implements the structured JSON surface and conversion into the shared IR. |
| crates/ourios-querier/src/dsl/display.rs | Implements the canonical serializer intended to round-trip through the string parser. |
| crates/ourios-querier/Cargo.toml | Adds serde, serde_json, and proptest dependencies for the structured surface + property testing. |
| Cargo.lock | Updates lockfile for new dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…erands Address the Copilot + CodeRabbit review on the RFC 0002 DSL parser slice: - flatten same-kind and/or at parse time via Predicate::and/or smart constructors so the IR is associative-normalised (a and (b and c) == a and b and c), and wrap a same-kind child in the serializer so a non-canonical tree still round-trips (RFC0002.7) - serialize floats as fixed-point digits.digits, never an exponent the lexer would reject; guard the contract with a debug_assert - reject a non-string first operand to matches/contains/starts_with/ ends_with (§6.1) via a shared require_string_operand, used by both surfaces (RFC0002.2) - structured surface: reject integers outside i64 instead of float coercion; reject negative severity numbers; reject a stray top-level by on count; reject empty and/or lists and flatten same-kind; validate sort.key as a §7 sort_key ident - lexer: reject literal newlines / control chars outside strings (single-line queries, §4 P7 / RFC0002.10) and name newlines in the in-string error - timestamp tail: require a real Z / ±hh:mm offset - strengthen RFC0002.8 to assert each error names the offending construct (still leak-free), and tighten the round-trip generator's string-fn operands to string-typed fields Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tured keys The RFC0002.7 round-trip proptest built an unbounded recursive predicate tree; generating/parsing/comparing it overflowed CI's ~2 MiB test-thread stack. Bound the generator with prop_recursive (depth 4, ~16 nodes), cap the leaf string/attr-key strategies, canonicalise the freely-generated tree to the parser/serialiser's flattened shape, and run the property on an explicit 32 MiB worker thread. parse(serialize(q)) == q still holds. Add deny_unknown_fields to the structured surface so malformed JSON errors instead of being silently accepted: RawQuery, RawFieldObject, the range/count/sort stage bodies, and (since serde forbids deny_unknown on untagged enums) each predicate-node variant via a dedicated struct. Narrow the mod.rs round-trip doc to the canonical IR the parsers produce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/dsl/structured.rs (1)
460-462:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the
renderstage body instead of discarding it.This arm accepts any
{"render": ...}payload and drops it on the floor becausebodyis never deserialized. For example,{"render":{"bogus":true}}currently succeeds even though that extra data is not representable in the IR and disappears after serialization.Suggested fix
#[derive(Deserialize)] #[serde(deny_unknown_fields)] struct RawSort { key: String, #[serde(default)] desc: bool, } + +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct RawRender {} impl RawStage { fn into_ir(self) -> Result<Stage, DslError> { @@ "render" => { reject_stray_by("render")?; + let _: RawRender = from_stage_body(body, "render")?; Ok(Stage::Render) } "sum" | "min" | "max" | "avg" => agg_into_ir(&agg_tag, body, by_value),🤖 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/dsl/structured.rs` around lines 460 - 462, The "render" match arm currently ignores the payload (`body`) and just returns Stage::Render, so stray fields like {"render":{"bogus":true}} are dropped; update the "render" arm to validate the body: either deserialize `body` into the expected render IR struct (e.g., a RenderOptions/RenderBody type) and return Stage::Render(render_struct), or if the render stage must be empty, call the existing helper (reject_stray_by or similar) passing the `body` to reject any unexpected fields; ensure you reference the `reject_stray_by` helper and the Stage::Render constructor in your change.
🤖 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/dsl/structured.rs`:
- Around line 460-462: The "render" match arm currently ignores the payload
(`body`) and just returns Stage::Render, so stray fields like
{"render":{"bogus":true}} are dropped; update the "render" arm to validate the
body: either deserialize `body` into the expected render IR struct (e.g., a
RenderOptions/RenderBody type) and return Stage::Render(render_struct), or if
the render stage must be empty, call the existing helper (reject_stray_by or
similar) passing the `body` to reject any unexpected fields; ensure you
reference the `reject_stray_by` helper and the Stage::Render constructor in your
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28d52d17-8b0d-4a85-aa1d-131d115b8864
📒 Files selected for processing (6)
crates/ourios-querier/src/dsl/display.rscrates/ourios-querier/src/dsl/ir.rscrates/ourios-querier/src/dsl/mod.rscrates/ourios-querier/src/dsl/parse.rscrates/ourios-querier/src/dsl/structured.rscrates/ourios-querier/tests/rfc0002_dsl.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/ourios-querier/src/dsl/mod.rs
- crates/ourios-querier/tests/rfc0002_dsl.rs
- crates/ourios-querier/src/dsl/display.rs
- crates/ourios-querier/src/dsl/parse.rs
Review pass 3: the structured render stage now rejects a non-empty body
({\"render\":1} / {\"render\":{...}}) — render is argument-less; the crate
doc no longer says the DSL is deferred (it now exposes pub mod dsl); the
display module doc no longer claims uXXXX-free escaping (it does emit
\uXXXX for control chars).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+597
to
+600
| Some(Tok::Number(n)) => { | ||
| let v = n.parse::<i64>().map_err(|_| { | ||
| DslError::new(format!("severity number {n:?} is not an integer")) | ||
| })?; |
Comment on lines
+93
to
+110
| fn name_to_field(name: &str) -> Result<Field, DslError> { | ||
| Some(match name { | ||
| "body" => Field::Body, | ||
| "severity" => Field::Severity, | ||
| "ts" => Field::Ts, | ||
| "observed_ts" => Field::ObservedTs, | ||
| "trace_id" => Field::TraceId, | ||
| "span_id" => Field::SpanId, | ||
| "scope" => Field::Scope, | ||
| "flags" => Field::Flags, | ||
| "service" => Field::Service, | ||
| "template_id" => Field::TemplateId, | ||
| "confidence" => Field::Confidence, | ||
| "lossy" => Field::Lossy, | ||
| _ => return Err(DslError::new(format!("unknown field name {name:?}"))), | ||
| }) | ||
| .ok_or_else(|| DslError::new(format!("unknown field name {name:?}"))) | ||
| } |
Comment on lines
+510
to
+512
| fn fields_to_ir(fields: Vec<RawField>) -> Result<Vec<Field>, DslError> { | ||
| fields.into_iter().map(RawField::into_ir).collect() | ||
| } |
Comment on lines
+78
to
+81
| // Inside an `and`, wrap a child `or` (lower binding). Canonical IR is | ||
| // flattened, so a same-kind child never appears here; wrap it anyway | ||
| // so a hand-built non-canonical tree still serialises to a string | ||
| // that re-parses to the same shape. |
This was referenced Jun 7, 2026
Merged
This was referenced Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SLICE 1 of the RFC 0002 logs query DSL (Branch B / surface β) lands in
ourios-querier: a newdslmodule with two front-ends over one IR, plus a canonical serializer that round-trips. This is the user-facing language in front of the (already-implemented) RFC 0007 execution layer.Hazard
CLAUDE.md§4.6 (no DataFusion/SQL leakage) is preserved: nodatafusion/arrow/SQL type appears in any public DSL signature, andDslErrormessages cite the offending token/clause without naming an engine/SQL construct.What's in the module (
crates/ourios-querier/src/dsl/)ir.rs— the typed query IR (Query/Predicate/Field/CmpOp/OrdOp/Value/SeverityValue/Call/Stage/AggFn/Time), mirroring the §7 grammar. Durations/timestamps are kept as validated lexical strings for this slice.parse.rs— a hand-rolled tokenizer + recursive-descent parser implementing §7 exactly: severity RHS restricted toseverity_name | numberwith ordering ops only (no regex on severity);str_fnarity + string-operand andresolves_to(number)checks; the §7 string escapes (\" \\ \n \t \r \uXXXX) with literal-newline/control rejection; bare-field validation; dotted vs bracketed attribute keys.structured.rs— serdeDeserializetypes for the §6.4 JSON surface (MCP/agent contract), converting to the sameQueryIR. Fields are a bare name or{resource|attr: key};severitycomparisons route to the severity predicate so both surfaces agree.display.rs— the canonical single-line β serializer such thatparse(serialize(q)) == q.mod.rs— the hand-rolledDslError(Display+std::error::Error, nothiserror, matching the repo'sQueryErrorstyle).Stubs flipped (
tests/rfc0002_dsl.rs)QueryIR (the one-core/two-surfaces invariant, §6.4).parse(serialize(q)) == q(4096-case run is green locally).severity =~ error, unknown field, unterminated string, literal newline, wrong fn arity, bare identifier as value, …) each return a specificDslError; the error string is asserted to contain nodatafusion/arrow/sqlsubstring.The other 8 stubs (RFC0002.1/.3/.4/.5/.6/.9/.10/.11) stay
#[ignore]'d — they need the DataFusion compile / published JSON schema in later slices.Design decisions (flagging grammar ambiguities rather than silently inventing)
1h(duration) and"1h"(string) are different tokens, but a structured comparisonvalueis a JSON primitive, so a JSON string maps toValue::Str. Durations/timestamps remain distinct exactly where the grammar needs them — therange(...)bounds, carried as lexical strings parsed intoTimevia the shared time grammar. RFC0002.2 equivalence is over the queries both surfaces can express; a duration-valued comparison is a string-DSL-only construct. (Documented at the top ofstructured.rs.)agg_fn(path) [by …]is rendered as{"<fn>": <path>}with an optional sibling"by": [...]. Untagged enums interact badly with that fn-as-key shape, so stages are deserialized as a tagged object and dispatched by hand (the kind = the non-bykey), which also yields precise, leak-free errors on a bad tag.and/or/not) over the terse aliases; dotted attr keys when every segment is a bare identifier, else bracketed; integer-valued floats serialized with a forced.0so they re-lex asFloat;sortalways prints an explicitasc/desc.Verification (run locally)
cargo fmt --all --check— cleancargo clippy -p ourios-querier --all-targets --all-features -- -D warnings— cleancargo test -p ourios-querier— 36 lib unit tests + 3 flipped acceptance tests pass; 8 acceptance stubs stay ignored; existing querier tests unchanged🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests