feat(server): rfc 0022 green pt4 — storage.promoted_attributes config plumbing, status → green - #348
Conversation
…3.2)
The RFC 0020 schema extension: a strict promoted_attributes sub-section
under storage.* with per-element ${env:...} substitution (scalar_vec —
the scalar_opt model applied per list element), resolved onto
ServerConfig via a build_promoted_attributes validator (empty keys are a
config error; dedup + the implicit service.name stay the
PromotedAttributes contract). Env-only config keeps the default set —
the RFC adds a file key, not an OURIOS_* variable.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…22 §3.2/§3.4) ReceiverConfig -> build_write_sinks -> the record sink's new with_promoted_attributes builder; both encode sites (the locked flush_partition and the off-lock publish_partition) switch to encode_records_to_parquet_with_promoted. Compaction previously hardcoded the default set via Writer::open_in, so a rewrite would have silently dropped configured promoted columns — the opposite of §3.4's re-projection; compact_partition_with_promoted / run_sweep_with_promoted now carry the set (the bare names delegate with the default, per the _with_promoted convention) and the server threads the resolved set into both roles. Threading pinned by tests on both sink paths and on the consolidated compaction output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements RFC 0022 §3.2/§3.4 by introducing a ChangesPromoted attributes threading
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Main as ourios-server main
participant Config as ServerConfig
participant Receiver
participant Compactor
participant Writer as ourios-parquet Writer
Main->>Config: build_promoted_attributes(resource, log)
Config-->>Main: PromotedAttributes
Main->>Receiver: ReceiverConfig { promoted }
Main->>Compactor: with_promoted_attributes(promoted)
Receiver->>Receiver: build_write_sinks(store, promoted)
Receiver->>Writer: encode_records_to_parquet_with_promoted(promoted)
Compactor->>Writer: compact_partition_with_promoted(promoted) -> open_in_with_promoted
Writer-->>Compactor: consolidated file with promoted columns
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR completes the operator-facing RFC 0022 plumbing for storage.promoted_attributes by extending the strict server config schema, resolving/promoting key sets onto the runtime ServerConfig, and threading the resulting PromotedAttributes through all write paths (receiver flush/publish and compaction rewrites). It also flips RFC 0022’s status from red to green to reflect that the end-to-end surface and scenarios are now implemented and tested.
Changes:
- Extend
ourios-serverfile config schema withstorage.promoted_attributes.{resource,log}including per-element scalar parsing and${env:...}substitution, then validate/resolve ontoServerConfig. - Thread
PromotedAttributesthrough receiver sinks and ingester write paths so both flush and off-lock publish project the configured promoted columns. - Make compaction re-project rewritten output under the current promoted set via
_with_promotedvariants, and add targeted tests.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/0022-queryable-attribute-columns.md | Flip RFC 0022 status frontmatter red → green. |
| crates/ourios-server/src/receiver.rs | Plumb PromotedAttributes into receiver sink construction and tests. |
| crates/ourios-server/src/main.rs | Resolve storage.promoted_attributes onto ServerConfig and thread into receiver + compactor. |
| crates/ourios-server/src/config/file.rs | Add strict config schema for promoted attributes, scalar list parsing, and per-element env substitution. |
| crates/ourios-parquet/src/lib.rs | Re-export promoted-aware compaction entrypoint. |
| crates/ourios-parquet/src/compaction.rs | Add compact_partition_with_promoted and ensure compaction uses promoted-aware writer; add test. |
| crates/ourios-ingester/src/record_sink.rs | Add promoted set to sink; use promoted-aware encoding in both flush and publish paths. |
| crates/ourios-ingester/src/compactor.rs | Add promoted-aware sweep/compaction wiring and Compactor::with_promoted_attributes. |
| crates/ourios-ingester/src/lib.rs | Re-export promoted-aware sweep function. |
| crates/ourios-ingester/tests/rfc0022_promoted_threading.rs | New integration tests asserting both write paths project the configured promoted set. |
| crates/ourios-ingester/Cargo.toml | Add parquet dev-dependency for schema inspection in new test. |
| Cargo.lock | Lockfile update to include the new dev-dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-ingester/src/compactor.rs (1)
299-323: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a compactor-level promoted-set test. The current coverage exercises
compact_partition_with_promotedand the sink threading paths, but notCompactor::with_promoted_attributes(...)itself. Add an end-to-end sweep test through this constructor and assert the consolidated Parquet file includes the configured promoted columns.🤖 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-ingester/src/compactor.rs` around lines 299 - 323, Add an end-to-end compactor test that goes through Compactor::new and Compactor::with_promoted_attributes instead of only compact_partition_with_promoted. In the new test, configure a promoted set on the Compactor, run a sweep/compaction path, and verify the resulting consolidated Parquet file still contains the configured promoted columns. Use the Compactor::with_promoted_attributes method and the existing compaction/sink test helpers so the coverage exercises the constructor path directly.
🧹 Nitpick comments (2)
crates/ourios-ingester/Cargo.toml (1)
101-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inconsistent version-pinning style vs. neighboring
tempfiledependency.
tempfileright above is exact-pinned ("=3.27.0"), while the newparquetdev-dependency uses a caret range ("58"). Not a functional issue (the workspaceCargo.lockstill pins exact resolved versions), just a style inconsistency worth aligning if the project intends exact-pin discipline for dev-dependencies.🤖 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-ingester/Cargo.toml` around lines 101 - 103, The new parquet dev-dependency uses a version range style that is inconsistent with the exact-pinned tempfile entry nearby. Update the dependency declaration in Cargo.toml to match the project’s pinning convention if exact pins are intended for dev-dependencies, keeping the parquet entry aligned with the neighboring dependency style and the existing ourios-parquet/arrow stack compatibility note.crates/ourios-parquet/src/compaction.rs (1)
760-817: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a property test for promoted-set reprojection.
This new test is example-based (one fixed key, one file pair). Given
compact_partition_with_promotedis a "reconstruction" path over an explicit invariant (output schema = promoted-set columns + row conservation for arbitrary key sets), aproptestcase generating varied resource-key sets alongside the existingcompaction_conserves_every_rowproptest would give stronger coverage of the RFC 0022 projection invariant across arbitrary promoted configurations.As per coding guidelines, "Use property tests (
proptest) for anything with an invariant: the template miner, the Parquet writer, the query planner. Reconstruction is always a property test."🤖 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-parquet/src/compaction.rs` around lines 760 - 817, The new compaction reprojection test is example-based but the behavior is an invariant, so strengthen it by turning the `compaction_reprojects_under_the_given_promoted_set` coverage into a `proptest` similar to `compaction_conserves_every_row`. Generate varied promoted key sets and resource-attribute combinations, then assert through `compact_partition_with_promoted` that the output schema always includes the promoted columns plus the implicit `service.name` path and that row conservation still holds.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.
Outside diff comments:
In `@crates/ourios-ingester/src/compactor.rs`:
- Around line 299-323: Add an end-to-end compactor test that goes through
Compactor::new and Compactor::with_promoted_attributes instead of only
compact_partition_with_promoted. In the new test, configure a promoted set on
the Compactor, run a sweep/compaction path, and verify the resulting
consolidated Parquet file still contains the configured promoted columns. Use
the Compactor::with_promoted_attributes method and the existing compaction/sink
test helpers so the coverage exercises the constructor path directly.
---
Nitpick comments:
In `@crates/ourios-ingester/Cargo.toml`:
- Around line 101-103: The new parquet dev-dependency uses a version range style
that is inconsistent with the exact-pinned tempfile entry nearby. Update the
dependency declaration in Cargo.toml to match the project’s pinning convention
if exact pins are intended for dev-dependencies, keeping the parquet entry
aligned with the neighboring dependency style and the existing
ourios-parquet/arrow stack compatibility note.
In `@crates/ourios-parquet/src/compaction.rs`:
- Around line 760-817: The new compaction reprojection test is example-based but
the behavior is an invariant, so strengthen it by turning the
`compaction_reprojects_under_the_given_promoted_set` coverage into a `proptest`
similar to `compaction_conserves_every_row`. Generate varied promoted key sets
and resource-attribute combinations, then assert through
`compact_partition_with_promoted` that the output schema always includes the
promoted columns plus the implicit `service.name` path and that row conservation
still holds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 460bdee1-6ce5-4547-84ca-13ab550a2a8d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/ourios-ingester/Cargo.tomlcrates/ourios-ingester/src/compactor.rscrates/ourios-ingester/src/lib.rscrates/ourios-ingester/src/record_sink.rscrates/ourios-ingester/tests/rfc0022_promoted_threading.rscrates/ourios-parquet/src/compaction.rscrates/ourios-parquet/src/lib.rscrates/ourios-server/src/config/file.rscrates/ourios-server/src/main.rscrates/ourios-server/src/receiver.rsdocs/rfcs/0022-queryable-attribute-columns.md
… dev-dep Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RFC 0022 green pt4 — the
storage.promoted_attributesconfig plumbing (§3.2), completing the RFC's operator-facing surface. Flips the RFC statusred→green: all seven §5 scenarios pass (writer.1/.2#345, query-side.3/.4/.6#346, pruning/drift.5/.7#347) and the knob the spec promises now exists end-to-end.What
Phase A — config schema + resolution (
ourios-server):storage.promoted_attributes.{resource,log}on the strict RFC 0020 schema (deny_unknown_fieldssub-section). Elements get the scalar treatment via a newscalar_vec(thescalar_optmodel applied per element, RFC 0020 §3.3 rule 7), and${env:…}substitution applies per element inStorageSection::substitute.build_promoted_attributesvalidator resolves the key lists ontoServerConfig(empty keys — e.g. a reference that resolved to nothing — are a config error; dedup and the implicitservice.namestayPromotedAttributes::new's contract). The env-only path keeps the default set: the RFC adds a file key, not anOURIOS_*variable.Phase B — threading to every write path:
ReceiverConfig.promoted→build_write_sinks→ParquetRecordSink::with_promoted_attributes(..)(builder, matchingwith_audit_barrier). Both encode sites switch toencode_records_to_parquet_with_promoted: the locked flush (flush_partition) and the off-lock publish (publish_partitiongains the parameter;publish_ownedcaptures the set alongside the store handle).compact_partitionhardcoded the default set viaWriter::open_in, so a rewrite would have silently dropped configured promoted columns — the opposite of §3.4's "re-projects with the current set". Newcompact_partition_with_promoted/run_sweep_with_promotedvariants (the bare names delegate with the default set, the established_with_promotedconvention),Compactor::with_promoted_attributes(..), and the server threads the resolved set into both roles.Tests
file.rs); resolution ontoServerConfig+ empty-key rejection (main.rs).ourios-ingesterintegration suite pins that both write paths (lockedflush_all, off-lockpublish_owned) project the configured set into flushed files (schema read back via aparquetdev-dep, version in lockstep with the arrow stack).compaction.rstest consolidates default-set inputs under a configured set and asserts the consolidated file carries the configured column + the implicitservice.name(§3.4 convergence).Invariants / hazards (CLAUDE.md §3/§4)
OPTIONALcolumns only, per RFC 0022 §3.4 — no historical rewrite required; compaction convergence is a side effect, nothing depends on it.service.name).Verification
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features,mdbook build(status frontmatter change) — all green locally.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation