feat(querier): execute minimal tenant/time/template queries via DataFusion (slice 1) - #87
Merged
Merged
Conversation
…usion (slice 1) Querier::run now actually runs. It roots a DataFusion ListingTable at the request tenant's data/tenant_id=<enc>/ partition dir (structural tenant isolation — RFC0007.5), declares year/month/ day/hour as path-only Hive partition cols (tenant_id stays a plain file column relative to that root), filters on time_unix_nano range + template_id, collects, and returns the matching-row count. DataFusion/arrow types stay strictly internal — only Ourios types cross the public API (§4.6); datafusion errors map to QueryError::Storage. QueryRequest gains template_id; QueryResult gains rows; Querier::new takes the bucket_root. datafusion 53 is a normal dep (pulls arrow/parquet 58 alongside ourios-parquet's 55 — harmless, they only meet on disk). Live tests (tests/execution.rs): tenant + template + half-open time-range filtering counts, and RFC0007.5 tenant isolation + empty-tenant. Pruning stats (RFC0007.1 / B1) and the B2 latency bench are the next slices; their stubs stay #[ignore]'d. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements Querier::run using DataFusion over tenant-scoped Hive-partitioned Parquet directories, adds ourios-parquet and test dev-deps, extends QueryRequest with template_id and QueryResult with rows, updates QueryError display, and adds integration tests validating filtering, counting, and tenant isolation. ChangesQuerier Execution Implementation
Sequence DiagramsequenceDiagram
participant Client
participant Querier
participant DataFusion
participant ParquetStore
Client->>Querier: run(QueryRequest)
Querier->>Querier: build tenant path (bucket_root/data/tenant_id=<encoded>/)
Querier->>ParquetStore: check tenant directory
alt tenant dir exists
Querier->>DataFusion: register ListingTable (year/month/day/hour)
alt time_range present
Querier->>DataFusion: apply time_unix_nano → TimestampNanosecond filter
end
alt template_id present
Querier->>DataFusion: apply template_id equality filter
end
Querier->>DataFusion: execute count (df.count().await)
DataFusion-->>Querier: row counts
else tenant dir missing
ParquetStore-->>Querier: not found
Querier->>Client: return empty QueryResult
end
Querier-->>Client: QueryResult { rows, stats }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the first end-to-end execution “slice” for ourios-querier: Querier::run now runs a minimal predicate query (tenant + optional time range + optional template_id) against the RFC 0005 Parquet layout using DataFusion, returning a matching row count while keeping DataFusion/Arrow types out of the public API.
Changes:
- Implement
Querier::runexecution via DataFusionListingTable, including tenant-rooted table setup and filter application. - Extend the public throwaway query surface (
QueryRequest.template_id,QueryResult.rows,Querier::new(bucket_root)). - Add live execution integration tests (including RFC0007.5 tenant isolation) and wire in required dependencies.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-querier/src/lib.rs | Implements DataFusion-backed execution, adds request/result fields, and enforces tenant isolation by table rooting. |
| crates/ourios-querier/Cargo.toml | Adds datafusion + ourios-parquet dependency and test-only deps for integration testing. |
| crates/ourios-querier/tests/execution.rs | New integration tests that write Parquet fixtures and validate row-count results and tenant isolation. |
| crates/ourios-querier/tests/acceptance.rs | Removes the ignored RFC0007.5 stub and points to the new live execution test. |
| Cargo.lock | Locks new transitive dependencies introduced by DataFusion and test dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib.rs`:
- Around line 176-177: The ListingTableUrl construction currently builds a
file:// URI from tenant_dir (ListingTableUrl::parse(format!("file://{}/",
tenant_dir.display()))) which should instead pass a scheme-less local directory
path (preserve the trailing "/") so DataFusion handles encoding correctly;
update the call to construct the path from tenant_dir (with trailing slash)
before calling ListingTableUrl::parse and keep the existing map_err(storage_err)
behavior. In Querier::run replace the materializing count pattern that does
df.collect().await and sums batch.num_rows() with the DataFusion aggregate API
by calling let rows = df.count().await?; and return that aggregated count
instead of iterating collected batches.
- Around line 228-234: The current code calls df.collect().await and sums
RecordBatch::num_rows(), which materializes all rows; instead build and run a
global aggregate on the DataFrame to compute COUNT(*) (use df.aggregate with an
expression like count(lit(1)) or equivalent), execute the plan as a stream
(e.g., execute_stream or similar DataFusion executor) and read the single scalar
from the first output batch/row, then set QueryResult.rows to that scalar u64;
update the code paths around df.collect(), batches, and rows so only the scalar
result is read and missing/empty results are handled safely before returning
QueryResult with stats unchanged.
🪄 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: 2c0bde0d-43c9-4b79-b4b7-f198c78b7124
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/ourios-querier/Cargo.tomlcrates/ourios-querier/src/lib.rscrates/ourios-querier/tests/acceptance.rscrates/ourios-querier/tests/execution.rs
…a DataFusion (slice 1)
…a DataFusion (slice 1)
…a DataFusion (slice 1)
…a DataFusion (slice 1)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-querier/tests/execution.rs (1)
150-158: ⚡ Quick winHardcoded partition path: only the tenant-scoped root controls the tmp-only empty-result behavior.
Querier::runroots theListingTableat<bucket_root>/data/tenant_id={percent_encode_tenant(tenant)}and returns empty when there are no published*.parquetunder that tenant dir—even if it contains only*.parquet.tmp; so the hardcodedyear=/month=/day=/hour=subpath isn’t what determines whether the test hits the tmp-only branch.- The real coupling is that
tenant_id=amust matchpercent_encode_tenant("a"); consider deriving the on-disk path via the sameWriter/PartitionKey::deriveand then renaming the emitted<uuid>.parquetto<uuid>.parquet.tmpso the test stays aligned with any future layout changes.🤖 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/tests/execution.rs` around lines 150 - 158, The test hardcodes a deep partition path instead of using the same tenant-root logic as Querier::run, so change the setup to derive the on-disk path using the same utilities: call percent_encode_tenant("a") (or reuse PartitionKey::derive/Writer used by producers) to compute the tenant-scoped root (the same root ListingTable is rooted at in Querier::run), create that directory and then write a file by taking a real emitted "<uuid>.parquet" name from the Writer/PartitionKey generation and renaming it to "<uuid>.parquet.tmp" (or create the tmp name from the generated uuid) so the test hits the tmp-only branch even if layout changes; reference Querier::run, ListingTable, percent_encode_tenant, Writer, and PartitionKey::derive to locate the relevant helpers.
🤖 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.
Nitpick comments:
In `@crates/ourios-querier/tests/execution.rs`:
- Around line 150-158: The test hardcodes a deep partition path instead of using
the same tenant-root logic as Querier::run, so change the setup to derive the
on-disk path using the same utilities: call percent_encode_tenant("a") (or reuse
PartitionKey::derive/Writer used by producers) to compute the tenant-scoped root
(the same root ListingTable is rooted at in Querier::run), create that directory
and then write a file by taking a real emitted "<uuid>.parquet" name from the
Writer/PartitionKey generation and renaming it to "<uuid>.parquet.tmp" (or
create the tmp name from the generated uuid) so the test hits the tmp-only
branch even if layout changes; reference Querier::run, ListingTable,
percent_encode_tenant, Writer, and PartitionKey::derive to locate the relevant
helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52cbd0fa-56c9-4ffa-85c3-abcad80a70f9
📒 Files selected for processing (2)
crates/ourios-querier/src/lib.rscrates/ourios-querier/tests/execution.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/ourios-querier/src/lib.rs
…a DataFusion (slice 1)
…s (slice 1) Closes the two test gaps from PR review where a behavior fix lacked a test (per the test-per-fix discipline): - bucket_path_with_spaces_resolves — a bucket path with a space queries correctly (the canonicalize + scheme-less URL fix). - read_dir_error_surfaces_as_storage_not_empty — a tenant path that is a file (ENOTDIR, not NotFound) surfaces as QueryError::Storage rather than being masked as empty. The other accepted fixes were already covered (count() by the existing count asserts, §4.6 Display by the colocated test, empty-store by tenant_dir_without_committed_parquet_is_empty) or exempt (doc-only, refactor to columns:: constants). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6 tasks
This was referenced Jun 2, 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.
What
Slice 1 of the querier execution path —
Querier::runnow genuinely executes against the RFC 0005 Parquet store via DataFusion. This is the first time the read path runs end-to-end, and it's the start of answering the query thesis (B1/B2) that actually justifies the architecture.Per the maintainer decision, the query surface is throwaway (minimal predicates, no RFC 0002 DSL) until B1/B2 prove it's worth a stable language.
How it works
ListingTableat the request tenant'sdata/tenant_id=<enc>/dir — usingourios-parquet's canonicalpercent_encode_tenantso the path matches the writer byte-for-byte. Tenant isolation is structural (RFC0007.5): no other tenant's files are reachable.year/month/day/hourare declared as path-only Hive partition cols;tenant_idstays a plain file column relative to that root (resolving the "name is both partition + file column" conflict).time_unix_nanohalf-open range (typed asTimestamp(ns, UTC)to match the schema) +template_idequality; collects; returns the matching-row count.datafusion/arrow/SQL type in any public signature; datafusion errors map toQueryError::Storage.API (throwaway surface)
QueryRequestgainstemplate_id: Option<u64>;QueryResultgainsrows: u64;Querier::new(bucket_root).Deps
datafusion 53is now a normal dep (pulls arrow/parquet 58 alongside ourios-parquet's 55). Harmless: the two never meet in memory — DataFusion reads Parquet files from disk that ourios-parquet wrote.ourios-parquetpromoted to a normal dep forpercent_encode_tenant.Tests (live)
tests/execution.rs, writing fixtures viaourios_parquet::Writer:executes_and_counts_matching_rows— tenant + template-exact + half-open time-range filtering all return correct counts.rfc0007_5_tenant_isolation— each tenant sees only its rows; an unknown tenant is an empty result, not an error.RFC0007.5flipped from#[ignore]stub to live. Workspace: 329 passed / 0 failed / 48 ignored; fmt/clippy/mdbook green.Next slices
row_groups_scanned/pruned+bytes_readfrom the DataFusionParquetExecmetrics intoQueryStats, flip RFC0007.1 live.ourios-benchagainstcorpus/otel-demo-v*→ the first query-thesis number.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores