Skip to content

feat(parquet): Store sized listing + blocking delete for the RFC 0019 compactor migration - #293

Merged
jensholdgaard merged 3 commits into
mainfrom
rfc0019-store-sized-list-delete
Jun 27, 2026
Merged

feat(parquet): Store sized listing + blocking delete for the RFC 0019 compactor migration#293
jensholdgaard merged 3 commits into
mainfrom
rfc0019-store-sized-list-delete

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jun 27, 2026

Copy link
Copy Markdown
Owner

What

Two backend-agnostic Store primitives the RFC 0019 compactor migration (slice 2b) needs and the querier path (#290/#291) didn't:

  • list_with_sizes_blocking(prefix) -> Vec<(String, u64)> — the compactor's small-file candidate check needs each object's byte length; the backend already reports it in the listing (ObjectMeta::size), so it comes for free here rather than via a per-object head. The listing core is factored into a shared list_entries, so list and the sized variant share the segment-wise tenant-isolation gate (RFC0019.5) and the lexicographic key ordering — no duplicated filter logic.
  • delete_blocking(key) — the compactor's orphan / post-commit input GC. A thin bridge over the async delete; it adds no existence check, so missing-key behaviour is backend-dependent: LocalFileSystem maps an absent key to is_not_found, while S3 DELETE is idempotent and returns success. The compactor's GC loops treat both as "already reclaimed" (match is_not_found, otherwise count a failure), so the difference is invisible to them — no head round-trip is added to force a uniform contract.

Both are thin block_on_off_runtime bridges over the existing async core.

Why

Foundational, independently-mergeable prereq for the compactor's move off raw std::fs onto Store (so data compaction can target S3 as well as local), the analog of #290 (list_blocking) / #291 (AuditReader::open_bytes) before the querier migration (#292).

Invariants (CLAUDE.md §3.7)

Tenant isolation is preserved: list_with_sizes_blocking reuses the same segment-wise prefix_match gate as list_blocking, so a string-prefix sibling (tenant_id=ab when listing tenant_id=a) is excluded. No on-disk format change (§3.5). No behavior change to existing call sites.

Tests

  • Local unit tests: list_with_sizes_reports_byte_lengths_in_key_order (sizes + ordering + sibling exclusion), delete_blocking_removes_and_local_missing_is_not_found (local backend's not-found-on-absent).
  • Localstack-#[ignore]d S3 tests (run in the s3-integration CI job): store_list_with_sizes_reports_byte_lengths_on_s3, store_delete_blocking_removes_on_s3 (asserts a redundant delete is tolerated — success or not-found — matching S3's idempotent DELETE, so it won't fail against real AWS).

Local gate

cargo fmt --all --check, cargo clippy -p ourios-parquet --all-targets --all-features -D warnings, RUSTDOCFLAGS=-D warnings cargo doc -p ourios-parquet, cargo test -p ourios-parquet — all green. S3 tests run in CI's s3-integration job.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jensholdgaard, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 54 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e6ef1823-bb13-4cee-b5cf-4771174ad30a

📥 Commits

Reviewing files that changed from the base of the PR and between e77b887 and c6c7c32.

📒 Files selected for processing (1)
  • crates/ourios-parquet/tests/rfc0013_object_store.rs
📝 Walkthrough

Walkthrough

Store listing now returns (key, size) tuples, adds blocking wrappers for size-aware listing and delete, and extends tests for ordering, prefix isolation, byte sizes, and missing-key deletes.

Changes

Store blocking bridges and size-aware listing

Layer / File(s) Summary
Internal size-aware listing
crates/ourios-parquet/src/store.rs
Store::list now derives keys from list_entries, which filters matching metas into sorted (key, size) tuples while preserving tenant-prefix gating.
Blocking wrappers
crates/ourios-parquet/src/store.rs
list_with_sizes_blocking and delete_blocking expose the async listing and delete paths through synchronous wrappers.
Validation
crates/ourios-parquet/src/store.rs, crates/ourios-parquet/tests/rfc0013_object_store.rs
Store tests and ignored S3 integration tests cover size-aware listing output, tenant-prefix exclusion, and not-found delete behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

I hop through prefixes, soft and neat,
with sizes tucked in each parquet treat.
A missing key? The burrow knows,
not-found puffs where deletion goes. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: sized listing plus blocking delete for the parquet Store.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description is detailed and covers what, why, invariants, tests, and validation; only the template headings/Related section are not followed exactly.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rfc0019-store-sized-list-delete

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds two synchronous Store primitives needed for the RFC 0019 compactor migration by exposing (1) size-aware listings and (2) a blocking delete, both implemented as thin bridges over existing async object_store operations while preserving existing listing isolation/ordering behavior.

Changes:

  • Introduce Store::list_with_sizes_blocking(prefix) -> Vec<(String, u64)> backed by a shared list_entries core that enforces segment-wise prefix gating and lexicographic ordering.
  • Add Store::delete_blocking(key) as a sync bridge over Store::delete.
  • Add local unit tests plus ignored LocalStack S3 integration tests for the new APIs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/ourios-parquet/src/store.rs Adds delete_blocking, factors listing into list_entries, and exposes list_with_sizes_blocking while keeping ordering + prefix gating centralized.
crates/ourios-parquet/tests/rfc0013_object_store.rs Adds ignored S3 integration coverage for size-aware listing and blocking delete behavior on the AmazonS3 backend.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The RFC 0019 compactor migration (slice 2b) needs two backend-agnostic
Store primitives the querier path didn't: list_with_sizes_blocking, for
the small-file candidate check (sizes come from the listing's ObjectMeta,
no per-object head), and delete_blocking, for orphan/input GC. Both bridge
the existing async core; list_with_sizes factors the listing core into a
shared list_entries so list and the sized variant share the segment-wise
tenant-isolation gate and key ordering. A missing delete surfaces as
is_not_found (the GC loops treat it as already-reclaimed). Local unit tests
plus localstack-ignored S3 tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread crates/ourios-parquet/src/store.rs Outdated
Comment thread crates/ourios-parquet/tests/rfc0013_object_store.rs
Copilot: S3 DELETE is idempotent (absent key -> success), unlike
LocalFileSystem (absent -> not-found). Document the backend-dependent
behaviour on delete_blocking, scope the local unit test's assertion to the
local backend, and make the S3 test tolerant (success or not-found) rather
than asserting not-found, which would fail against real AWS. The compactor's
GC treats either outcome as already-reclaimed, so no head round-trip is added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread crates/ourios-parquet/tests/rfc0013_object_store.rs Outdated
Comment thread crates/ourios-parquet/src/store.rs
The test doc still said a missing S3 key surfaces as is_not_found, which
contradicts the now-tolerant assertion (S3 DELETE is idempotent). Reword to
match. (PR description updated likewise.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@jensholdgaard
jensholdgaard merged commit 71e440e into main Jun 27, 2026
21 checks passed
@jensholdgaard
jensholdgaard deleted the rfc0019-store-sized-list-delete branch June 27, 2026 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants