feat(v3): add eql_v3.ore_cllw SEM index-term type and operators#272
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
tobyhede
force-pushed
the
eql-v3-ore-cllw
branch
2 times, most recently
from
9b28963 to
e255acb
Compare
![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)
tests/sqlx/tests/encrypted_domain/family/inlinability.rs (1)
118-129:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffAdd the ore_cllw 2-arg comparators to the eql_v3 SEM inline-critical check.
The upstream contract in
tasks/pin_search_path.sql:270-273explicitly listsore_cllw_eq,ore_cllw_neq,ore_cllw_lt,ore_cllw_lte,ore_cllw_gt,ore_cllw_gteas inline-critical (mirroring theore_block_u64_8_256_*comparators already in this query). This test should validate them to match the contract symmetrically.🔍 Proposed fix to add ore_cllw comparators
AND ( (p.pronargs = 2 AND p.proname IN ( 'ore_block_u64_8_256_eq','ore_block_u64_8_256_neq', 'ore_block_u64_8_256_lt','ore_block_u64_8_256_lte', 'ore_block_u64_8_256_gt','ore_block_u64_8_256_gte')) + OR (p.pronargs = 2 AND p.proname IN ( + 'ore_cllw_eq','ore_cllw_neq', + 'ore_cllw_lt','ore_cllw_lte', + 'ore_cllw_gt','ore_cllw_gte')) OR (p.pronargs = 1 AND p.proname IN (🤖 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 `@tests/sqlx/tests/encrypted_domain/family/inlinability.rs` around lines 118 - 129, The test's eql_v3 SEM inline-critical check is missing the 2-argument ore_cllw comparators; update the IN list used with p.pronargs = 2 and p.proname IN (...) to include ore_cllw_eq, ore_cllw_neq, ore_cllw_lt, ore_cllw_lte, ore_cllw_gt, ore_cllw_gte so they are validated alongside ore_block_u64_8_256_*; locate the clause that checks (p.pronargs = 2 AND p.proname IN (...)) and add those ore_cllw_* names to that list to match the upstream contract.
🧹 Nitpick comments (1)
tests/sqlx/tests/encrypted_domain/family/inlinability.rs (1)
92-97: 💤 Low valueUpdate the documentation to mention the ore_cllw comparators.
The comment mentions "the ore_cllw/has_ore_cllw extractors" (1-arg functions) but doesn't call out the ore_cllw comparison operators that take the composite type (2-arg). For consistency with how the ore_block comparators are described, consider updating the doc to clarify that both extractors and comparators are covered.
📝 Suggested doc clarification
-/// take a composite (ore_block_u64_8_256) or raw jsonb (hmac_256, bloom_filter, -/// the ore_cllw/has_ore_cllw extractors, the two per-encrypted-value -/// `jsonb_array_to_*` helpers) arg, so they are NOT caught by the structural +/// take a composite (ore_block_u64_8_256 and its comparators, ore_cllw and its +/// comparators) or raw jsonb (hmac_256, bloom_filter, the ore_cllw/has_ore_cllw +/// extractors, the two per-encrypted-value `jsonb_array_to_*` helpers) arg, so +/// they are NOT caught by the structural🤖 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 `@tests/sqlx/tests/encrypted_domain/family/inlinability.rs` around lines 92 - 97, The documentation comment currently mentions "the ore_cllw/has_ore_cllw extractors" but omits the two-argument ore_cllw comparison operators; update the comment to explicitly call out both the 1-arg extractors (ore_cllw, has_ore_cllw) and the 2-arg ore_cllw comparators (the comparison operators that accept the ore_cllw composite type), mirroring how ore_block_u64_8_256 comparators are described, so readers know both extractors and comparators must be inline_critical allowlisted.
🤖 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 `@src/v3/sem/ore_cllw/types.sql`:
- Around line 3-23: The SQL type eql_v3.ore_cllw documentation is missing the
required Doxygen-style `@param` and `@return` tags; update the comment block above
CREATE TYPE eql_v3.ore_cllw to include an `@param` entry describing the bytes
field (e.g., "bytes — full byte string including domain tag and ciphertext") and
an `@return` entry describing what the type represents/returns (e.g., "Transient
ORE CLLW term used for range comparisons during query execution"), leaving the
rest of the explanatory text intact so the type and its bytes field are fully
documented.
---
Outside diff comments:
In `@tests/sqlx/tests/encrypted_domain/family/inlinability.rs`:
- Around line 118-129: The test's eql_v3 SEM inline-critical check is missing
the 2-argument ore_cllw comparators; update the IN list used with p.pronargs = 2
and p.proname IN (...) to include ore_cllw_eq, ore_cllw_neq, ore_cllw_lt,
ore_cllw_lte, ore_cllw_gt, ore_cllw_gte so they are validated alongside
ore_block_u64_8_256_*; locate the clause that checks (p.pronargs = 2 AND
p.proname IN (...)) and add those ore_cllw_* names to that list to match the
upstream contract.
---
Nitpick comments:
In `@tests/sqlx/tests/encrypted_domain/family/inlinability.rs`:
- Around line 92-97: The documentation comment currently mentions "the
ore_cllw/has_ore_cllw extractors" but omits the two-argument ore_cllw comparison
operators; update the comment to explicitly call out both the 1-arg extractors
(ore_cllw, has_ore_cllw) and the 2-arg ore_cllw comparators (the comparison
operators that accept the ore_cllw composite type), mirroring how
ore_block_u64_8_256 comparators are described, so readers know both extractors
and comparators must be inline_critical allowlisted.
🪄 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
Run ID: 9b0284e9-668a-4d17-9d89-46d09febecd8
📒 Files selected for processing (8)
src/v3/sem/ore_cllw/functions.sqlsrc/v3/sem/ore_cllw/operator_class.sqlsrc/v3/sem/ore_cllw/operators.sqlsrc/v3/sem/ore_cllw/types.sqltasks/pin_search_path.sqltasks/test/splinter.shtests/sqlx/tests/encrypted_domain/family/inlinability.rstests/sqlx/tests/ore_cllw_opclass_tests.rs
Self-contained CLLW ORE searchable-encrypted-metadata index-term type used by the encrypted-JSONB surface for entry-level ordering: the type, extractors/comparators, comparison operators, and a btree operator class, all under src/v3/sem/ore_cllw/ (no eql_v2 dependency).
The ore_cllw SEM fork (598a35b) added the inlinable extractors and operator backing functions but did not give them the same inlining/lint treatment eql_v2 has: - splinter (tasks/test/splinter.sh): add allowlist rows for eql_v3.ore_cllw, has_ore_cllw and the six ore_cllw_* operators. Without these the splinter job fails on the two uncovered findings. - pin_search_path.sql: add the six composite-arg ore_cllw_* operators and the jsonb ore_cllw/has_ore_cllw extractors to the eql_v3 inline-critical clause. The operators take a composite arg (not a jsonb-backed domain) so the structural skip did not spare them — they were being silently pinned, killing inlining and the btree functional -index match, exactly what the eql_v2 inline_critical_oids list avoids. - ore_cllw_opclass_tests.rs: backing_functions_are_inlinable now asserts both eql_v2 and eql_v3 operators are unpinned + inlinable SQL. - inlinability.rs: add eql_v3.bloom_filter(jsonb) to the v3 unpinned assertion list (parity with its sibling eql_v3.hmac_256).
…tes and docs Address review findings 1, 3, 4, 5, 6 on the eql-v3-ore-cllw branch. behavioral test hardcoded eql_v2. v3 is a hand-written fork (src/v3/sem/ ore_cllw/) that can drift independently. Add tests/sqlx/tests/ ore_cllw_v3_opclass_tests.rs (13 #[sqlx::test]) mirroring the v2 suite, adapted to v3's jsonb-only surface (single eql_v3.ore_cllw(jsonb) extractor, no encrypted column / ste_vec / ->): comparator + CLLW per-byte semantics, all six operators, cross-domain tag ordering, extractor NULL handling, ore_cllw_ops DEFAULT FOR TYPE, and functional-index EXPLAIN tests (ORDER BY -> Index Scan/no Sort; WHERE range -> Index Cond). catalog type with no golden slipped past the shell loop (completeness was enforced only by the separate Rust gate). Add a catalog cross-check against `cargo run -p eql-codegen -- list-types` before the loop, mirroring the matrix-inventory gate. operators_for_terms unions, an inconsistency for a future mixed-term domain. Make is_ord_capable union across terms and role_for_terms use Role::rank precedence (Ord > Eq > Match > Storage). Output-neutral for the single-term catalog (parity goldens byte-identical); add mixed-term unit tests. mise.toml inventory-task comment both claimed a single committed snapshot. Correct both to describe the two committed snapshots (ordered baseline + derived/pinned eq-only). silently"; it actually panics at the read_to_string unwrap. Reword. Finding 2 was refuted (the ore_cllw/has_ore_cllw unpinned contract is already guarded) and is not changed.
tobyhede
force-pushed
the
eql-v3-ore-cllw
branch
from
cd8ec3f to
97e7377
Compare
yujiyokoo
approved these changes
Jun 12, 2026
yujiyokoo
left a comment
yujiyokoo
left a comment
Contributor
There was a problem hiding this comment.
Looks good. Added 2 questions
| RETURN NULL; | ||
| END IF; | ||
|
|
||
| IF a.bytes IS NULL OR b.bytes IS NULL THEN |
Contributor
There was a problem hiding this comment.
Just out of curiosity, does this work differently from IF a::text IS NULL OR b::text IS NULL THEN just above?
Contributor
Author
There was a problem hiding this comment.
I had to dig into this one - code is ported from v2
Composite types have row IS NULL semantics - true when every field is explicitly NULL.
Cast forces empty composite to NULL.
Then bytes check is on the inner type.
…nline-critical check Addresses PR #272 review feedback: - Add three eql_v3.ore_cllw opclass tests exercising compare_ore_cllw_term's different-length branches the equal-length operator tests never hit: shorter-sorts-first on an equal prefix, empty-bytes short-circuit, and a differing shared-prefix byte outranking length. - Add the six ore_cllw_* 2-arg comparators to the eql_v3 SEM inline-critical check in inlinability.rs, mirroring tasks/pin_search_path.sql:275-278 and the ore_block_u64_8_256_* entry; update the doc comment to call out the composite-arg comparators.
eql_v3.compare_ore_cllw_term guards with `a::text IS NULL` (true only for a genuine SQL-NULL composite), so a hand-crafted ROW(NULL) falls through to the `a.bytes IS NULL` check and RAISES the extractor-invariant violation. eql_v2 guards with `a IS NULL`, which — for a single-field composite — is also true for ROW(NULL), so v2 silently returns NULL and its RAISE branch is unreachable. No committed test exercised this divergence: both the existing v3 and v2 tests only cover the shared SQL-NULL -> RETURN NULL path. Add comparator_raises_on_row_null_composite to validate v3 raises on ROW(NULL), and clarify the sibling NULL-return test's comment to make the divergence explicit.
tobyhede
force-pushed
the
eql-v3-ore-cllw
branch
from
cd0555e to
86fd880
Compare
tobyhede
added a commit
that referenced
this pull request
Jun 20, 2026
…nline-critical check Addresses PR #272 review feedback: - Add three eql_v3.ore_cllw opclass tests exercising compare_ore_cllw_term's different-length branches the equal-length operator tests never hit: shorter-sorts-first on an equal prefix, empty-bytes short-circuit, and a differing shared-prefix byte outranking length. - Add the six ore_cllw_* 2-arg comparators to the eql_v3 SEM inline-critical check in inlinability.rs, mirroring tasks/pin_search_path.sql:275-278 and the ore_block_u64_8_256_* entry; update the doc comment to call out the composite-arg comparators.
tobyhede
added a commit
that referenced
this pull request
Jun 20, 2026
feat(v3): add eql_v3.ore_cllw SEM index-term type and operators
tobyhede
added a commit
that referenced
this pull request
Jun 22, 2026
…nline-critical check Addresses PR #272 review feedback: - Add three eql_v3.ore_cllw opclass tests exercising compare_ore_cllw_term's different-length branches the equal-length operator tests never hit: shorter-sorts-first on an equal prefix, empty-bytes short-circuit, and a differing shared-prefix byte outranking length. - Add the six ore_cllw_* 2-arg comparators to the eql_v3 SEM inline-critical check in inlinability.rs, mirroring tasks/pin_search_path.sql:275-278 and the ore_block_u64_8_256_* entry; update the doc comment to call out the composite-arg comparators.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds the self-contained
eql_v3.ore_cllwCLLW-ORE searchable-encrypted-metadata (SEM) index-term type undersrc/v3/sem/ore_cllw/: the type, extractors/comparators, comparison operators, and a btree operator class. Noeql_v2dependency.It is a peer of the existing
eql_v3.hmac_256/eql_v3.ore_block_u64_8_256SEM types and provides entry-level ordering for the encrypted-JSONB document surface.Why split out
This is a foundational SEM primitive with a hard
-- REQUIRE:boundary —src/v3/jsonb/{operators,aggregates}.sqlrequiresrc/v3/sem/ore_cllw/operators.sql. Landing it as its own PR underneath theeql_v3.jsondocument surface (#267) keeps that PR focused on the document type rather than mixing in a new index-term primitive.Stack
Scope
src/v3/sem/ore_cllw/{types,functions,operators,operator_class}.sql— 4 new files, ~389 lines, purely additive.Summary by CodeRabbit
New Features
Tests