feat(querier): two-arm body equality — the template arm (RFC 0044 slice 2) - #672
Conversation
…ce 2) body ==/!= now compiles to the RFC 0044 §3.1 two-arm form. The physical arm keeps covering retained and lossy bodies, gated on body_kind = String so a structured body's canonical JSON bytes can never string-match (§3.4). The template arm resolves the literal at plan time against the tenant registry (the slice-1 matcher) and compiles each candidate to its version-qualified template_id (prunable) plus element-wise params/separators equalities — params are single whitespace-free tokens by construction, so element equality is byte-identical reconstruction equality (§3.3), and overflow-spilled params cannot false-match (truncated vs full capture) while their records stay reachable through the retained body. != is explicit three-valued: a NULL physical body no longer silently drops mined records — the mirror of #664. The registry rides the same RFC 0033 cached-map acquisition as the alias fold (one map, one frontier per query), skipped entirely when the predicate has neither resolves_to nor a body equality; the injected-alias-map path acquires when a body equality needs it. Closes #664. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 36 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 (2)
📝 WalkthroughWalkthroughChangesRFC0044 body equality now resolves template candidates during planning and compiles RFC0044 body equality
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Querier
participant TemplateMapLoader
participant Compiler
participant QueryExecutor
Client->>Querier: run_query_with DSL predicate
Querier->>TemplateMapLoader: load_or_derive when body equality or alias resolution is needed
TemplateMapLoader-->>Querier: template map and registry
Querier->>Compiler: compile predicate with alias map and registry
Compiler-->>QueryExecutor: physical and template candidate predicate
QueryExecutor-->>Client: query results
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 RFC 0044 slice 2 in ourios-querier by compiling body == / body != into a two-arm predicate: a physical-body arm (gated on body_kind = String) plus a template-aware arm that matches version-qualified (template_id, template_version) and element-wise params/separators, addressing issue #664 where mined records had NULL physical bodies and were incorrectly pruned/missed.
Changes:
- Add plan-time collection of
bodyequality literals and their registry-derived candidate templates, then compilebody ==/!=into the two-arm DataFusion expression. - Adjust query execution to acquire the cached template map when needed by either
resolves_to(...)alias folding orbody ==/!=template matching. - Add new integration tests (RFC0044.1–.5 plus the empty-set half of .8) covering mined, retained, and structured-body behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/ourios-querier/tests/it/rfc0044_body_equality.rs | Adds RFC 0044 integration scenarios validating correct body ==/!= behavior over mined/retained/structured bodies. |
| crates/ourios-querier/tests/it/main.rs | Registers the new RFC 0044 integration test module. |
| crates/ourios-querier/src/lib.rs | Extends template-map acquisition conditions to include body ==/!= registry needs; passes registry into compilation. |
| crates/ourios-querier/src/compile.rs | Adds plan-time body-literal resolution and compiles body ==/!= into the two-arm predicate using registry candidates + separators/params checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ourios-querier/src/compile.rs (1)
929-955: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPanic risk:
body_eqs[literal]indexing.
body_eqs[literal]panics ifliteralisn't a key. This currently can't happen becausecollect_body_equalitieswalks the exact same predicate shape beforecompile_comparisonruns, but it's a latent footgun for any future refactor that lets the two traversals drift (e.g. a newPredicatevariant added to one match but not the other) — a query would crash instead of returning aQueryError. The file already has an established, safer pattern for this exact shape:resolves_to_expr(line ~1491) usesalias_classes.get(&n)with a graceful fallback instead of indexing.🛡️ Proposed fix
- (Field::Body, CmpOp::Ord(OrdOp::Eq), Value::Str(literal)) => { - Ok(body_equality(&body_eqs[literal], literal, df, false)) - } - (Field::Body, CmpOp::Ord(OrdOp::Ne), Value::Str(literal)) => { - Ok(body_equality(&body_eqs[literal], literal, df, true)) - } + (Field::Body, CmpOp::Ord(OrdOp::Eq), Value::Str(literal)) => { + let plan = body_eqs.get(literal).expect( + "collect_body_equalities resolves every body ==/!= literal \ + compile_comparison later visits", + ); + Ok(body_equality(plan, literal, df, false)) + } + (Field::Body, CmpOp::Ord(OrdOp::Ne), Value::Str(literal)) => { + let plan = body_eqs.get(literal).expect( + "collect_body_equalities resolves every body ==/!= literal \ + compile_comparison later visits", + ); + Ok(body_equality(plan, literal, df, true)) + }At minimum an
.expect()with a message documenting the invariant is a cheap improvement over a bare index panic.🤖 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-querier/src/compile.rs` around lines 929 - 955, Replace the direct body_eqs[literal] indexing in compile_comparison with a graceful missing-key path that returns an appropriate QueryError, or at minimum use expect with a clear message documenting the collect_body_equalities invariant. Preserve the existing body_equality behavior when the literal is present and apply the same handling to both equality and inequality branches.
🧹 Nitpick comments (1)
crates/ourios-querier/src/compile.rs (1)
973-1028: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse the named string body kind constant instead of
0_u8.Replace
lit(0_u8)with a singleBodyKind-backed constant/cast forStringso the String predicate is self-documenting and protected against future enum reordering. TheBodyKind::Absentvariant is part of the row model via RFC 0025, but this gate is intentionally the String-only path, soNoneis not the right fix.🤖 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-querier/src/compile.rs` around lines 973 - 1028, Update body_equality’s string_kind predicate to use the named BodyKind-backed constant or cast representing String instead of the raw 0_u8 literal. Preserve the existing String-only gate semantics and do not substitute BodyKind::Absent or None.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.
Inline comments:
In `@crates/ourios-querier/src/compile.rs`:
- Around line 364-439: Add colocated unit tests for uses_body_equality and
collect_body_equalities, covering nested predicates, tokenizer failure, zero
candidates, and multiple candidates, plus candidate_arm covering template
metadata, parameter and separator indexing, operand order, and 1-based indices.
Use the existing lowered() structural-assertion style and test candidate_arm
directly without a DataFrame or scan.
---
Outside diff comments:
In `@crates/ourios-querier/src/compile.rs`:
- Around line 929-955: Replace the direct body_eqs[literal] indexing in
compile_comparison with a graceful missing-key path that returns an appropriate
QueryError, or at minimum use expect with a clear message documenting the
collect_body_equalities invariant. Preserve the existing body_equality behavior
when the literal is present and apply the same handling to both equality and
inequality branches.
---
Nitpick comments:
In `@crates/ourios-querier/src/compile.rs`:
- Around line 973-1028: Update body_equality’s string_kind predicate to use the
named BodyKind-backed constant or cast representing String instead of the raw
0_u8 literal. Preserve the existing String-only gate semantics and do not
substitute BodyKind::Absent or None.
🪄 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: fe47a5f3-5865-4798-83ff-f80a6053e57c
📒 Files selected for processing (4)
crates/ourios-querier/src/compile.rscrates/ourios-querier/src/lib.rscrates/ourios-querier/tests/it/main.rscrates/ourios-querier/tests/it/rfc0044_body_equality.rs
…eview) IS TRUE / IS NOT TRUE make the candidate disjunction total: a NULL param slot under a matching template (a corrupted row — reconstruct's own fallback treats it as body-retained) reads as "not this candidate", so equality never matches the unknowable and != never three-valued-drops it. Unit tests pin the walker, the collector (dedup + tokenizer-failure), and the candidate arm's lowered shape (version qualification, 1-based indices). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/ourios-querier/src/compile.rs:1059
candidate_armdoesn’t enforce thereconstruct()shape preflight (separators.len() == template.len() + 1andparams.len() == wildcard_count). A producer bug / corrupted row with extra params or separators can still satisfy the current element-wise equality checks for the expected prefix and incorrectly match the template arm, even though reconstruction would fall back to the retained body (or empty whenbodyis NULL). Add a cheap exact-length guard so only faithfully-shaped rows can match the template arm.
fn candidate_arm(candidate: &BodyLiteralMatch, separators: &[Vec<u8>]) -> Expr {
let mut arm = col(columns::TEMPLATE_ID)
.eq(lit(candidate.template_id))
.and(col(columns::TEMPLATE_VERSION).eq(lit(candidate.template_version)));
for (i, value) in candidate.params.iter().enumerate() {
let idx = i64::try_from(i).unwrap_or(i64::MAX).saturating_add(1);
arm = arm.and(
get_field(array_element(col(columns::PARAMS), lit(idx)), "value")
.eq(lit(ScalarValue::Binary(Some(value.as_bytes().to_vec())))),
);
}
for (k, sep) in separators.iter().enumerate() {
let idx = i64::try_from(k).unwrap_or(i64::MAX).saturating_add(1);
arm = arm.and(
array_element(col(columns::SEPARATORS), lit(idx))
.eq(lit(ScalarValue::Binary(Some(sep.clone())))),
);
}
arm
A retained body is the truth: an overflow-spilled param's truncated stored value could otherwise false-match a literal crafted from the truncation, exactly the case reconstruct refuses. Non-lossy retention loses nothing — §3.3 makes the physical arm equivalent there. Regression test: the crafted literal matches nothing, the retained body matches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/ourios-querier/src/compile.rs:423
collect_body_equalitiestokenizes each body literal to extractseparators, and thenbody_literal_candidatestokenizes the same literal again to produce the candidate set. This duplicates work per distinct literal and will scale linearly with the number of body-equality literals in a predicate. Consider refactoring so the literal is tokenized once and bothseparatorsand candidate unification reuse the same tokenization output (e.g. a helper that takes the pre-tokenized literal, or a function that returns both candidates + separators).
let separators = ourios_miner::tokenize::tokenize(literal).map_or_else(
|_| Vec::new(),
|tk| {
tk.separators
.iter()
.map(|s| s.as_bytes().to_vec())
.collect()
},
);
out.insert(
literal.clone(),
BodyEqualityPlan {
candidates: body_literal_candidates(registry, literal),
separators,
},
crates/ourios-querier/src/compile.rs:979
body_kindis compared against a hard-coded0_u8to identifyBodyKind::String. That ordinal is part of the Parquet on-disk contract (seeourios-parquet/src/record_batch.rs:589-598), so leaving it as a magic number here makes the predicate easy to break if the encoding ever changes (e.g. adding a new variant). Introduce a named local constant (or shared exported constant) documenting the mapping and use it in the filter.
let string_kind = col(columns::BODY_KIND).eq(lit(0_u8));
let physical_present = has_column(df, columns::BODY);
Summary
RFC 0044 slice 2 — the user-visible fix for #664.
body ==/!=compiles to the §3.1 two-arm form:body_kind = String— which is also what enforces §3.4 (a structured body's canonical JSON bytes can never string-match, even for a crafted literal).template_idequality (prunable via existing statistics) plus element-wiseparams/separatorsequalities via the samearray_element/get_fieldloweringparam(n)uses. Params are single whitespace-free tokens by construction, so element equality is byte-identical reconstruction equality (CLAUDE.md§3.3); overflow-spilled params cannot false-match (truncated ≠ full capture) and their records remain reachable through the retained body.!=is explicit three-valued (§3.2): a NULL physical body no longer silently drops mined records — the exact mirror of the body equality predicate silently misses template-mined records (row group pruned on the NULL body column) #664 bug.Invariants / hazards
lossy = trueby construction.Verification
Six new it scenarios (RFC0044.1–.5 + the empty half of .8) against real stores with the real registry fold — the #664 reproduction now returns its record with the row group scanned. Full querier suite 111/111 it + 124 lib, server it 52 green, fmt/clippy clean.
Remaining slices: alias/version traversal (.6), partitioned pruning fixtures (.7/.8 full), the RFC0044.9 property suite.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F
Summary by CodeRabbit
New Features
body ==andbody !=query matching for template-derived and physically stored audit bodies.Tests