feat(parquet): rfc 0022 green pt1 — promoted attribute columns in the writer - #345
Conversation
… writer RFC0022.1/.2 live. Every data file now carries the RFC 0022 promoted attribute projection: - `PromotedAttributes` (new `promoted` module): the effective key set — the implicit, non-removable `service.name` plus configured resource/log keys, deduplicated preserving order; column names are the literal DSL paths (`resource.<key>` / `attr.<key>`). - `data_schema_with_promoted`: base §3.2 schema plus the promoted `OPTIONAL` Utf8 fields (Parquet STRING logical type), appended in set order. `data_schema()` stays the base shape readers address by name — §3.9 covers both directions, so files written under any promoted set coexist. - Projection (§3.1): a promoted cell is the attribute's string value byte-for-byte, or NULL for absent/non-string `AnyValue`s — no truncation, first occurrence wins. The canonical-JSON columns are built exactly as before and remain the source of truth. - Writer: `open_in_with_promoted` / `encode_records_to_parquet_with_promoted` carry an explicit set; every existing constructor defaults to `service.name`-only, so the default path always projects it (RFC0022.1). Writer properties bloom-filter each promoted column (single-part ColumnPath — the dots are literal, not nesting); dictionary + statistics ride the global defaults. Evidence: RFC0022.1/.2 acceptance tests (values incl. non-string→NULL, STRING logical-type leaf, byte-identical JSON vs the canonical encoder, implicit service.name alongside configured keys, no column for unconfigured keys, footer-verified bloom+dict+stats); promoted unit tests; schema-pin extended with the promoted variant (base pin untouched). Full workspace gate: 866 passed / 0 failed, clippy pedantic, rustdoc, fmt. Config plumbing (`storage.promoted_attributes`, RFC 0020 extension) and the promoted.size telemetry land in green pt2; querier compile in pt3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds RFC 0022 promoted attribute columns to Parquet schema construction, record batching, and writer paths. It introduces promoted attribute derivation and projection helpers, threads promoted schemas through encoding, and adds schema and Parquet tests. ChangesPromoted attribute columns
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Implements the first “green” slice of RFC 0022 on the Parquet writer side by adding write-time projection of selected attribute keys into dedicated OPTIONAL Utf8 columns (always including resource.service.name) while keeping the canonical JSON columns as the source of truth.
Changes:
- Add
PromotedAttributesand promoted-key string projection logic, plusdata_schema_with_promotedto extend the base schema additively. - Update the Parquet writer to declare the extended schema, project promoted columns, and enable bloom filters per promoted column.
- Turn RFC0022.1/.2 writer tests green and add a schema-pin test for the promoted-column extension.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-parquet/src/promoted.rs | New promoted-attribute set + projection helper for string-only promoted cells. |
| crates/ourios-parquet/src/lib.rs | Export promoted module/types and add data_schema_with_promoted. |
| crates/ourios-parquet/src/record_batch.rs | Add mined_records_to_batch_with_promoted to append projected promoted columns. |
| crates/ourios-parquet/src/writer.rs | Extend writer schema/properties for promoted columns; add *_with_promoted APIs; bloom filters for promoted cols. |
| crates/ourios-parquet/tests/rfc0022_promoted_columns.rs | Green tests for RFC0022.1/.2 projection semantics + metadata assertions. |
| crates/ourios-parquet/tests/schema_pin.rs | Pin default and configured promoted-schema shapes as additive OPTIONAL Utf8 fields. |
💡 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.
🧹 Nitpick comments (3)
crates/ourios-parquet/src/promoted.rs (1)
44-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a proptest for the dedup + first-match invariants.
PromotedAttributes::new's dedup-preserving-order andproject_string_value's first-match/string-only projection are exactly the kind of invariants this crate's guidelines call out for property testing; current coverage is example-based only (service_name_is_implicit_first_and_deduplicated,projection_is_string_only_first_match). Aproptestover arbitrary resource/log key lists (with/without duplicates, with/without an explicitservice.name) and arbitraryKeyValueattribute lists would pin these invariants more robustly than fixed examples.As per coding guidelines,
**/crates/ourios-{miner,parquet,querier}/**/*.rs: "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/promoted.rs` around lines 44 - 117, Add proptest coverage for the invariants in PromotedAttributes::new and project_string_value. Create property tests that generate arbitrary resource/log key lists (including duplicates and optional explicit service.name) to verify service.name is always first, duplicates are removed, and order is preserved in PromotedAttributes::new; also generate arbitrary KeyValue lists to verify project_string_value returns the first matching string value only and yields None for missing or non-string values.Source: Coding guidelines
crates/ourios-parquet/src/writer.rs (1)
244-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the streaming
Writer::open_in_with_promotedpath.The one-shot
encode_records_to_parquet_with_promotedis well covered bytests/rfc0022_promoted_columns.rs, but nothing exercisesWriter::open_in_with_promoted→append_records→closewith a non-defaultPromotedAttributesset. This is the path production ingestion actually uses (append_chunks, multi-callnum_rowsaccounting,self.promotedwiring), and it's new, non-trivial code that currently only gets indirect coverage through shared helper functions.Consider adding a test similar to
open_in_writes_through_a_store_and_reports_key_and_size, but usingWriter::open_in_with_promotedwith a configured set and asserting the round-tripped file contains the expected promoted columns.Also applies to: 608-646
🤖 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/writer.rs` around lines 244 - 292, The streaming Writer::open_in_with_promoted path is not covered with non-default PromotedAttributes, so add a test that opens a Writer via open_in_with_promoted, appends records through append_records, then closes it and verifies the output file round-trips with the expected promoted columns. Reuse the style of open_in_writes_through_a_store_and_reports_key_and_size, but explicitly exercise the self.promoted wiring, num_rows accounting, and close path to cover the production ingestion flow end to end.Source: Coding guidelines
crates/ourios-parquet/src/record_batch.rs (1)
64-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a colocated unit test for the new promoted batching path.
mined_records_to_batch_with_promoted/project_promoted_columnare new, non-trivial logic but only exercised indirectly through the crate-level integration tests (tests/rfc0022_promoted_columns.rs). A small test in this file's own#[cfg(test)] mod tests(mirroring the existingmined_records_to_batchtests, e.g. asserting column order/NULL projection for a couple ofMinedRecords) would keep this file self-contained per the project's testing convention.As per coding guidelines,
**/crates/**/*.rs: "Unit tests must be next to the code and are mandatory 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-parquet/src/record_batch.rs` around lines 64 - 109, Add a colocated unit test in the `record_batch` module’s existing `#[cfg(test)] mod tests` for the new `mined_records_to_batch_with_promoted` / `project_promoted_column` path. Mirror the current `mined_records_to_batch` tests by building a small set of `MinedRecord`s plus `PromotedAttributes`, then assert the promoted columns are appended in the expected order and that string projection yields NULLs for absent/non-string values. Keep the test next to `mined_records_to_batch_with_promoted` so the file remains self-contained and the new logic is directly covered.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.
Nitpick comments:
In `@crates/ourios-parquet/src/promoted.rs`:
- Around line 44-117: Add proptest coverage for the invariants in
PromotedAttributes::new and project_string_value. Create property tests that
generate arbitrary resource/log key lists (including duplicates and optional
explicit service.name) to verify service.name is always first, duplicates are
removed, and order is preserved in PromotedAttributes::new; also generate
arbitrary KeyValue lists to verify project_string_value returns the first
matching string value only and yields None for missing or non-string values.
In `@crates/ourios-parquet/src/record_batch.rs`:
- Around line 64-109: Add a colocated unit test in the `record_batch` module’s
existing `#[cfg(test)] mod tests` for the new
`mined_records_to_batch_with_promoted` / `project_promoted_column` path. Mirror
the current `mined_records_to_batch` tests by building a small set of
`MinedRecord`s plus `PromotedAttributes`, then assert the promoted columns are
appended in the expected order and that string projection yields NULLs for
absent/non-string values. Keep the test next to
`mined_records_to_batch_with_promoted` so the file remains self-contained and
the new logic is directly covered.
In `@crates/ourios-parquet/src/writer.rs`:
- Around line 244-292: The streaming Writer::open_in_with_promoted path is not
covered with non-default PromotedAttributes, so add a test that opens a Writer
via open_in_with_promoted, appends records through append_records, then closes
it and verifies the output file round-trips with the expected promoted columns.
Reuse the style of open_in_writes_through_a_store_and_reports_key_and_size, but
explicitly exercise the self.promoted wiring, num_rows accounting, and close
path to cover the production ingestion flow end to end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b65fa3c-0444-420d-9bfa-9399ed6458f9
📒 Files selected for processing (6)
crates/ourios-parquet/src/lib.rscrates/ourios-parquet/src/promoted.rscrates/ourios-parquet/src/record_batch.rscrates/ourios-parquet/src/writer.rscrates/ourios-parquet/tests/rfc0022_promoted_columns.rscrates/ourios-parquet/tests/schema_pin.rs
…d default Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6e7c182 to
5a7642a
Compare
Projecting each promoted key with its own attrs.iter().find() pass made batch projection O(keys x records x attrs). Build a key->column index once per batch and fill every promoted StringBuilder while visiting each record's attribute list once, preserving the first-occurrence-decides projection (first hit pins the cell, NULL when it is non-string), now also pinned by a batch-level test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-parquet/tests/rfc0022_promoted_columns.rs (1)
234-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a proptest for first-occurrence projection semantics.
This test pins one hand-picked duplicate-key scenario. As per coding guidelines,
**/crates/ourios-{miner,parquet,querier}/**/*.rsshould "Use property tests (proptest) for anything with an invariant" — first-occurrence-wins projection is exactly such an invariant (arbitrary key/attribute-order permutations, string vs. non-string first occurrences). A proptest generating randomized attribute lists and asserting the projected cell always matches the first promoted-key occurrence would give broader coverage than this single fixed example.🤖 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/tests/rfc0022_promoted_columns.rs` around lines 234 - 261, The new promoted projection test only covers one fixed duplicate-key case, but first-occurrence-wins is an invariant that should be exercised more broadly. Replace or supplement promoted_projection_is_first_occurrence_per_record with a proptest that generates arbitrary attribute lists and key-order permutations, then asserts the projected cell for attr.http.route always matches the first promoted-key occurrence (including non-string first occurrences yielding None). Use the existing helpers like PromotedAttributes::new, encode_records_to_parquet_with_promoted, read_all, and promoted_values to keep the test focused on the projection behavior.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.
Nitpick comments:
In `@crates/ourios-parquet/tests/rfc0022_promoted_columns.rs`:
- Around line 234-261: The new promoted projection test only covers one fixed
duplicate-key case, but first-occurrence-wins is an invariant that should be
exercised more broadly. Replace or supplement
promoted_projection_is_first_occurrence_per_record with a proptest that
generates arbitrary attribute lists and key-order permutations, then asserts the
projected cell for attr.http.route always matches the first promoted-key
occurrence (including non-string first occurrences yielding None). Use the
existing helpers like PromotedAttributes::new,
encode_records_to_parquet_with_promoted, read_all, and promoted_values to keep
the test focused on the projection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3958c81a-3ea3-4001-a31d-ac540f4ed90e
📒 Files selected for processing (3)
crates/ourios-parquet/src/promoted.rscrates/ourios-parquet/src/record_batch.rscrates/ourios-parquet/tests/rfc0022_promoted_columns.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/ourios-parquet/src/record_batch.rs
- crates/ourios-parquet/src/promoted.rs
What
RFC 0022 green pt1 — writer-side promoted attribute columns (RFC0022.1/.2 live). First implementation slice after the design sign-off (#343) and red (#344).
PromotedAttributes(ourios-parquet::promoted): the effective key set — implicit, non-removableservice.name+ configured resource/log keys, deduplicated preserving order. Column names are the literal DSL paths.data_schema_with_promoted: base §3.2 schema + promotedOPTIONALUtf8 fields (ParquetSTRINGlogical type overBYTE_ARRAY), appended in set order.data_schema()unchanged (reader-side base; §3.9 covers absent and unknown columns, so mixed promoted sets coexist).NULL(absent / non-stringAnyValue), no truncation, first match wins. JSON columns byte-identical to before — still the source of truth.open_in_with_promoted/encode_records_to_parquet_with_promoted; all existing constructors default toservice.name-only, so every new file projectsservice.name(RFC0022.1). Bloom filter per promoted column; dict + stats on the global defaults.§5 evidence
STRINGlogical type,resource_attributesJSON byte-identical to the canonical encoder (pre-amendment bytes)resource./attr.columns, implicitservice.namerides along, unconfigured keys grow no column, footer-verified bloom + dictionary + statistics per promoted columnOPTIONALUtf8, default + configured shapesInvariants
OPTIONALcolumns only; base schema untouched; the pre-upgrade fixture guard (rfc0021_2) still passes, and the reader's §3.9 unknown-column rule covers new files read by older code.==silently); the promoted set is opt-in beyondservice.name. Thepromoted.sizetelemetry lands with the config plumbing in pt2 (same slice as the knob that can grow the exposure).MinedRecords.Next: pt2 (RFC 0020
storage.promoted_attributes+ ingester wiring +ourios.storage.parquet.promoted.sizevia the weaver registry), pt3 (querier compile, .3/.4/.6), pt4 (pruning + drift, .5/.7, status → green).🤖 Generated with Claude Code
Summary by CodeRabbit
service.nameis always projected; promoted columns are nullable UTF-8 and deterministically ordered.