Skip to content

feat(querier): rfc 0022 green pt2 — promoted predicate compile (RFC0022.3/.4/.6) - #346

Merged
jensholdgaard merged 3 commits into
mainfrom
rfc0022-green-pt2
Jul 3, 2026
Merged

feat(querier): rfc 0022 green pt2 — promoted predicate compile (RFC0022.3/.4/.6)#346
jensholdgaard merged 3 commits into
mainfrom
rfc0022-green-pt2

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jul 3, 2026

Copy link
Copy Markdown
Owner

RFC 0022 green pt2 — the query-side predicate compile. Discharges §5 scenarios RFC0022.3 (pre-amendment parity), RFC0022.4 (operator gating), and RFC0022.6 (projection-blind read path); .5/.7 stay stubbed for the pruning slice.

What

  • attr_match two-arm compile (§3.3). When the scanned union schema carries a key's promoted column (resource.<k> / attr.<k> — §3.4's compile rule), ==/!= compile to the typed arm OR (P IS NULL AND <JSON arm>) fallback, and ordering/regex compile against the typed arm only (pre-amendment rows read NULL and silently never match, per §3.3). Non-promoted keys keep the feat(querier): compile the DSL IR to the execution layer (RFC0002.1/.3/.4/.5/.6) #146 JSON-LIKE behaviour exactly; the rejection text moves to "non-promoted attributes support only == / != in this query surface". Promoted columns are addressed via Expr::Column(Column::new_unqualified(..)) (dotted names would parse as qualified references under col()).
  • Scan schema is now the true §3.9 union (ourios-querier/src/lib.rs). Latent bug: ListingTableConfig::infer_schema infers from the first table path only, and resolve_data_urls produces per-file URLs — so the "union" the old comment claimed was actually "whichever file listed first" (nondeterministic). Harmless before because every consumer of an absent column degraded correctly; load-bearing now that predicate compilation gates on the union schema. The querier infers per file and Schema::try_merges. No extra IO class: the Parquet format already fetches every listed file's footer for statistics at plan time.
  • Committed pre-amendment fixture testdata/rfc0022/pre-amendment.parquet (the RFC 0021 §6 fixture discipline), since every current writer path implicitly promotes service.name and can no longer produce a pre-amendment schema. The #[ignore]d generator test documents/regenerates it.
  • Test helpers: write_all_with_promoted / rec_with_attrs in the shared querier fixtures; RESOURCE_PREFIX/ATTR_PREFIX exported from ourios_parquet::promoted so the compiler derives column names from the same constants the writer declares.

Scenario mapping

  • RFC0022.3 — live oracle: a pre-only bucket compiles pure-LIKE (no promoted column in its union schema — the exact pre-RFC 0022 code path); asserts combined == pre_only + post_only per query plus template-scoped row identity.
  • RFC0022.4 — ordering/regex answer typed-arm-only over a mixed scan (/login in the pre-amendment file would match >= "/ch" but must not); non-promoted keys (present in the JSON!) still reject with InvalidQuery; ==/!= unchanged both sides. The fixture's installed filename sorts before the writer's UUIDv7 names so a regression to first-file schema inference fails deterministically.
  • RFC0022.6 — a hand-forged file whose promoted cells disagree with the JSON truth: rows round-trip from the JSON columns; no forged value reaches the returned LogRow (Debug-scan asserted).

Invariants / hazards (CLAUDE.md §3/§4)

  • §3.5 schema evolution: no on-disk change. Read-side only; pre-amendment files are answered via the §3.3 fallback arm (that is the migration plan, §3.4). The schema-union fix restores the documented §3.9 read behaviour.
  • Hazard docs(rfc-0001): add §5 acceptance criteria (drafted → specified) #6 (DSL leakage): surface unchanged — same field paths gain operators; rejection/error text stays engine-free (no DataFusion/arrow types or messages).
  • Tests are specifications: three #[ignore]d red stubs are implemented and un-ignored; no existing test weakened or deleted.

Verification

cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings, cargo test --all-features — all green locally (114 suite blocks, 0 failures).

Remaining for RFC 0022 green: .5 (pruning oracle) + .7 (promoted-set drift) in the pruning slice; storage.promoted_attributes config plumbing (RFC 0020 schema extension, §3.2) as its own follow-up slice.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Querying now handles promoted attribute columns consistently across mixed parquet data, including correct comparisons and operator support.
  • Bug Fixes

    • Improved schema handling so scans use the combined fields from all files, reducing missing-column query issues.
    • Fixed attribute filtering to return correct results when promoted columns are present or absent.
  • Tests

    • Added broader integration coverage for promoted attributes, mixed data sets, operator behavior, and JSON-backed reads.

jensholdgaard and others added 2 commits July 3, 2026 21:21
…iles

