feat(v3): N-block ORE comparator + ordered numeric & timestamptz#276
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:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/mutations.rs (1)
121-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the
sqlx::testfixture path before merge.These five attributes currently point at a fixture directory that CI cannot resolve. The pipeline is failing with
os error 2fortests/sqlx/tests/encrypted_domain/family/../../../fixtures/eql_v2_int4.sql, so this PR is blocking on test discovery rather than comparator behavior. Please update thepath = ...value to the actual fixture directory used by this crate foreql_v2_int4.sql.Also applies to: 206-206, 282-282, 342-342, 390-390
🤖 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/mutations.rs` at line 121, The sqlx::test attribute's fixtures path is incorrect and CI fails with "os error 2"; update the path value inside the #[sqlx::test(fixtures(...))] attribute so it points to the crate's actual fixtures directory that contains eql_v2_int4.sql (replace the current "../../../fixtures" string with the correct relative path used by this crate) and apply the same fix to the other identical #[sqlx::test(...)] attributes in this file so all five attributes reference the real fixtures location.Source: Pipeline failures
🧹 Nitpick comments (1)
tests/sqlx/src/fixtures/eql_plaintext.rs (1)
198-204: ⚡ Quick winAdd parity tests for the new
Decimalplaintext mapping.This new
EqlPlaintextimpl is the only scalar mapping in this file without the same cast / SQL-type /Plaintext-variant tests the others already have, so drift in theScalarKind::Numericwiring would be easy to miss.Suggested test additions
#[cfg(test)] mod tests { use super::*; + #[test] + fn decimal_cast_is_decimal() { + assert_eq!(<rust_decimal::Decimal as EqlPlaintext>::CAST.as_str(), "decimal"); + } + + #[test] + fn decimal_plaintext_sql_type_is_numeric() { + assert_eq!( + <rust_decimal::Decimal as EqlPlaintext>::PLAINTEXT_SQL_TYPE.as_str(), + "numeric" + ); + } + + #[test] + fn decimal_to_plaintext_wraps_in_decimal_variant() { + let value = rust_decimal::Decimal::new(314, 2); + match value.to_plaintext() { + Plaintext::Decimal(Some(inner)) => assert_eq!(inner, value), + other => panic!("expected Plaintext::Decimal(Some(3.14)), got {other:?}"), + } + } + #[test] fn i32_casts_to_int() { assert_eq!(<i32 as EqlPlaintext>::CAST.as_str(), "int"); }🤖 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/src/fixtures/eql_plaintext.rs` around lines 198 - 204, Add parity tests for the new Decimal plaintext mapping: create tests that assert (1) the EqlPlaintext impl for rust_decimal::Decimal reports ScalarKind::Numeric, (2) to_plaintext returns Plaintext::Decimal(Some(value)) for a sample Decimal, and (3) SQL cast/type roundtrip consistency similar to other scalar tests in this file (ensure SQL type mapping and literal/cast behavior matches the other scalar parity tests). Locate the EqlPlaintext impl for rust_decimal::Decimal and mirror the existing test patterns used for other scalars in this file to validate the Decimal->ScalarKind, Decimal->Plaintext variant, and SQL-type/cast wiring.
🤖 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 `@CHANGELOG.md`:
- Around line 29-30: The CHANGELOG entries for the
encrypted-domain/schema/comparator additions reference incorrect PR numbers
(e.g. the `eql_v3.timestamptz` entry shows (`#257`) and `eql_v3.numeric` shows
(`#241`)); locate the three affected entries mentioning `eql_v3.timestamptz`,
`eql_v3.numeric` and the comparator entry that references `#255/`#276 and replace
those parenthetical links with the correct PR URLs or remove the PR link if the
introducing PR is not public—ensure the text still reads cleanly and that any
remaining links point to valid PRs in the correct repository.
In `@docs/plans/2026-06-11-ore-block-comparator-n-blocks-plan.md`:
- Around line 1256-1265: Remove the stray closing fenced-code marker (```) at
the end of the spec coverage map in the document so the markdown block is
balanced; locate the trailing fence after the Design §11.1/11.2/11.3 lines in
docs/plans/2026-06-11-ore-block-comparator-n-blocks-plan.md and delete it so the
prose remains plain text without an unmatched code fence.
In `@docs/reference/adding-a-scalar-encrypted-domain-type.md`:
- Around line 329-363: The page still describes the old fixed-8-block ORE
surface (mentions symbols like eql_v3.ore_block_u64_8_256 and
src/v3/sem/ore_block_u64_8_256/* and calls timestamptz “equality-only”), so
update the doc to the width-agnostic ORE surface: replace hardcoded 8-block
names with the new generic ORE identifiers used in the repo, remove the
statement that timestamptz is equality-only (explain it supports ordered
comparisons under the new comparator), update any examples and paths that
reference the old v3/ore_block_u64_8_256 layout, and add a short cross-reference
to the 2026-06-11 ORE design doc and the new numeric/Decimal section so the
documentation is internally consistent with the merged changes.
In `@src/v3/sem/ore_block_256/functions.sql`:
- Around line 62-64: The extractor currently calls
eql_v3.jsonb_array_to_ore_block_256(val->'ob') whenever
eql_v3.has_ore_block_256(val) is true, but has_ore_block_256 treats any non-null
ob (scalar/object) as present; update the extractor (ore_block_256) to assert
that val->'ob' is a JSON array before calling jsonb_array_to_ore_block_256: call
jsonb_typeof(val->'ob') and if it is not 'array' raise an error (or return a
hard failure) so malformed non-array ob payloads are rejected at the extractor
boundary rather than producing NULLs from jsonb_array_to_ore_block_256. Ensure
you change both the block at lines ~62-64 and the similar block around lines
~73-80 referencing eql_v3.has_ore_block_256 and
eql_v3.jsonb_array_to_ore_block_256.
- Around line 32-36: The JSON-array branch currently uses array_agg which yields
NULL for zero rows and loses element order; update the SELECT that builds
eql_v3.ore_block_256 so it unnests with ordinality (use unnest(... ) WITH
ORDINALITY AS b(ord, ordinality)), aggregate with
array_agg(ROW(b.ord)::eql_v3.ore_block_256_term ORDER BY ordinality) to preserve
order, and wrap the aggregate in COALESCE(...,
ARRAY[]::eql_v3.ore_block_256_term[]) (or '{}'::eql_v3.ore_block_256_term[]) so
an empty JSON array becomes an empty terms array (ensuring
compare_ore_block_256_terms sees [] not NULL).
---
Outside diff comments:
In `@tests/sqlx/tests/encrypted_domain/family/mutations.rs`:
- Line 121: The sqlx::test attribute's fixtures path is incorrect and CI fails
with "os error 2"; update the path value inside the #[sqlx::test(fixtures(...))]
attribute so it points to the crate's actual fixtures directory that contains
eql_v2_int4.sql (replace the current "../../../fixtures" string with the correct
relative path used by this crate) and apply the same fix to the other identical
#[sqlx::test(...)] attributes in this file so all five attributes reference the
real fixtures location.
---
Nitpick comments:
In `@tests/sqlx/src/fixtures/eql_plaintext.rs`:
- Around line 198-204: Add parity tests for the new Decimal plaintext mapping:
create tests that assert (1) the EqlPlaintext impl for rust_decimal::Decimal
reports ScalarKind::Numeric, (2) to_plaintext returns
Plaintext::Decimal(Some(value)) for a sample Decimal, and (3) SQL cast/type
roundtrip consistency similar to other scalar tests in this file (ensure SQL
type mapping and literal/cast behavior matches the other scalar parity tests).
Locate the EqlPlaintext impl for rust_decimal::Decimal and mirror the existing
test patterns used for other scalars in this file to validate the
Decimal->ScalarKind, Decimal->Plaintext variant, and SQL-type/cast wiring.
🪄 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: 0a7664f8-c6d3-4de7-b2c2-61eb3425c987
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
CHANGELOG.mdcrates/eql-codegen/src/context.rscrates/eql-codegen/src/generate.rscrates/eql-scalars/src/lib.rscrates/eql-scalars/src/term.rscrates/eql-scalars/src/tests.rscrates/eql-tests-macros/src/lib.rsdocs/plans/2026-06-11-ore-block-comparator-n-blocks-design.mddocs/plans/2026-06-11-ore-block-comparator-n-blocks-plan.mddocs/reference/adding-a-scalar-encrypted-domain-type.mdsrc/v3/schema.sqlsrc/v3/sem/bloom_filter/functions.sqlsrc/v3/sem/ore_block_256/functions.sqlsrc/v3/sem/ore_block_256/operator_class.sqlsrc/v3/sem/ore_block_256/operators.sqlsrc/v3/sem/ore_block_256/types.sqlsrc/v3/sem/ore_block_u64_8_256/operators.sqltasks/pin_search_path.sqltasks/test/clean_install_v3.shtasks/test/splinter.shtests/codegen/reference/int4/int4_ord_functions.sqltests/codegen/reference/int4/int4_ord_ore_functions.sqltests/sqlx/Cargo.tomltests/sqlx/fixtures/drop_operator_classes.sqltests/sqlx/src/fixtures/eql_plaintext.rstests/sqlx/src/fixtures/scalar_fixture.rstests/sqlx/src/scalar_domains.rstests/sqlx/src/scalar_types.rstests/sqlx/tests/encrypted_domain/family/inlinability.rstests/sqlx/tests/encrypted_domain/family/mutations.rstests/sqlx/tests/encrypted_domain/family/sem.rstests/sqlx/tests/ore_block_comparator_tests.rs
💤 Files with no reviewable changes (1)
- src/v3/sem/ore_block_u64_8_256/operators.sql
| - **`eql_v3.timestamptz` encrypted-domain type family (ordered).** Four jsonb-backed domains for encrypted `timestamptz` columns — `eql_v3.timestamptz` (storage-only), `eql_v3.timestamptz_eq` (`=` / `<>` via HMAC), and `eql_v3.timestamptz_ord` / `eql_v3.timestamptz_ord_ore` (also `<` `<=` `>` `>=`, `MIN` / `MAX` via 12-block ORE) — generated from the `timestamptz` row in `eql-scalars::CATALOG` by the same materializer as the `eql_v3.date` family. Values are **UTC-normalized** (cipherstash has no timezone-preserving type): plaintexts encrypt under the `timestamp` cast. Ordering works because the `eql_v3` ORE block comparator now derives its block count from the ciphertext width (see the comparator entry below) instead of assuming 8. Index via a functional index on the `eql_v3.eq_term` / `eql_v3.ord_term` extractors, not an operator class on the domain. ([#257](https://github.com/cipherstash/encrypt-query-language/pull/257)) | ||
| - **`eql_v3.numeric` encrypted-domain type family (ordered).** Four jsonb-backed domains for encrypted `numeric` / `decimal` columns — `eql_v3.numeric` (storage-only), `eql_v3.numeric_eq` (`=` / `<>` via HMAC), and `eql_v3.numeric_ord` / `eql_v3.numeric_ord_ore` (also `<` `<=` `>` `>=`, `MIN` / `MAX` via 14-block ORE) — generated from the `numeric` row in `eql-scalars::CATALOG`. cipherstash encrypts `Plaintext::Decimal` at native 14-block ORE width; ordering matches `rust_decimal::Decimal` ordering exactly (equivalent scales such as `1` and `1.0` collide, like Postgres `numeric`). Index via a functional index on the `eql_v3.eq_term` / `eql_v3.ord_term` extractors. Why: a type-safe, ordered encrypted decimal column, the first scalar to exercise an ORE term wider than 8 blocks. ([#241](https://github.com/cipherstash/encrypt-query-language/issues/241)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the GitHub issue/PR number for this code review? Is it #276? Do PRs #241, #257, #255 predate this PR?
💡 Result:
I can’t determine the GitHub issue/PR number for “this code review” from the information provided. What I found: “#276” appears as many unrelated GitHub issues in different repositories (e.g., openid/sharedsignals issue #276 and others), but the search results you provided do not include any link, repository name, PR title, reviewer comment text, or other identifier that would uniquely tie “this code review” to a specific GitHub PR/issue number [1][2]. Also, the PRs you mentioned (#241, #257, #255) similarly show up only as incidental numbers in unrelated content (e.g., issue text lists) rather than as PR numbers in a specific repository context; therefore I can’t verify whether they predate the relevant PR [3]. If you paste the URL of the code review, the repository name, or the PR/issue title (or a snippet of the review comment that says what was changed), I can immediately identify the exact GitHub PR/issue number and answer whether #276 is correct and whether #241/#257/#255 predate it.
Citations:
- 1: editorial issues in the RISC spec under public review openid/sharedsignals#276
- 2: Force "squash and merge" title to be the pull request title refined-github/refined-github#276
- 3: importing IMGT files with repLoad immunomind/immunarch#107
🌐 Web query:
cipherstash/encrypt-query-language pull request 241
💡 Result:
There is no public pull request numbered 241 in the cipherstash/encrypt-query-language repository as of June 12, 2026 [1][2]. The repository's recent development activity includes pull requests such as #169 and #170 [3][4], and related issue tracking reaching into the 200s (e.g., issue #205) [5], but pull request #241 does not exist or has not been made public [1][2].
Citations:
- 1: https://github.com/cipherstash/encrypt-query-language?tab=readme-ov-file
- 2: https://github.com/cipherstash/encrypt-query-language/releases
- 3: Canonical shared encryption config #170
- 4: feat(test): add benchmarking helpers and pg_stat_statements support #169
- 5: perf/correctness: emit hm (HMAC-256) at sv element level, not b3 (option 1) #205
🌐 Web query:
cipherstash/encrypt-query-language pull request 255
💡 Result:
There is no publicly available information regarding a pull request #255 in the cipherstash/encrypt-query-language repository as of June 12, 2026 [1][2]. Activity in that repository shows that pull request numbers are significantly lower than 255; for example, pull request #170 was merged in April 2026 [3]. It is possible that the pull request number was misidentified, or it may exist in a different repository within the CipherStash organization (such as cipherstash/proxy, which has seen higher PR activity) [4][5][6]. However, official sources do not reference a PR #255 for encrypt-query-language [1][2].
Citations:
- 1: https://github.com/cipherstash/encrypt-query-language?tab=readme-ov-file
- 2: https://github.com/cipherstash/encrypt-query-language/activity
- 3: Canonical shared encryption config #170
- 4: refactor: ♻️ use std::mem::size_of for accurate cipher cache sizing proxy#309
- 5: fix(deps): remediate medium vulnerabilities (CIP-2743) proxy#375
- 6: Update cipherstash-client and add contention validation tests for scoped cipher proxy#373
🌐 Web query:
cipherstash/encrypt-query-language pull request 257
💡 Result:
Based on the available information for the repositories associated with CipherStash, there is no public record of a pull request #257 in the cipherstash/encrypt-query-language repository [1]. The repository cipherstash/encrypt-query-language has a relatively small number of total pull requests compared to its development history, and recent activity, such as PR #170 [2], does not correspond to #257. Similarly, while there have been pull requests in the related cipherstash/proxy repository (such as #374, #375, and #341) [3][4][5], no specific details or existence of a "pull request 257" are documented across the public GitHub activity for these organizations [1][6]. If you have a specific reference to this pull request, it may be private, deleted, or misidentified, as it does not appear in the public tracking for the CipherStash projects [1][6].
Citations:
- 1: https://github.com/cipherstash/encrypt-query-language?tab=readme-ov-file
- 2: Canonical shared encryption config #170
- 3: Encryption sanity checks proxy#341
- 4: fix(deps): remediate medium vulnerabilities (CIP-2743) proxy#375
- 5: Add ARCHITECTURE.md proxy#374
- 6: https://github.com/cipherstash/proxy
🌐 Web query:
cipherstash/encrypt-query-language pull request 276
💡 Result:
There is no publicly available record of a pull request #276 in the cipherstash/encrypt-query-language repository as of June 12, 2026 [1][2]. The cipherstash/encrypt-query-language repository has a relatively small number of pull requests and releases [1][2]. Recent activity in this repository includes PRs such as #169 and #170 [3][4], while related repositories under the CipherStash organization, such as cipherstash/proxy, have reached higher PR numbers (e.g., #375) [5], which may be the source of confusion regarding the pull request number.
Citations:
- 1: https://github.com/cipherstash/encrypt-query-language?tab=readme-ov-file
- 2: https://github.com/cipherstash/encrypt-query-language/releases
- 3: Canonical shared encryption config #170
- 4: feat(test): add benchmarking helpers and pg_stat_statements support #169
- 5: fix(deps): remediate medium vulnerabilities (CIP-2743) proxy#375
Fix CHANGELOG.md PR link references for the added encrypted-domain/schema/comparator entries
Lines 29–30, 33, and 39–41 link to PRs #241/#255/#257 (and are tied to the narrative of this change), but cipherstash/encrypt-query-language has no public PRs with those numbers (or #276); update these links to the correct PRs that introduced the entries (or remove/replace links if the introducing PRs aren’t public).
🤖 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 `@CHANGELOG.md` around lines 29 - 30, The CHANGELOG entries for the
encrypted-domain/schema/comparator additions reference incorrect PR numbers
(e.g. the `eql_v3.timestamptz` entry shows (`#257`) and `eql_v3.numeric` shows
(`#241`)); locate the three affected entries mentioning `eql_v3.timestamptz`,
`eql_v3.numeric` and the comparator entry that references `#255/`#276 and replace
those parenthetical links with the correct PR URLs or remove the PR link if the
introducing PR is not public—ensure the text still reads cleanly and that any
remaining links point to valid PRs in the correct repository.
Source: Coding guidelines
| Spec coverage map: | ||
| - Design §3 (N-block comparator + missed `9 → 1+n` + guards) → Phase 2. | ||
| - Design §4 (rename to `ore_block_256`) → Phase 1. | ||
| - Design §5 (catalog: numeric spec + timestamptz promotion) → Task 3.1–3.2. | ||
| - Design §6 (harness wiring: macro routing, scalar_fixture arm, deps, EqlPlaintext, ScalarType/OrderedScalar, scalar_types list) → Tasks 3.3–3.8. | ||
| - Design §7 (fixtures) → Task 4.2. | ||
| - Design §8 (tests: malformed-length guards (Phase 2), ascending-chain + 702-byte numeric and 604-byte timestamptz direct assertions (Task 4.4), matrix order_by as primary ordering regression (Task 4.3), inventory/self-contained/parity gates, distinctness guard (Task 3.7)) → Phase 2 + Tasks 3.7, 4.3–4.5. | ||
| - Design §9 (changelog) → Task 5.1. | ||
| - Design §11.1/11.2/11.3 (verified-upstream notes, numeric fixture checklist, distinctness guard, timestamptz) → Tasks 3.1, 3.7. | ||
| ``` |
There was a problem hiding this comment.
Remove the stray unlabeled code fence at the end of the plan.
The spec-coverage map is plain prose, but the extra closing fence after it leaves the markdown with an unmatched fenced block. markdownlint-cli2 is correctly flagging this one.
🧰 Tools
🪛 LanguageTool
[style] ~1260-~1260: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...imestamptz promotion) → Task 3.1–3.2. - Design §6 (harness wiring: macro routing, scal...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~1261-~1261: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..., scalar_types list) → Tasks 3.3–3.8. - Design §7 (fixtures) → Task 4.2. - Design §8 (...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~1262-~1262: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...8. - Design §7 (fixtures) → Task 4.2. - Design §8 (tests: malformed-length guards (Pha...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~1263-~1263: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...3.7)) → Phase 2 + Tasks 3.7, 4.3–4.5. - Design §9 (changelog) → Task 5.1. - Design §11...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~1264-~1264: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .... - Design §9 (changelog) → Task 5.1. - Design §11.1/11.2/11.3 (verified-upstream note...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.22.1)
[warning] 1265-1265: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/plans/2026-06-11-ore-block-comparator-n-blocks-plan.md` around lines
1256 - 1265, Remove the stray closing fenced-code marker (```) at the end of the
spec coverage map in the document so the markdown block is balanced; locate the
trailing fence after the Design §11.1/11.2/11.3 lines in
docs/plans/2026-06-11-ore-block-comparator-n-blocks-plan.md and delete it so the
prose remains plain text without an unmatched code fence.
Source: Linters/SAST tools
| #### A fourth fixture shape: non-integer, non-chrono, non-text (`numeric` / `Decimal`) | ||
|
|
||
| `numeric` (backed by `rust_decimal::Decimal`, 14-block ORE — the first scalar | ||
| whose ORE term is wider than 8 blocks) is ordered but is **neither** the integer | ||
| materialiser, **nor** chrono (`temporal`), **nor** `text` (it owns a `Decimal`, | ||
| not a `String`, and has no `Match` index). It therefore introduces a **fourth | ||
| fixture discriminator**, which means touching the proc-macro routing, not just | ||
| the type list. Beyond the §3.1 `eql_plaintext.rs` wiring above (`Cast::DECIMAL`, | ||
| `PlaintextSqlType::NUMERIC`, the `cast_for_kind` / `plaintext_sql_type_for_kind` | ||
| arms, `Sealed for Decimal`, `EqlPlaintext for Decimal` → `Plaintext::Decimal`), | ||
| it also needs: | ||
|
|
||
| - an **`is_numeric_token`** arm in `crates/eql-tests-macros/src/lib.rs`'s | ||
| fixture-module router — without it `scalar_types!(fixture_modules)` panics at | ||
| compile time on the unrecognised kind (the router handled only `temporal` / | ||
| `text` before); | ||
| - a **`numeric` arm** in the `scalar_fixture!` macro | ||
| (`tests/sqlx/src/fixtures/scalar_fixture.rs`) — the temporal arm's twin | ||
| (`[Unique, Ore]`, pivot-presence asserts via `OrderedScalar`), but no `Match` | ||
| and no chrono; | ||
| - a hand-written **`numeric_values()`** accessor plus `impl ScalarType` / | ||
| `OrderedScalar for Decimal` in `tests/sqlx/src/scalar_domains.rs` — parsing the | ||
| catalog's `Fixture::Numeric` strings into a `LazyLock<Vec<Decimal>>` (the | ||
| catalog stays zero-dep; the parse lives in the harness). `Decimal: Ord` supplies | ||
| the expected sort order — `ore-rs` guarantees the ciphertext order agrees, and | ||
| equivalent scales (`1` ≡ `1.0`) collide like `Decimal`'s own `Ord`. Add a | ||
| **`fixtures_are_distinct_by_value`** guard (parse → `HashSet`): the zero-dep | ||
| catalog only dedupes by literal string, so `"1"` / `"1.0"` would slip past it | ||
| but collide in both the ORE ciphertext and the fixture table; | ||
| - the **`rust_decimal` dependency** + the sqlx **`rust_decimal` feature** in | ||
| `tests/sqlx/Cargo.toml` (in `[dependencies]`, not `[dev-dependencies]` — the | ||
| `Decimal` impls live in the crate's library code). | ||
|
|
||
| See `docs/plans/2026-06-11-ore-block-comparator-n-blocks-design.md` for the full | ||
| worked example (and the N-block ORE comparator change the wide term relies on). |
There was a problem hiding this comment.
Finish updating this reference page for the width-agnostic ORE surface.
The new numeric section is accurate, but the rest of the page still documents the pre-PR v3 surface: earlier sections refer to eql_v3.ore_block_u64_8_256 / src/v3/sem/ore_block_u64_8_256/* and describe timestamptz as equality-only because the comparator is hardcoded to 8. Leaving those sections unchanged makes the reference self-contradictory for anyone using it after this merge.
🤖 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 `@docs/reference/adding-a-scalar-encrypted-domain-type.md` around lines 329 -
363, The page still describes the old fixed-8-block ORE surface (mentions
symbols like eql_v3.ore_block_u64_8_256 and src/v3/sem/ore_block_u64_8_256/* and
calls timestamptz “equality-only”), so update the doc to the width-agnostic ORE
surface: replace hardcoded 8-block names with the new generic ORE identifiers
used in the repo, remove the statement that timestamptz is equality-only
(explain it supports ordered comparisons under the new comparator), update any
examples and paths that reference the old v3/ore_block_u64_8_256 layout, and add
a short cross-reference to the 2026-06-11 ORE design doc and the new
numeric/Decimal section so the documentation is internally consistent with the
merged changes.
tobyhede
force-pushed
the
eql-v3-ore-block-256
branch
2 times, most recently
from
951b472 to
2d3fa18
Compare
Complete the eql_v3.ore_block_u64_8_256 -> ore_block_256 rename across the docs (CLAUDE.md, the scalar guide, eql-functions, sql-support; eql_v2 names left unchanged) and the v3 SEM file header — the @file/subset comment had over-applied the rename to the v2-origin path (src/ore_block_u64_8_256). Regenerate the codegen reference goldens for every catalog type after rebasing onto eql_v3: add the numeric reference dir and expand timestamptz to its now-ordered shape so the parity gate passes. Also address review feedback: - ScalarKind::rust_type returns the now-real numeric type (rust_decimal::Decimal); only jsonb remains surfaceless. De-stale its doc and the 'timestamptz is equality-only' test comment; add numeric_maps_to_decimal. - Correct the guide's stale 'timestamptz is equality-only' prose and a dangling link to the removed design plan doc. - Add comparator_rejects_mismatched_block_widths (8-block vs 14-block terms must raise via the different-lengths guard). - Add the PR link (#276) to the numeric and N-block changelog entries.
Complete the eql_v3.ore_block_u64_8_256 -> ore_block_256 rename across the docs (CLAUDE.md, the scalar guide, eql-functions, sql-support; eql_v2 names left unchanged) and the v3 SEM file header — the @file/subset comment had over-applied the rename to the v2-origin path (src/ore_block_u64_8_256). Regenerate the codegen reference goldens for every catalog type after rebasing onto eql_v3: add the numeric reference dir and expand timestamptz to its now-ordered shape so the parity gate passes. Also address review feedback: - ScalarKind::rust_type returns the now-real numeric type (rust_decimal::Decimal); only jsonb remains surfaceless. De-stale its doc and the 'timestamptz is equality-only' test comment; add numeric_maps_to_decimal. - Correct the guide's stale 'timestamptz is equality-only' prose and a dangling link to the removed design plan doc. - Add comparator_rejects_mismatched_block_widths (8-block vs 14-block terms must raise via the different-lengths guard). - Add the PR link (#276) to the numeric and N-block changelog entries.
tobyhede
force-pushed
the
eql-v3-ore-block-256
branch
from
755a94b to
79ee0c3
Compare
Complete the eql_v3.ore_block_u64_8_256 -> ore_block_256 rename across the docs (CLAUDE.md, the scalar guide, eql-functions, sql-support; eql_v2 names left unchanged) and the v3 SEM file header — the @file/subset comment had over-applied the rename to the v2-origin path (src/ore_block_u64_8_256). Regenerate the codegen reference goldens for every catalog type after rebasing onto eql_v3: add the numeric reference dir and expand timestamptz to its now-ordered shape so the parity gate passes. Also address review feedback: - ScalarKind::rust_type returns the now-real numeric type (rust_decimal::Decimal); only jsonb remains surfaceless. De-stale its doc and the 'timestamptz is equality-only' test comment; add numeric_maps_to_decimal. - Correct the guide's stale 'timestamptz is equality-only' prose and a dangling link to the removed design plan doc. - Add comparator_rejects_mismatched_block_widths (8-block vs 14-block terms must raise via the different-lengths guard). - Add the PR link (#276) to the numeric and N-block changelog entries.
tobyhede
force-pushed
the
eql-v3-ore-block-256
branch
from
79ee0c3 to
d54918d
Compare
has_ore_block_256 used "val ->> 'ob' IS NOT NULL", which stringifies a
scalar/object 'ob' and reports it present. ore_block_256 then fed the
malformed payload into jsonb_array_to_ore_block_256, which returns NULL
instead of raising, silently degrading a structurally invalid ORE term
into a NULL comparison/index term.
Tighten the guard to require a JSON array
(jsonb_typeof(val->'ob') = 'array'); a present-but-non-array 'ob' now
RAISEs at the extractor boundary. '{}' (absent ob) and '{"ob": null}'
(JSON null) remain absent (false). Adds T5 characterization cases for
the scalar/object 'ob' presence checks and the extractor RAISE.
Addresses CodeRabbit review thread on PR #276.
CodeRabbit Autofix — 2 threads triaged & resolvedBoth were 3 days old, in Fixed — reject non-array Won't fix — empty-array → empty terms (+ Note: the DB-backed SEM test was verified to compile locally but not executed (local Docker/colima would not boot); CI runs the live suite. |
Derive the ORE block count N from term length instead of hardcoding 8, and wire the ordered numeric scalar while promoting timestamptz to ordered.
Direct comparator tests over generated fixtures: numeric terms are 702 bytes (N=14) and order across a full 14-value ascending chain spanning sign, magnitude, and fractional scale (so the left blocks decide ordering — the regression the missed 9 -> 1+n offset would fail); timestamptz terms are 604 bytes (N=12) and order 1900 < 2099. Verified end-to-end: numeric + timestamptz ordered matrix suites (211 each, incl. < <= > >= / ORDER BY / MIN / MAX) green; full SQLx suite 2026 passed; test:matrix:inventory reconciles both new ordered types; self-contained v3 install green.
…stamptz CHANGELOG: rewrite the timestamptz entry (ordering now ships), add the numeric ordered-domain entry, and add a Fixed entry for the N-block ORE comparator generalization + the eql_v3 ore_block_u64_8_256 -> ore_block_256 rename. Reference guide: document the fourth (numeric/Decimal) fixture discriminator — proc-macro routing, scalar_fixture arm, numeric_values accessor + distinctness guard, rust_decimal dep.
Complete the eql_v3.ore_block_u64_8_256 -> ore_block_256 rename across the docs (CLAUDE.md, the scalar guide, eql-functions, sql-support; eql_v2 names left unchanged) and the v3 SEM file header — the @file/subset comment had over-applied the rename to the v2-origin path (src/ore_block_u64_8_256). Regenerate the codegen reference goldens for every catalog type after rebasing onto eql_v3: add the numeric reference dir and expand timestamptz to its now-ordered shape so the parity gate passes. Also address review feedback: - ScalarKind::rust_type returns the now-real numeric type (rust_decimal::Decimal); only jsonb remains surfaceless. De-stale its doc and the 'timestamptz is equality-only' test comment; add numeric_maps_to_decimal. - Correct the guide's stale 'timestamptz is equality-only' prose and a dangling link to the removed design plan doc. - Add comparator_rejects_mismatched_block_widths (8-block vs 14-block terms must raise via the different-lengths guard). - Add the PR link (#276) to the numeric and N-block changelog entries.
has_ore_block_256 used "val ->> 'ob' IS NOT NULL", which stringifies a
scalar/object 'ob' and reports it present. ore_block_256 then fed the
malformed payload into jsonb_array_to_ore_block_256, which returns NULL
instead of raising, silently degrading a structurally invalid ORE term
into a NULL comparison/index term.
Tighten the guard to require a JSON array
(jsonb_typeof(val->'ob') = 'array'); a present-but-non-array 'ob' now
RAISEs at the extractor boundary. '{}' (absent ob) and '{"ob": null}'
(JSON null) remain absent (false). Adds T5 characterization cases for
the scalar/object 'ob' presence checks and the extractor RAISE.
Addresses CodeRabbit review thread on PR #276.
… fixture Strengthen the N-block ORE comparator tests so they run creds-free on no-creds CI shards, sourcing real ORE terms from committed fixtures: - assert_orders_like_oracle: all-pairs oracle agreement + antisymmetry (replaces the adjacent-pair-only ascending chain), with a row-count drift guard against the catalog fixture order - comparator_length_guard_sweep: boundary/off-by lengths for the 49*N+16 guard across N=1..14 - wide_block_term_compares_equal_to_itself: reflexive path at N=14/12 - numeric_scale_equivalents_collide: 1 == 1.0 ORE collision via the new hand-written v3_numeric_collision fixture (the value-equal pair the catalog distinctness guard forbids in eql_v2_numeric) Also fix the stale timestamptz comment in scalar_domains.rs (now ordered, not eq-only).
The eql-types crate landed on eql_v3 (PR #236) after this branch forked, so merging the base in surfaced a catalog_parity failure: CATALOG now has the numeric family and ordered timestamptz, but v3::all() didn't. - numeric.rs: four ordered domains (storage/_eq/_ord/_ord_ore), mirroring date.rs; numeric is the first scalar with a >8-block ORE term (14) - timestamptz.rs: add the two ordered domains; the eq-only/8-block-limit rationale is gone now that eql_v3.ore_block_256 derives N from term length - mod.rs: register the six new domains in all(), in CATALOG order - terms.rs: the ob term's SQL constructor is eql_v3.ore_block_256 (renamed this branch) and is width-agnostic (8/12/14 blocks) - v3_conformance.rs: cover numeric + timestamptz ord wire shapes; drop the stale equality-only claim - README: timestamptz no longer eq-only; add numeric
tobyhede
force-pushed
the
eql-v3-ore-block-256
branch
from
df13221 to
61c5f50
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
This isn't correct. The current version of ore.rs is always 8-blocks output. To handle longer values, 8-block chunks are created in an array.
However, the upcoming changes to ore.rs will support a variable length encoding. I suggest holding off on this work until that lands.
|
@coderdan ore-rs decimal.rs (N=14): https://docs.rs/crate/cipherstash-client/0.35.0/source/src/encryption/ore_indexer/mod.rs I've pushed a more detailed analysis. |
|
OK cool. I guess it was updated. |
2 participants
Generalizes the
eql_v3ORE block comparator from hardcoded 8 blocks to anyN, and ships two new ordered scalar families on top. Closes #241.What changed
N-block comparator (the fix). Derive block count from term length instead of assuming 8:
9 → 1+n) — the bit Generalize ORE block comparator to N blocks (not hardcoded 8) for wide-term ordering (decimal/timestamp) #241's sketch missed.49·N + 16, N ≥ 1. The≤ 16guard is load-bearing: a 16-byte term →N=0→ silent all-equal fall-through.N=8(int4/int2/int8/date unchanged).Rename
eql_v3.ore_block_u64_8_256 → eql_v3.ore_block_256(v3 SEM only — width-honest nowNvaries).eql_v2.ore_block_u64_8_256untouched.eql_v3.numeric— new ordered family, 14-block ORE (terms = 702 bytes). Backed byrust_decimal::Decimal;1and1.0collide like Postgresnumeric.eql_v3.timestamptz— promoted equality-only → ordered, 12-block ORE (terms = 604 bytes).EQ_ONLY → ORDERED, nothing else.