feat(querier): drift query over the audit stream (RFC 0010, H5.3) - #165
Merged
Conversation
Implement RFC 0010's `drift from <t1> to <t2>` verb head: a closed,
audit-stream query that answers "which templates gained a version in the
half-open window [from, to)". Discharges RFC 0001 scenario H5.3.
- DSL surface: a top-level `Statement` enum (Logs | Drift) makes the two
query shapes mutually exclusive (RFC 0010 §6.1, make-invalid-states-
unrepresentable). The string head `drift from <t> to <t>` and the
structured `{"drift":{"from","to"}}` object both lower to one
`DriftQuery` IR, reusing the RFC 0002 §7 `time` production verbatim and
foreclosing trailing `|` stages. Published JSON Schema gains a `drift`
fragment alongside the log query.
- Audit-tree scan + drift aggregation: a new `drift` module scans
`audit/tenant_id=<enc>/` (tenancy is a partition prune, RFC0010.4 /
§3.7), day-granularity window prune over the RFC 0005 audit layout, and
lowers to a DataFusion Filter → Aggregate → Sort plan: event_type IN
(template_widened, template_type_expanded), timestamp in [from, to),
GROUP BY template_id → widening_count/min(old_version)/max(new_version)/
first_seen/last_seen, ORDER BY widening_count DESC, template_id ASC
(RFC 0010 §6.3/§6.6). No SQL/DataFusion type crosses the public surface
(H6 / RFC0010.8) — `DriftRow`/`DriftResult` carry plain scalars.
- Tests: RFC0010.1–.8 acceptance tests in ourios-querier/tests/drift.rs,
seeded via the production ParquetAuditSink. H5.3 relocated here from the
miner crate (which can't run a querier, cf. RFC0001.5/.6); the miner
hazards stub's #[ignore]/todo!() is replaced with a pointer comment.
The general audit aggregation and the §9 finer forks stay deferred per
RFC 0010 §3.2/§8.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 32 minutes and 56 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 (1)
📝 WalkthroughWalkthroughAdds RFC 0010 "drift" queries: new Statement/DriftQuery IR, text and JSON parsing/serialization, DataFusion-based execution over tenant audit Parquet with pruning and path-safety, public Querier::run_drift API, and unit/integration tests validating behavior. ChangesRFC 0010 Drift Query Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Querier
participant WindowResolver
participant AuditScanner
participant DataFusion
participant Decoder
Client->>Querier: run_drift(query, tenant, now_unix_nano)
Querier->>WindowResolver: resolve [from,to) nanoseconds
Querier->>AuditScanner: discover/prune tenant parquet files
AuditScanner->>Querier: validated file list
Querier->>DataFusion: execute filter+aggregate plan
DataFusion->>Decoder: return record batches
Decoder->>Querier: decoded DriftRow list + stats
Querier->>Client: DriftResult{rows, stats}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Implements RFC 0010’s drift audit-stream query surface in ourios-querier, including DSL/structured parsing, compilation+execution over the per-tenant audit/ Parquet stream, and acceptance tests (also discharging RFC 0001 scenario H5.3 via relocation into querier tests).
Changes:
- Adds a new top-level DSL
Statementwith a closed-formdrift from <t1> to <t2>query (DriftQuery) alongside existing log queries. - Implements drift execution over the audit stream (DataFusion plan: filter → aggregate → sort) and exposes
Querier::run_driftplusDriftResult/DriftRow. - Adds RFC0010.1–.8 acceptance tests and structured-query JSON schema support for drift statements.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-querier/tests/drift.rs | New RFC0010 acceptance tests + relocated RFC0001 H5.3 drift scenario. |
| crates/ourios-querier/tests/common/mod.rs | Adds audit-stream event fixtures and a helper to write real audit Parquet data for drift tests. |
| crates/ourios-querier/src/lib.rs | Wires in drift module, re-exports drift result types, and adds Querier::run_drift. |
| crates/ourios-querier/src/dsl/structured.rs | Adds parse_structured_statement to support structured drift statements; keeps parse_structured log-only. |
| crates/ourios-querier/src/dsl/structured_query.schema.json | Updates schema root to oneOf log-query vs drift-query statement shapes. |
| crates/ourios-querier/src/dsl/parse.rs | Introduces parse_statement for drift/log dispatch; keeps parse log-only and drift-rejecting. |
| crates/ourios-querier/src/dsl/mod.rs | Exposes new statement-level parsing/serialization APIs and IR types. |
| crates/ourios-querier/src/dsl/ir.rs | Adds Statement and DriftQuery IR to make drift/log shapes unambiguous. |
| crates/ourios-querier/src/dsl/display.rs | Adds canonical serialization for drift/log statements (serialize_statement). |
| crates/ourios-querier/src/drift.rs | New drift compile+exec implementation over audit Parquet files with pruning and result decoding. |
| crates/ourios-querier/src/compile.rs | Makes resolve_time pub(crate) for reuse by drift window resolution. |
| crates/ourios-miner/tests/hazards.rs | Replaces the ignored H5.3 stub with a pointer to the relocated querier drift test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…iles Mirror the log-query path: every drift audit file must canonicalize under the tenant's canonical audit/tenant_id=… root, so a symlinked *.parquet cannot resolve into another tenant's tree (RFC0010.4 / §3.7). Adds a unix symlink-escape test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…not Display QueryError::Storage Display hides detail (H6 — no DataFusion leak), so the symlink-escape test now matches the variant and asserts detail contains "escapes tenant partition root" — confirming the tenant-root backstop fired rather than an unrelated read error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/ourios-querier/src/dsl/display.rs (1)
16-33: ⚡ Quick winAdd a colocated round-trip test for the new statement serializer.
serialize_statementis now the public entrypoint for the new drift statement shape, but this module still only exercisesQueryround-trips. A smallStatement::Driftserialize→parse_statementassertion here would protect the new branch and keep the coverage next to the code that owns it.Suggested test
#[cfg(test)] mod tests { - use crate::dsl::{parse, serialize}; + use crate::dsl::{parse, parse_statement, serialize, serialize_statement}; + use crate::dsl::ir::{DriftQuery, Statement, Time}; /// A small, diverse corpus that exercises every production; each must /// round-trip through serialize→parse to the same IR. @@ fn floats_serialise_without_an_exponent() { // A small-magnitude float must serialise as fixed-point `digits.digits` // (the lexer rejects an exponent form), and round-trip. use crate::dsl::ir::{CmpOp, Field, OrdOp, Predicate, Query, Value}; for f in [0.000_000_001_f64, 0.000_25, 123_456.789] { @@ assert_eq!(parse(&s).unwrap(), q, "round-trip failed for {s:?}"); } } + + #[test] + fn drift_statement_round_trips_through_canonical_form() { + let statement = Statement::Drift(DriftQuery { + from: Time::Duration { + neg: true, + literal: "7d".into(), + }, + to: Time::Now, + }); + + let serialized = serialize_statement(&statement); + let reparsed = parse_statement(&serialized).unwrap(); + + assert_eq!(reparsed, statement); + } }As per coding guidelines,
crates/**/src/**/*.rs: Unit tests must be colocated next to the code for anything non-trivial.🤖 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/display.rs` around lines 16 - 33, Add a colocated unit test in this file that constructs a DriftQuery (or Statement::Drift wrapping one), calls serialize_statement on it, then calls super::parse_statement (or parse_statement) to parse the produced string and asserts the parsed Statement equals the original; reference serialize_statement and Statement::Drift for the round‑trip, and you can reuse helper functions/types like DriftQuery and write_time/serialize_drift to build the expected serialized form if needed. Ensure the test is annotated with #[cfg(test)] and #[test] and lives in the same module so it exercises the serialize_statement -> parse_statement round‑trip for the drift branch.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.
Inline comments:
In `@crates/ourios-querier/src/drift.rs`:
- Around line 193-220: Move the day-window prune so it's checked before
performing expensive directory reads: inside the loop that processes
`stack.pop()`, call `day_partition_in_window(&dir, start, end)` first and if it
returns false, skip listing `dir` (i.e., continue) so you avoid calling
`std::fs::read_dir(&dir)` for out-of-window `day=...` directories; preserve
current behavior of only appending `parquets` when in-window (the
`files.append(&mut parquets)` call) and still push subdirectories for non-day or
unparseable paths as needed.
In `@crates/ourios-querier/src/dsl/parse.rs`:
- Around line 1497-1504: The test drift_is_not_a_reserved_field_in_a_log_query
currently parses "true | limit 5" and never exercises the 'drift' token; change
it to call parse_statement with a log pipeline that contains the identifier
drift after the head (e.g. "true | where drift == 1" or "true | stats count by
drift") and update the assertion to expect a parsing failure (Err) for an
unknown field rather than successful Statement::Logs parsing — specifically
invoke parse_statement("...") and assert it returns an Err and that the error
indicates an unknown/unknown-field for "drift" so the test fails if 'drift' is
treated specially as a statement head.
In `@crates/ourios-querier/src/lib.rs`:
- Around line 491-498: The Display message for the QueryError::Storage variant
is store-specific and should be made generic before exposing run_drift; update
the Display implementation for QueryError (the branch that currently returns
"failed to read the log store") to a store-neutral message such as "storage
operation failed" or "failed to access storage" and then update the
pinned-message tests in this file that assert the old string to expect the new
generic text; locate the Display impl for QueryError, the Storage variant, and
the pinned message tests in this file and change both the emitted message and
test expectations accordingly so run_drift (and drift::run_drift) no longer
point operators to the wrong subsystem.
---
Nitpick comments:
In `@crates/ourios-querier/src/dsl/display.rs`:
- Around line 16-33: Add a colocated unit test in this file that constructs a
DriftQuery (or Statement::Drift wrapping one), calls serialize_statement on it,
then calls super::parse_statement (or parse_statement) to parse the produced
string and asserts the parsed Statement equals the original; reference
serialize_statement and Statement::Drift for the round‑trip, and you can reuse
helper functions/types like DriftQuery and write_time/serialize_drift to build
the expected serialized form if needed. Ensure the test is annotated with
#[cfg(test)] and #[test] and lives in the same module so it exercises the
serialize_statement -> parse_statement round‑trip for the drift branch.
🪄 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: c51cec33-a889-4b5c-bc0c-29d826052df7
📒 Files selected for processing (12)
crates/ourios-miner/tests/hazards.rscrates/ourios-querier/src/compile.rscrates/ourios-querier/src/drift.rscrates/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/dsl/structured_query.schema.jsoncrates/ourios-querier/src/lib.rscrates/ourios-querier/tests/common/mod.rscrates/ourios-querier/tests/drift.rs
…neutral storage error Restructure audit_files_in_window so an out-of-window day= leaf is skipped before it is listed (the prune now gates read_dir, not just the file append); year=/month=/tenant_id= dirs are non-leaf and still descended. De-duplicate canonical audit paths so an in-tenant symlink can't double-read/double-count, mirroring the log path. Short-circuit an empty [from, to) window before any audit-tree IO or DataFusion planning. Reword the QueryError::Storage Display to the store-neutral "failed to read storage" now that drift surfaces it for the audit stream too, still without leaking the detail (H6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t token in its guard test The 'drift is not a log query' message used a \ line-continuation that preserved source indentation as runs of spaces in the operator-facing text; build it with concat! so it renders with single spaces. The mid-query guard test now parses a query that actually contains a drift token after the head (attr.drift), asserting it stays a normal Logs query so the test fails if a mid-query drift were ever treated specially. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…as zero decode_drift_rows read .value(i) without NULL checks, but the audit schema marks template_id/old_version/new_version (and the timestamp aggregates) nullable — a corrupt/foreign audit file could decode a NULL group key as template_id 0. Guard each and surface QueryError::Storage; add a decode unit test on a NULL group key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
Implements RFC 0010 — Audit-stream queries & template drift (the
driftverb head + audit-tree scan + the §6.3 fixed aggregation) and discharges RFC 0001 scenario H5.3. RFC 0010 was just merged (#164) and is the contract this PR codes to.The thesis gap this closes: RFC 0001 §6.7 specified drift detection as SQL "for spec clarity"; RFC 0002 §6.3 deferred the audit-stream query path; RFC 0005 already persists the events. RFC 0010 is that future capability — a single closed DSL query head over the per-tenant
audit/Parquet stream.Drift surface as implemented
dsl::ir) — a top-levelenum Statement { Logs(Query), Drift(DriftQuery) }. ADriftQuery { from: Time, to: Time }is its own shape, not a flag on a row query (make-invalid-states-unrepresentable, RFC 0010 §6.1).from/toreuse the RFC 0002 §7Timeproduction verbatim.dsl::parse) —parse_statementdispatches on the leadingdrifttoken:drift from <t1> to <t2>. The head admits no trailing|stages (closed form, §6.1). The RFC 0002parseentry stays log-only and rejects a drift head.dsl::structured) —parse_structured_statementaccepts{"drift":{"from":…,"to":…}}(deny-unknown-fields, nopredicate/stagessibling). The published JSON Schema gains adrift_queryfragment under a rootoneOf.driftmodule) — lowersDriftQueryto a DataFusionFilter → Aggregate → Sortplan over the audit files:event_type IN ('template_widened','template_type_expanded')ANDtimestampin half-open[from, to), GROUP BYtemplate_id→widening_count=COUNT(*),MIN(old_version),MAX(new_version),MIN(timestamp)(first_seen),MAX(timestamp)(last_seen), ORDER BYwidening_countDESC,template_idASC. Built programmatically — no raw SQL.DriftResult { rows: Vec<DriftRow>, stats: QueryStats }, a distinct shape from the logQueryResult(RFC 0010 §6.4).DriftRowcarries plain owned scalars; nodatafusion/arrow/SQL type crosses the public surface.Querier::run_drift(&DriftQuery, &TenantId, now_unix_nano).Audit-tree scan + per-tenant prune
The scan roots at
<bucket>/audit/tenant_id=<percent-encoded tenant>/— tenant isolation is the partition prune (the leading Hive key, RFC 0005 §3.4 / §6.5), not a post-scan filter. The window drives a day-granularityyear/month/dayprune (the audit layout has nohoursegment) that skips whole partition dirs that can't overlap[from, to)before any footer opens; an exacttimestamppredicate then trims the boundary days. Conservative: an unparseable/non-leaf path is never pruned, so the row-leveltimestampfilter stays the correctness authority. A missing tenant dir is an empty result (RFC0010.5), not an error.Hazards / invariants addressed
template_widenedevents surfaces asdrift from -7d to now.parse_statement → DriftQuery → DriftRow.tenantarg is required).Tests
crates/ourios-querier/tests/drift.rs(greppable/// Scenario RFC0010.Ndoc-comments), seeded via the productionParquetAuditSink: drifted-templates-with-counts, half-open boundary exclusion, event_type scoping (rejected_degenerate/compaction excluded), tenant isolation, empty→empty, ordering + tie-break, aggregate version/time bounds, no-SQL-leakage.h5_3_drift_query_returns_templates_that_gained_a_version) lives intests/drift.rs(the miner crate can't run a querier, cf. RFC0001.5/.6 already relocated torfc0001_query_semantics.rs). The minerhazards.rs#[ignore]/todo!()stub is replaced with a one-line pointer comment.Out of scope (per RFC 0010 §3.2 / §8)
The general audit-stream aggregation engine, the
compaction-event query surface, and the §9 finer forks (default window, tie-break alternatives, etc.) stay deferred exactly as the RFC says.Verification
Reproduced CI locally:
cargo test --all-features→ 577 passed; 0 failed; 29 ignored (the 29 are pre-existing red-gate stubs in other RFCs; H5.3 is no longer among them).cargo fmt --all --checkclean.cargo clippy --all-targets --all-features -- -D warningsclean. No docs touched, somdbook buildnot required.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Bug Fixes