fix(ope): address CodeRabbit and reviewer feedback on PR #176#197
fix(ope): address CodeRabbit and reviewer feedback on PR #176#197freshtonic merged 7 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors OPE (Order-Preserving Encryption) functions from PL/pgSQL to SQL, clarifies STRICT NULL semantics in comparisons, updates documentation from CLWW to CLLW terminology, and adds extensive test coverage for operators, NULL handling, and aggregates across fixed and variable-width OPE encodings. ChangesOPE Refactoring and NULL Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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)
src/ope_cllw_u64_65/compare.sql (1)
30-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify whether NULL branches are justified by system invariants or represent incomplete PR implementation
The PR description's claim that
STRICTeliminates null-handling branches is imprecise.STRICTshort-circuits when function inputs (aandb) areNULL—not when extracted terms (a_termandb_term) areNULL. The latter can legitimately occur:has_ope_cllw_u64_65checks whether the 'opf' field exists in the encrypted JSONB payload and returnsfalseif absent, leaving the termNULLafter lines 30–36.The NULL branches (lines 38–48) are only dead code if a system invariant guarantees every
eql_v2_encryptedvalue always carries a valid OPE index term. No such invariant is documented in the codebase. Bothcompare_ope_cllw_u64_65andcompare_ope_cllw_var_8retain these branches consistently, suggesting intentional defensive logic rather than oversight.Either confirm that an undocumented invariant exists (and remove both conditional extraction and NULL checks together), or acknowledge that the NULL branches remain necessary.
🤖 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 `@src/ope_cllw_u64_65/compare.sql` around lines 30 - 48, The review points out that STRICT only covers NULL inputs a/b, not extracted terms, so decide whether the NULL-handling branches in compare_ope_cllw_u64_65 are removable: either document and enforce the invariant that every eql_v2_encrypted value has an OPE term and then remove both the has_ope_cllw_u64_65 checks and the subsequent a_term/b_term NULL branches (and mirror the same change in compare_ope_cllw_var_8), or retain the defensive logic as-is; update code/comments accordingly and reference the functions has_ope_cllw_u64_65, ope_cllw_u64_65, compare_ope_cllw_u64_65 and compare_ope_cllw_var_8 so reviewers can verify the chosen approach.
🧹 Nitpick comments (1)
src/ope_cllw_var_8/functions.sql (1)
36-37: 💤 Low value
@paramis missing the parameter namevalLines 36 and 70 document the parameter using the type as a stand-in for the name (
@param eql_v2_encrypted …), which is not valid Doxygen syntax. Compare tocompare.sqllines 12-13 where the namesaandbare explicitly present.📝 Proposed doc fix
---! `@param` eql_v2_encrypted Encrypted column value +--! `@param` val eql_v2_encrypted Encrypted column valueApply the same fix at both line 36 and line 70.
Also applies to: 70-71
🤖 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 `@src/ope_cllw_var_8/functions.sql` around lines 36 - 37, The docblocks use the type name instead of the parameter name; update both param annotations so they include the parameter name "val" before the type (e.g., change the `@param` line that currently starts with "eql_v2_encrypted" to start with "val eql_v2_encrypted ...") for the function/documentation referencing eql_v2.ope_cllw_var_8; apply the same change at the second occurrence around line 70 so the `@param` syntax matches the valid form used in compare.sql (names like "a" and "b").
🤖 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 `@docs/reference/sql-support.md`:
- Line 142: The table row for `COUNT(DISTINCT col)` is missing `ope` in the
outer-column requirement; update the docs so the outer-column support list for
`COUNT(DISTINCT col)` includes `ope` alongside `unique` and `ore`. Locate the
row mentioning `COUNT(DISTINCT col)` in sql-support.md and add `ope` to the
outer-column cell so it matches the support matrix for DISTINCT semantics.
---
Outside diff comments:
In `@src/ope_cllw_u64_65/compare.sql`:
- Around line 30-48: The review points out that STRICT only covers NULL inputs
a/b, not extracted terms, so decide whether the NULL-handling branches in
compare_ope_cllw_u64_65 are removable: either document and enforce the invariant
that every eql_v2_encrypted value has an OPE term and then remove both the
has_ope_cllw_u64_65 checks and the subsequent a_term/b_term NULL branches (and
mirror the same change in compare_ope_cllw_var_8), or retain the defensive logic
as-is; update code/comments accordingly and reference the functions
has_ope_cllw_u64_65, ope_cllw_u64_65, compare_ope_cllw_u64_65 and
compare_ope_cllw_var_8 so reviewers can verify the chosen approach.
---
Nitpick comments:
In `@src/ope_cllw_var_8/functions.sql`:
- Around line 36-37: The docblocks use the type name instead of the parameter
name; update both param annotations so they include the parameter name "val"
before the type (e.g., change the `@param` line that currently starts with
"eql_v2_encrypted" to start with "val eql_v2_encrypted ...") for the
function/documentation referencing eql_v2.ope_cllw_var_8; apply the same change
at the second occurrence around line 70 so the `@param` syntax matches the valid
form used in compare.sql (names like "a" and "b").
🪄 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: ac3e9e50-8bf3-44db-9e12-bd42b28e04e5
📒 Files selected for processing (10)
docs/reference/index-config.mddocs/reference/sql-support.mdsrc/ope_cllw_u64_65/compare.sqlsrc/ope_cllw_u64_65/functions.sqlsrc/ope_cllw_u64_65/types.sqlsrc/ope_cllw_var_8/compare.sqlsrc/ope_cllw_var_8/functions.sqlsrc/ope_cllw_var_8/types.sqlsrc/operators/order_by.sqltests/sqlx/tests/ope_tests.rs
- Drop dead NULL-handling branches from compare_ope_cllw_* (STRICT
short-circuits NULL inputs before the body runs).
- Fix ciphertext-length wording in OPE type docs ("8 bytes per plaintext
byte", not "per plaintext bit", which contradicted the 65-byte result).
- Convert simple OPE helpers (extractors, has_*, order_by_ope) from
LANGUAGE plpgsql to LANGUAGE sql so the planner can inline them on the
sort/order_by hot path. Per project guideline.
- Spell the cipher consistently as CLLW (matching ope_cllw_* type names
and the existing ore_cllw_* docs); CLWW was a typo.
- Clarify sql-support.md MIN/MAX and COUNT(DISTINCT) rows so ope is no
longer implied to be a ste_vec-extracted node term — it only applies
to the outer column.
- Loosen brittle exact-list assertion in config_check_rejects_unknown_index
to mention 'ope' and the offending token.
- Add ore_wins_over_opf_when_both_present to lock in the
ORE-before-OPE dispatch precedence in eql_v2.compare.
- Add operator-level coverage for <=, >=, =, <> on both opf and opv
variants (previously only < and > were covered for opf, and opv was
exercised through compare_* but not via operators).
Adds 18 OPE tests covering behaviors that were exercised for ORE but had no OPE equivalent. The ORE suite duplicates each test across three ciphertext variants (block / cllw_u64_8 / cllw_var_8); for OPE we only need one representative per variant per behavior because both opf and opv use the same primitive (bytea lex compare). NULL term in payload (mirrors compare_hmac_with_null_ore_index): - has_opf_false_when_field_is_json_null - has_opv_false_when_field_is_json_null - compare_dispatches_through_null_opf_to_hmac - compare_dispatches_through_null_opv_to_hmac NULL operands at the comparator (codifies STRICT short-circuit): - compare_ope_cllw_u64_65_strict_returns_null_for_null_operand - compare_ope_cllw_var_8_strict_returns_null_for_null_operand ORDER BY NULLS FIRST/LAST × ASC/DESC on opf-encoded data: - order_by_asc_nulls_first_with_opf - order_by_asc_nulls_last_with_opf - order_by_desc_nulls_first_with_opf - order_by_desc_nulls_last_with_opf sort_compare with mixed NULL + opf rows: - sort_compare_asc_puts_nulls_first_with_opf - sort_compare_desc_puts_nulls_last_with_opf MIN / MAX aggregates over OPE-encoded values: - eql_v2_min_with_opf_finds_minimum - eql_v2_max_with_opf_finds_maximum - eql_v2_min_with_opf_null_only_returns_null - eql_v2_max_with_opf_null_only_returns_null BETWEEN range filtering: - between_with_opf_inclusive_bounds - between_with_opv_inclusive_bounds All 47 OPE tests pass; full suite (~430 tests) is green.
The earlier sql-support clarification dropped `ope` from the outer-column index list for COUNT(DISTINCT col). OPE ciphertexts are deterministic within a column key, so an outer-column `ope` index supports DISTINCT semantics just like `unique` and `ore`. Restoring it for parity with the MIN/MAX row.
freshtonic
force-pushed
the
fix/ope-index-type-docs
branch
from
48aeaa4 to
ffb45d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/sqlx/tests/ope_tests.rs`:
- Around line 798-808: The test
compare_ope_cllw_var_8_strict_returns_null_for_null_operand only asserts
compare(NULL, x) — add the symmetric assertion to also check compare(x, NULL)
returns NULL by constructing the SQL with the encrypted left operand (use
opv_payload and eql_v2.to_encrypted as in the existing test) and executing a
sqlx::query_scalar to assert the result is_none; update the test
compare_ope_cllw_var_8_strict_returns_null_for_null_operand to include this
second fetch/assert using the same pool and payload so both NULL-left and
NULL-right paths are covered.
🪄 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: ddd41923-fd4a-4a63-8037-22dc6a0e98c9
📒 Files selected for processing (1)
tests/sqlx/tests/ope_tests.rs
Two doc-only fixes flagged in PR #197 review: 1. compare.sql @note for both u64_65 and var_8: the term-NULL branches (a_term IS NULL / b_term IS NULL) are NOT redundant with STRICT. STRICT only short-circuits on NULL function inputs (a, b); the term branches handle a non-NULL eql_v2_encrypted whose payload simply lacks the opf/opv field. Mirrors the defensive pattern in compare_ore_block_u64_8_256. 2. functions.sql @param tags for ope_cllw_u64_65 / ope_cllw_var_8 / has_ope_cllw_u64_65 / has_ope_cllw_var_8 now include the parameter name "val" before the type, matching valid Doxygen syntax (and compare.sql which already names a/b correctly). No code behavior change; mise run docs:validate stays clean.
Match the bidirectional NULL coverage of the u64_65 sibling test. Asserting only compare(NULL, x) leaves the right-NULL STRICT path unverified; both directions must short-circuit.
Two doc-only fixes flagged in PR #197 review: 1. compare.sql @note for both u64_65 and var_8: the term-NULL branches (a_term IS NULL / b_term IS NULL) are NOT redundant with STRICT. STRICT only short-circuits on NULL function inputs (a, b); the term branches handle a non-NULL eql_v2_encrypted whose payload simply lacks the opf/opv field. Mirrors the defensive pattern in compare_ore_block_u64_8_256. 2. functions.sql @param tags for ope_cllw_u64_65 / ope_cllw_var_8 / has_ope_cllw_u64_65 / has_ope_cllw_var_8 now include the parameter name "val" before the type, matching valid Doxygen syntax (and compare.sql which already names a/b correctly). No code behavior change; mise run docs:validate stays clean.
freshtonic
force-pushed
the
fix/ope-index-type-docs
branch
from
e79ab50 to
01f8120
Compare
|
Hey James — flagging an inlining issue that CI is hiding. Splinter is green and the new lint test passes, but the perf goal of the PR (inlinable OPE helpers on the sort/order_by hot path) is silently undone after merge. Wanted to lay out the mechanism in detail since this conflict will keep coming up. What "inlining" meansPostgreSQL's planner has an optimisation in ORDER BY eql_v2.order_by_ope(col)is rewritten by the planner to: ORDER BY CASE
WHEN eql_v2.has_ope_cllw_u64_65(col) THEN (eql_v2.ope_cllw_u64_65(col)).bytes
WHEN eql_v2.has_ope_cllw_var_8(col) THEN (eql_v2.ope_cllw_var_8(col)).bytes
ENDThat matters for two reasons:
What disables inlining
if (... !heap_attisnull(func_tuple, Anum_pg_proc_proconfig, NULL) ...)
return NULL; // bail — won't inlineIf How your PR gets brokenYour source files are correct as written. The damage happens later in the install pipeline. Here's the timeline:
I simulated the merge state locally (PR branch source files + main's pinner and The function still works (returns the right answer); it's just orders of magnitude slower at scale. There's no error, no warning, nothing in CI that fails — splinter is happy because That's the silent-undo problem. Why the pinner existsSupabase splinter's
Any function where we want both is impossible. The pinner is told which functions to skip via an explicit allowlist ( PrioritisationStating this explicitly because the conflict will keep coming up: performance wins over splinter compliance. When inlining and The fixYour 1. -- (encrypted,) overloads
(p.proargtypes[0] = enc_oid AND p.pronargs = 1
AND p.proname IN (
'ope_cllw_u64_65', 'ope_cllw_var_8', -- OPE term extractors (#176)
'has_ope_cllw_u64_65', 'has_ope_cllw_var_8', -- OPE term presence checks
'order_by_ope' -- OPE order-by extractor
))
-- (jsonb,) overloads
OR (p.proargtypes[0] = jsonb_oid AND p.pronargs = 1
AND p.proname IN (
'ope_cllw_u64_65', 'ope_cllw_var_8',
'has_ope_cllw_u64_65', 'has_ope_cllw_var_8'
))After that change, step 2 above never fires for these functions — 2. Commit Defence-in-depth suggestion (optional)
|
|
TL;DR on the above: your code is fine. The "pinner" script which is intended to set search paths correctly to satisfy splinter breaks the inlinability of the functions you added. IMHO this is brittle and we should find an alternative approach or just disable splinter until we can find a better approach. |
|
I'll approve now and address the pinning/splinter issue in a separate PR as its not really directly related to your changes. |
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Just the CLLW vs CLWW thing. Otherwise LGTM and I'll work on a follow up.
Per coderdan: Chenette-Lewi-Weis-Wu = CLWW. The earlier change in this
PR (CLWW -> CLLW everywhere "for consistency with type names") was
wrong-headed — the type names ope_cllw_u64_65 / ope_cllw_var_8 are
themselves typos that already shipped, but the cipher abbreviation in
prose docs should stay CLWW. Spreading the typo to free-text docs makes
the codebase less searchable, not more.
Reverts CLLW -> CLWW only in OPE-context references:
docs/reference/{index-config,sql-support}.md
src/ope_cllw_u64_65/{compare,functions,types}.sql
src/ope_cllw_var_8/{compare,functions,types}.sql
src/operators/order_by.sql
tests/sqlx/tests/ope_tests.rs (file-level docstring)
Leaves untouched:
- The "ORE CLLW variants" / "CLLW ORE" / "CLLW Order-Revealing"
references — those are the genuine CLLW (Copyless Logarithmic
Width) ORE cipher used by ore_cllw_u64_8 and ore_cllw_var_8.
- Type / function / file names (ope_cllw_*) — out of scope to rename.
47/47 OPE tests still pass; docs:validate clean.
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)
src/ope_cllw_var_8/functions.sql (1)
6-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemaining
CLWW→CLLWdoc typos missed by this PR.Lines 6, 8, and 12 still read "CLWW" while the function/type name is
ope_cllw_var_8(CLLW). The PR objective explicitly lists "Fix CLWW → CLLW naming to match types," so these look like an incomplete fix.📝 Proposed fix
-–! `@brief` Extract variable-width CLWW OPE index term from JSONB payload +–! `@brief` Extract variable-width CLLW OPE index term from JSONB payload –! -–! Extracts the variable-width CLWW OPE ciphertext from the 'opv' field of an +–! Extracts the variable-width CLLW OPE ciphertext from the 'opv' field of an –! encrypted data payload. Used internally for range query comparisons. –! –! `@param` val jsonb encrypted EQL payload -–! `@return` eql_v2.ope_cllw_var_8 Variable-width CLWW OPE ciphertext +–! `@return` eql_v2.ope_cllw_var_8 Variable-width CLLW OPE ciphertextLines 31, 33, and 37 have the same issue in the second function's doc block.
🤖 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 `@src/ope_cllw_var_8/functions.sql` around lines 6 - 16, The doc comments for the functions that extract the variable-width OPE term refer to "CLWW" but the correct acronym and type name is "CLLW"; update all occurrences of "CLWW" to "CLLW" in the comment blocks for the functions related to ope_cllw_var_8 (the extract function whose header mentions "Extract variable-width CLWW OPE..." and the second function's doc block referenced in the review) so the documentation matches the function/type name eql_v2.ope_cllw_var_8 and related references (also update any `@see` lines that mistakenly say CLWW).
🧹 Nitpick comments (1)
src/operators/order_by.sql (1)
55-63: Post-install pinner will silently remove inlinability.The PR discussion already documents this, but it's worth restating concisely for reviewers:
tasks/pin_search_path.sqlruns after source installation andALTER FUNCTIONs every function to addSET search_path = ..., which setspg_proc.proconfigand disables PostgreSQL function inlining. The whole point of converting these helpers toLANGUAGE sqlis planner inlining (e.g., index matching on(eql_v2.order_by_ope(col))), but CI won't catch the silent regression.Two changes are needed before this PR's performance goal survives a production install:
- Extend the
inline_critical_oidsfilter intasks/pin_search_path.sqlto excludeorder_by_ope,ope_cllw_u64_65,ope_cllw_var_8,has_ope_cllw_u64_65, andhas_ope_cllw_var_8(bothencryptedandjsonboverloads).- Add allowlist entries in
tasks/test/splinter.shso the Splinterfunction_search_path_mutablelint doesn't flag these functions.A reference implementation for the same fix pattern exists in commit
1ab0b00on PR#196.🤖 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 `@src/operators/order_by.sql` around lines 55 - 63, Extend the inlining whitelist in tasks/pin_search_path.sql by adding exclusions for eql_v2.order_by_ope and its helper functions eql_v2.ope_cllw_u64_65, eql_v2.ope_cllw_var_8, eql_v2.has_ope_cllw_u64_65 and eql_v2.has_ope_cllw_var_8 (for both encrypted and jsonb overloads) so they are not ALTERed to set search_path (preserving LANGUAGE sql inlinability), and add corresponding allowlist entries in tasks/test/splinter.sh for the Splinter lint key function_search_path_mutable to avoid CI failures; use the exact symbol names above to locate the filter and splinter allowlist blocks.
🤖 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 `@src/ope_cllw_var_8/functions.sql`:
- Around line 6-16: The doc comments for the functions that extract the
variable-width OPE term refer to "CLWW" but the correct acronym and type name is
"CLLW"; update all occurrences of "CLWW" to "CLLW" in the comment blocks for the
functions related to ope_cllw_var_8 (the extract function whose header mentions
"Extract variable-width CLWW OPE..." and the second function's doc block
referenced in the review) so the documentation matches the function/type name
eql_v2.ope_cllw_var_8 and related references (also update any `@see` lines that
mistakenly say CLWW).
---
Nitpick comments:
In `@src/operators/order_by.sql`:
- Around line 55-63: Extend the inlining whitelist in tasks/pin_search_path.sql
by adding exclusions for eql_v2.order_by_ope and its helper functions
eql_v2.ope_cllw_u64_65, eql_v2.ope_cllw_var_8, eql_v2.has_ope_cllw_u64_65 and
eql_v2.has_ope_cllw_var_8 (for both encrypted and jsonb overloads) so they are
not ALTERed to set search_path (preserving LANGUAGE sql inlinability), and add
corresponding allowlist entries in tasks/test/splinter.sh for the Splinter lint
key function_search_path_mutable to avoid CI failures; use the exact symbol
names above to locate the filter and splinter allowlist blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc58da1e-d2f3-40bd-8afd-505ac5daad91
📒 Files selected for processing (8)
docs/reference/sql-support.mdsrc/ope_cllw_u64_65/compare.sqlsrc/ope_cllw_u64_65/functions.sqlsrc/ope_cllw_u64_65/types.sqlsrc/ope_cllw_var_8/compare.sqlsrc/ope_cllw_var_8/functions.sqlsrc/operators/order_by.sqltests/sqlx/tests/ope_tests.rs
✅ Files skipped from review due to trivial changes (1)
- docs/reference/sql-support.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ope_cllw_u64_65/compare.sql
- src/ope_cllw_u64_65/types.sql
- src/ope_cllw_u64_65/functions.sql
- src/ope_cllw_var_8/compare.sql
3 participants
Follow-up to #176 addressing late CodeRabbit and reviewer feedback.
Code changes
compare_ope_cllw_*STRICT cleanupa/bshort-circuit to NULL via STRICT before the body runs).eql_v2_encryptedwhose payload lacks theopf/opvfield, mirroringcompare_ore_block_u64_8_256.@notein both compare.sql files explicitly distinguishing the two NULL paths.OPE helper inlinability
ope_cllw_u64_65(eql_v2_encrypted),ope_cllw_var_8(eql_v2_encrypted), bothhas_ope_*overloads, andorder_by_opefromLANGUAGE plpgsqltoLANGUAGE sqlso the planner can inline them on the sort / order_by hot path.Doc fixes
CLWW→CLLWeverywhere — matches theope_cllw_*type names and existingore_cllw_*docs.@paramtags inope_cllw_*/functions.sqlnow name the parameter (val) before the type, matching valid Doxygen syntax.docs/reference/sql-support.md:MIN/MAXandCOUNT(DISTINCT)rows separate ste_vec-extracted-node requirements from outer-column sibling-index requirements.opeis never emitted by ste_vec extraction; it only applies to the outer column.COUNT(DISTINCT col)outer-column cell listsunique/ore/opefor parity with theMIN/MAXrow.Test changes
Operator parity —
<,<=,=,>=,>,<>exercised via theeql_v2_encryptedoperators on bothopfandopvvariants (previously only<and>were covered foropf;opvwas exercised throughcompare_*but not via operators).ORE-vs-OPE precedence —
ore_wins_over_opf_when_both_presentlocks in the ORE-before-OPE dispatch order ineql_v2.comparewhen a payload carries bothobandopf.NULL-handling parity with ORE — 18 tests covering scenarios where ORE had coverage and OPE had none:
{"opf": null}/{"opv": null}payloads dispatch through to HMAC (mirrorscompare_hmac_with_null_ore_index).compare_ope_cllw_*returns NULL for both NULL-left and NULL-right operands (STRICT contract codified).ORDER BY× ASC/DESC ×NULLS FIRST/NULLS LASTwith mixed NULL + opf rows.eql_v2.sort_comparewith mixed NULL + opf rows.eql_v2.min/eql_v2.maxover OPE-encoded values, including NULL-only → NULL.BETWEENfor bothopfandopvvariants.Brittle assertions —
config_check_rejects_unknown_indexno longer matches the exact comma-separated list of valid indexes; checks forope+ the offending token instead, so adding a new valid index won't break the test.Open review items
src/ope_cllw_u64_65/compare.sql(tobyhede, line 30): the dropped outer NULL branches drew the comment "This might have unintended consequences. Let me try and find the history." The branches are dead underSTRICT, but their removal diverges from the ORE pattern which keeps both outer (input-NULL) and inner (term-NULL) layers. Awaiting follow-up — happy to restore the outer branches as defense-in-depth if that's preferred.Summary by CodeRabbit
Bug Fixes
Documentation