ListingTableConfig::infer_schema infers from the first table path only,
and resolve_data_urls produces per-file URLs — so the schema the scan
(and now the RFC 0022 predicate compile) saw was one arbitrary file's,
not the RFC 0005 §3.9 union the surrounding comment claimed. Harmless
before because every consumer of an absent OPTIONAL column degrades
correctly; load-bearing once attr_match gates promoted-column arms on
the union schema. Infer per file and Schema::try_merge — no new IO
class, the Parquet format already reads every listed footer for
statistics at plan time.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RFC0022.3/.4/.6 go green. attr_match compiles the §3.3 two-arm form
when the scanned union schema carries the key's promoted column
(==/!= keep the P-IS-NULL JSON fallback for pre-amendment files and
non-string values; ordering/regex are typed-arm-only with the
documented silent non-match on NULL), addressed via an unqualified
Column since col() would parse the dotted names as qualified
references. Non-promoted keys keep the #146 LIKE behaviour; the
rejection text moves to 'non-promoted attributes support only == / !='.

The pre-amendment file is a committed fixture (RFC 0021 §6 discipline,
#[ignore]d generator) since every current writer path implicitly
promotes service.name. RFC0022.5/.7 stay stubbed for the pruning slice.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jensholdgaard
jensholdgaard requested a review from Copilot July 3, 2026 19:21
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

Next review available in: 53 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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 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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4418c9cb-2260-4025-8cf0-9e0ae6459293

📥 Commits

Reviewing files that changed from the base of the PR and between bf9802d and d0b5604.

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

Walkthrough

This PR implements RFC 0022 promoted attribute columns end-to-end: parquet prefix constants become public, the query compiler detects and uses promoted resource.<k>/attr.<k> columns with NULL-aware fallback to JSON, Querier::execute merges per-file schemas into a union schema, and new integration tests validate the behavior.

Changes

Promoted attribute columns support

Layer / File(s) Summary
Public promoted column-name prefixes
crates/ourios-parquet/src/promoted.rs
RESOURCE_PREFIX and ATTR_PREFIX are made public with documentation.
attr_match promoted-column compilation
crates/ourios-querier/src/compile.rs
attr_match detects promoted columns in the union schema, compiles equality/inequality with NULL-aware two-arm logic plus JSON fallback, and restricts ordering/regex operators to promoted-only keys, rejecting others with InvalidQuery.
Union schema merging in query execution
crates/ourios-querier/src/lib.rs
Querier::execute infers per-file schemas and merges them via Schema::try_merge instead of relying on inference over a single arbitrary path.
Test fixtures and RFC0022 integration tests
crates/ourios-querier/tests/common/mod.rs, crates/ourios-querier/tests/rfc0022_attr_columns.rs
New fixture helpers (write_all_with_promoted, rec_with_attrs) support promoted-attribute records; RFC0022.3/.4/.6 tests validate row-count parity, operator gating, and projection-blind JSON-truth behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test as RFC0022 Test
  participant Writer as Fixture Writer
  participant Querier as Querier::execute
  participant Compile as attr_match Compiler
  participant Schema as Union Schema

  Test->>Writer: write pre/post amendment records with PromotedAttributes
  Test->>Querier: submit QueryRequest
  Querier->>Schema: infer per-file schemas and merge (try_merge)
  Querier->>Compile: compile attribute predicate against union schema
  Compile->>Compile: check if promoted column exists
  alt promoted column exists
    Compile-->>Querier: NULL-aware promoted Expr with JSON fallback
  else promoted column absent
    Compile-->>Querier: JSON LIKE Expr (== / != only)
  end
  Querier-->>Test: results or QueryError::InvalidQuery
Loading

Possibly related PRs

  • jensholdgaard/ourios#146: Both PRs modify crates/ourios-querier/src/compile.rs attribute predicate compilation logic, with this PR extending the earlier RFC0002 DSL-to-DataFusion compilation to detect and use promoted columns.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly describes the main querier change: RFC 0022 promoted predicate compilation.
Description check ✅ Passed The description covers the summary, RFC context, implementation details, scenario mapping, and verification, so it is mostly complete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rfc0022-green-pt2

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

Implements the query-side predicate compilation needed for RFC 0022 promoted attribute columns, ensuring mixed scans (pre-/post-amendment Parquet files) compile predicates correctly against the true union schema while keeping the read path projection-blind.

Changes:

  • Add promoted-column-aware attribute predicate compilation (two-arm fallback for ==/!=, typed-only for ordering/regex when promoted).
  • Fix table schema inference to merge schemas across all scanned files (true union) instead of relying on first-path inference.
  • Implement RFC0022.3/.4/.6 integration tests and add shared test fixtures for writing with explicit promoted sets.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/ourios-querier/tests/rfc0022_attr_columns.rs Implements RFC0022.3/.4/.6 tests and fixture installation/generation logic for pre-amendment Parquet coverage.
crates/ourios-querier/tests/common/mod.rs Adds test helpers to write Parquet with explicit promoted attributes and to build records with both resource/log attributes.
crates/ourios-querier/src/lib.rs Ensures scan schema is the merged union across all scanned files by inferring per-file schema and merging.
crates/ourios-querier/src/compile.rs Extends attribute predicate compilation to use promoted columns when present, with fallback to JSON LIKE for legacy/non-promoted cases.
crates/ourios-parquet/src/promoted.rs Exposes promoted column-name prefixes for shared derivation between writer and querier compile.

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

Comment thread crates/ourios-querier/tests/rfc0022_attr_columns.rs
… idiom

Co-Authored-By: Claude Fable 5 <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 5 out of 6 changed files in this pull request and generated no new comments.

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