feat(parquet): Store listing wrapper for the RFC 0019 querier/compactor migration - #290
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesStore listing capability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 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 |
Add `Store::list_blocking(prefix)` (over `ObjectStore::list`, drained via the off-runtime bridge like the other blocking methods): recursive, prefix-scoped, returns store-relative keys. This is the seam the querier and compactor will walk instead of `std::fs` (RFC 0019 §3.3) so the same enumeration targets `LocalFileSystem` or S3 — the shared prerequisite for slices 2a/2b. Adds a `futures` dep (TryStreamExt::try_collect, already in the tree via object_store). Tested: a local unit test (prefix-scoped / whole-store / empty), plus a localstack-ignored S3 test proving listing on the real AmazonS3 backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jensholdgaard
force-pushed
the
rfc0019-green-querier-store
branch
from
June 23, 2026 12:11
dfbacd3 to
ec5f055
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-parquet/src/store.rs`:
- Around line 422-430: Remove the claim about lexicographic ordering from the
documentation comment for the list method. The doc comment currently states
results are returned "in the backend's lexicographic order," but object_store
0.13's list method does not guarantee any specific ordering. Update the first
sentence of the doc comment to remove the ordering guarantee and replace it with
a statement that the order of results is unspecified, such as "List every object
key under `prefix` (store-relative), recursively. The order of results is
unspecified."
🪄 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: 1f0748a3-0a65-461d-8213-51a32349f542
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/ourios-parquet/Cargo.tomlcrates/ourios-parquet/src/store.rscrates/ourios-parquet/tests/rfc0013_object_store.rs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a Store-level listing API to support RFC 0019’s querier/compactor migration away from direct std::fs traversal, allowing both local and S3 backends to be enumerated through the same abstraction.
Changes:
- Added
Store::list_blocking(prefix)implemented overObjectStore::listand bridged through the existing off-runtime blocking mechanism. - Added unit + ignored localstack integration tests verifying recursive, prefix-scoped, store-relative key enumeration.
- Added a direct
futuresdependency forTryStreamExt::try_collect.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/ourios-parquet/src/store.rs | Adds async listing + blocking wrapper and a local unit test for prefix-scoped enumeration. |
| crates/ourios-parquet/tests/rfc0013_object_store.rs | Adds an ignored localstack S3 integration test for list_blocking. |
| crates/ourios-parquet/Cargo.toml | Adds futures dependency for draining the listing stream. |
| Cargo.lock | Records the new direct dependency edge on futures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jensholdgaard
added a commit
that referenced
this pull request
Jun 23, 2026
…tore::list (Copilot/CodeRabbit #290) - Sort the keys before returning so the order is a deterministic lexicographic contract, not the backend's stream order (neither LocalFileSystem nor S3 guarantees one); reword the doc to "lexicographic order" (drop "backend's"). - Strip the prefix the correct direction (`location.prefix_match(root)`) and `filter_map` out non-matching entries instead of falling back to an absolute key — so an S3 string-prefix sibling (`ourios2/…` under `ourios`) can't leak. - Tests (unit + localstack) now assert the returned order directly, no test-side sort, so an ordering regression fails the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e::list (#290) - Sort the keys before returning so the order is a deterministic lexicographic contract, not the backend's stream order (neither LocalFileSystem nor S3 guarantees one); reword the doc to "lexicographic order". - Strip the prefix the correct direction (`location.prefix_match(root)`) and `filter_map` out non-matching entries instead of falling back to an absolute key — so an S3 string-prefix sibling (`ourios2/…` under `ourios`) can't leak. - Tests (unit + localstack) assert the returned order directly, no test-side sort, so an ordering regression fails the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jensholdgaard
force-pushed
the
rfc0019-green-querier-store
branch
from
June 23, 2026 20:48
9fca0b6 to
0447a69
Compare
…290) S3's `list` does string-prefix matching, so listing `tenant_id=a` can return the sibling `tenant_id=ab/…` — a tenant-isolation leak (RFC0019.5). The filter checked only the (empty) store root, not the requested prefix. Now gate each object on `prefix_match(&scoped)` (segment-wise, so siblings are excluded) before stripping the root. Tests (unit + localstack) add a `tenant_id=ab` fixture: the scoped list excludes it, the unscoped list includes it (in lexicographic order, `tenant_id=a/` before `tenant_id=ab/`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rd (Copilot #290) - The localstack `list_blocking(None)` assertion now includes the `tenant_id=ab` fixture (in lexicographic order), so the whole-bucket listing is complete and a dropped-key regression would fail. - Reword the `prefix_match(&scoped)` comment: binding to `_` marks the #[must_use] iterator used, it doesn't "consume" it.
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
RFC 0019 slice-2 prerequisite: a
Storelisting method, the shared seam the querier (2a) and compactor (2b) need to enumerate partitions/files on object storage instead ofstd::fs.How
Store::list_blocking(prefix: Option<&str>) -> Result<Vec<String>, StoreError>overObjectStore::list, drained withTryStreamExt::try_collectand run through the existing off-runtime bridge (likeget_blocking/put_blocking). Recursive, prefix-scoped, returns store-relative keys (the same key space asget/put) — so the same enumeration targetsLocalFileSystem(dev/test) or S3 (RFC 0019 §3.3).futuresdep (onlyTryStreamExt; already in the tree transitively viaobject_store).Tests
store::tests::list_blocking_enumerates_keys_under_a_prefix): prefix-scoped → only that tenant's objects; no prefix → whole store; non-matching prefix → empty.store_list_enumerates_keys_on_s3—#[ignore]d localstack test proving recursive, prefix-scoped, store-relative listing on the realAmazonS3backend (runs in thes3-integrationCI job).cargo clippy -p ourios-parquet --all-targets --all-features -- -D warningsclean;cargo fmt --all --checkgreen.Note
No
§5scenario flips green here — this is an enabling step. RFC 0019 staysred. Next: slice 2a migrates the querier read path (DataFusionregister_object_store+ the audit/alias/template-registry helpers) ontoStore, then 2b the compactor (+publish_cas), then slice 3 the localstack e2e (.2/.3/.4/.5) → green. Tenant isolation (RFC0019.5) is preserved structurally — the querier always builds the explicittenant_id=<enc>/path.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
list_blocking, with optional prefix filtering and deterministic lexicographic ordering. Returned keys are store-relative.Tests
list_blockingagainst an S3-compatible backend.