Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 17171df The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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 configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review of 7b30b04 (this PR's diff against feat/stack-encrypt-order-keys only). No prior review comments existed. I found nothing that blocks; three low-severity notes are inline and summarised below.
Findings (most severe first)
- Low — the ordered fixtures pin order-term bytes that the base PRs may still move.
v4_ordered_queries.json(Rust-derived, checked by Go) andv4_ordered_rows.json(Go-sealed, checked by Rust) both holdophex that depends on how #1235/#1242 settle thestored_rowsquestion. If either base changes the bytes before this merges, the GoTestV4OrderedQueriesMatchTheRustFixture/TestV4OrderedRowsMatchTheCommittedFixtureand the Rustthe_ordered_query_fixture_is_what_the_dispatch_derives/ordered_rows_the_go_sdk_sealed_open_in_rustall fail on this branch after a rebase. Regenerable (EQL_UPDATE_FIXTURES=1on both sides, Go first then Rust, or the other way for the queries file), but worth a line in the PR's rebase notes. The PR already says there is no frozenstored_v4_ordered_rowsfixture yet; the fixtures README (inline) should say so too so nobody treats these two as frozen. - Low —
encryption_gap's fallback comment is stale. Theelsebranch comment says it is reached by an "equality-only domain"; withord/ord_openow inENCRYPTION_DOMAINS, any unlisted OPE-only domain of a family with a plaintext (e.g.smallint's once it gains a kind) lands there too. The message itself is generic enough; only the comment is wrong. Inline. - Low — a README note on indexing would close the loop with the non-superuser story. The Go README says to sort with
ORDER BY eql_v4.ord_term(col)but not how to index it. Since the OPE term is abyteadomain, a plain functional btree oneql_v4.ord_term(col)works with no operator class, which is exactly why these types are the portable choice on a non-superuser install where the ORE opclass is skipped. One sentence would spare a reader reaching for the ORE opclass. Inline; optional.
Informational, no change asked: Eq on a TextOrd/TextOrdOpe field always derives op alongside hm, so =/<> with text that has no fold (an unassigned code point) is refused even though = reads only hm. Harmless in practice because the same text cannot be stored in that column, so no row could match; and the crate README already says "refused, stored or queried".
Verified as sound
- Bytes written to
opare what the SQL reads.OpeVisitorwriteshex::encode(term.as_bytes())of cllw-ore'sOpeCllw8VariableV1;eql_v4_internal.ope_cllw(jsonb)isdecode(val->>'op','hex')::bytea, and everylt/lte/gt/gte/eq/neqwrapper on the*_ord/*_ord_opedomains comparesord_term(a) <op> ord_term(b), i.e. PostgreSQLbyteaorder (memcmp, shorter prefix first). cllw-ore documents the OPE ciphertext as exact under lexicographic byte order (+128signal dominates the backward carry) and its ownOrdis derived byte order, so<on terms is<on order keys. The query side uses the sametarget::ope()plan under the same(table, column)CallerContext, andeach_ordered_type_stores_its_catalog_termspinsop(query) == op(stored)for all ten types. - Order equals plaintext order per family. Integers:
i32/i64go in at their ownOrderTargetwidth (I32/I64), andinteger_terms_order_by_valuecoversMIN < -70000 < -1 < 0 < 1 < ... < MAX, so signed values cross zero correctly. Floats: bothf32andf64map toOrderTarget::F64;float_terms_order_as_postgres_float8_doespins-inf < MIN < ... < -0.0 = 0.0 < ... < +inf < NaN = NaN(other payload), matchingfloat8sort, anda_real_orders_as_the_double_it_widens_topins the widening. Text:text_terms_order_by_the_foldpins the empty string, prefix-first (al<alice), case/accent ties (alice=ALICE,cafe=Café=CAFE+combining), andtext_order_ties_are_not_equalpins thathmstill differs and equalsTextEq's. Fixture lengths agree with cllw-ore's8n+1: 66 hex fori32, 130 fori64/f64and for widenedf32, and folded-text length + 1 for text. - Widths and refusal.
run_targetgoes throughPlaintext::from_frame, which refuses any other kind rather than narrowing or widening; the generated structs pin#[stash(plaintext = i32|i64|f32|f64|String)]; the SQL does not care about width, only the domain name, so aninteger_ordcolumn and abigint_ordcolumn cannot be cross-compared by accident. Go range-checks wider integer fields before sealing (pre-existing). - Twins.
IntegerOrd/IntegerOrdOpe(and the other families) both declareindexes: ["ope"]and store oneop;ord_and_ord_ope_store_one_termpins it.*OrdOrestays unproducible with the block-ORE reason.TextOrd/TextOrdOpestorehm+op, matching thetext_orddomain CHECK (v,i,c,hm,op) and thequery_text_ordCHECK (v,i,hm,op, noc);=/<>on text readeq_term, orderings readord_term. betweencannot leak through.target_operatorsfilterstakes_pair()andanswer()refuses an operator the target does not list asOperatorNotAnswered, so a hand-builtbetweenplan on an OPE target is refused rather than producing a one-bound operand. The Go/guest catalog tests assert exactlylt,lte,gt,gte(+eqfirst on text).- Docs vs non-superuser installs. Nothing here points at the ORE opclass; the Go README's term table already says
Opecompares in SQL with<,>,ORDER BYandOremust not. Consistent with OPE being the portable path. - Changeset. The bot's "No Changeset found" is a false alarm: the PR extends the existing unreleased
@cipherstash/eqlminor changeset rather than adding a file. That is the right package:eql-bindingsships in lockstep with the npm package, and the Go module has no changeset (its CHANGELOG is edited by hand here). No TS package or skill surface changes.
Checks run (no credentials, worktree at 7b30b04)
packages/eql:mise run types:check(regenerates the 27 binding files andlanguages/golang/encrypt/eql/eql_gen.go: no drift) andmise run codegen:parity: both clean.cargo test --locked -p eql-codegen,-p eql-bindings --all-features, and-p eql-encryption-tests --test v4_order --test targets --test v4_types: all pass (v4_order12 passed, the 2 PostgreSQL tests ignored without a database; CI runs them viatest:encryption:postgresintest-eql.yml).cargo clippy --locked -p eql-bindings -p eql-codegen -p eql-encryption-tests --all-features --all-targets -- -D warnings: clean.- Built all four Go guests (
wasm:guest:build,:eql,:deterministic,:eql:deterministic), thengo generate ./...inlanguages/golang: no diff;go vet ./...clean;go test ./encrypt/... ./stashgen/ ./cmd/...: pass, including the three newTestV4Ordered*tests against the deterministic EQL guest. - Not run: the PostgreSQL half of
v4_order(no local EQL v4 database). I read the generated SQL it exercises instead.
7b30b04 to
82ab168
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Incremental re-review of 82ab168 against the previous review on 7b30b04 (review 5563357370). Between the two heads the code diff is five files: the changeset split, the Go README indexing note, the encryption_gap comment and the fixtures README note. The Go and PostgreSQL test commit was already on 7b30b04, but I read it again closely here, as asked, and ran its PostgreSQL half locally, which the earlier review could not do.
Prior findings
- Fixtures README note on the ordered fixtures: fixed.
fixtures/README.mdnow says that neither ordered fixture is frozen, that both are regenerated together (Go writes the rows, Rust writes the queries), and that nostored_v4_ordered_rows_*exists yet. One wording nit is inline. - Stale
encryption_gapcomment: fixed. Theelsebranch ineql-codegen/src/bindings.rsnow says it is reached by any unlisted domain whose terms are neither ORE nor match-and-order. - Go README indexing sentence: fixed.
encrypt/README.mdnow saysord_termreturns abyteadomain, so a plainCREATE INDEX ON t (eql_v4.ord_term(col))serves<,>andORDER BYwith no operator class, and that this is why these types work on a non-superuser install.
The changeset is also cleaner now. The PR adds its own .changeset/eql-v4-ordered-ope-types.md (@cipherstash/eql minor) and no longer edits eql-v4-bindings.md, which matches the base branch exactly.
New findings
- Low: the PostgreSQL test of the Go-sealed rows checks only
<, so it never sorts Go's rows in SQL.postgres_compares_rows_the_go_sdk_sealedruns one<query per column. The probe ties row 2, so every column expects[1]. That catches a Go term for row 1 or row 2 on the wrong side of the probe. It cannot catch a row-3 term that ties the probe or row 2, because the only check on row 3 is that it is not below the probe.ORDER BY eql_v4.ord_term(col)over Go's bytes is never run either. The Rust-sealed test covers those operators, and the Go test checks the byte order of Go's terms, so this is a coverage gap rather than a known bug. Adding the other operators costs nothing: I ran them against the committed fixtures (details inline) and they give the expected answers. Details inline. - Nit: "the base build" in the fixtures README will be unclear once this merges. Inline.
Informational, no change asked: unlike v4_rows.json, the ordered rows have no CI step that seals them fresh in Go and hands them to Rust (the EQL_V4_ROWS_OUT / EQL_V4_ROWS step in tests-golang.yml). Rust only opens the committed ciphertexts. I don't think that matters here. TestV4OrderedRowsMatchTheCommittedFixture compares everything except c (so op and hm are drift-checked) and checks that the committed c still opens. A change in how Go writes the plaintext width would also move the op length (66 hex vs 130), which that check catches. The fresh hand-off for v4_rows already covers the AEAD agreement between the Go guest and native Rust.
Verified as sound
- CI runs it.
packages/eql/mise.toml'stest:encryption:postgresnow loops overtext_eq v4_types v4_orderand refuses an empty--ignoredselection. TheRust workspace cratesjob intest-eql.ymlruns it against the EQL v4 bundle built from the tree. On this head, the job log shows bothpostgres_compares_and_orders_ope_rows ... okandpostgres_compares_rows_the_go_sdk_sealed ... ok. - The keys match. The query fixture is derived under
FakeDataKeySource. The rows are sealed under the deterministic guest andtestSeed.TestV4OrderedQueriesMatchTheRustFixturederives the same query bytes from Go'sLt/Gte/Equnder the guest, so both sides use the same index key and<across them means something. - The expectations tell a fold from raw bytes.
ZucchinivsMangoandCarol/Alicevscarolsort differently by byte than by fold, so a case-sensitive term would fail thetitle/handlechecks. The float plaintexts are exact inf32, sof64::from(p.speed) < plain.as_f64()has no rounding edge.!expected.is_empty() && expected.len() < rows.len()stops a column from passing on an empty or full result. - The fixtures are deterministic and drift-checked. The Go side compares the fixture with
cstripped and only rewrites the file underEQL_UPDATE_FIXTURESwhen something other thancmoved. The Rust side asserts the query fixture byte for byte.tests-golang.yml's path filter includespackages/eql/tests/encryption/**, so regenerating the Rust query fixture triggers the Go check. TestV4OrderedFieldsSealToTheirEQLTypesAndOpenBackcovers the cases JSON cannot hold (the infinities and-0.0): strict term order between consecutive rows, the row 2/3 tie, and the sign of zero after a round trip.
Checks run (worktree at 82ab168, no credentials)
cargo test --locked -p eql-codegen,-p eql-bindings --all-features,-p eql-encryption-tests: all pass (v4_order: 12 passed, 2 ignored).mise run test:encryption:postgresagainst a throwaway local PostgreSQL 17 with the tree's v4 bundle:text_eq1,v4_types3,v4_order2, all pass.- Ad hoc SQL over the committed
v4_ordered_rows.jsonandv4_ordered_queries.json, per column:ORDER BY eql_v4.ord_term(col) DESCgives3,2,1,>gives[3]and>=gives[2,3].=gives[2]on the eight number columns and nothing ontitle/handle, as expected, because=reads the case-sensitivehmand row 2 ismango/CarolagainstMango/carol. - Built the four encrypt guests and the auth guest. In
languages/golang:go generate ./...leaves no diff,go vet ./...is clean, andgo test ./encrypt/... ./stashgen/... ./cmd/...passes, including the threeTestV4Ordered*tests run with-v. - Not run: the repo's
scripts/__tests__vitest suite (nonode_modulesin this worktree). CI is green on this head.
| ), | ||
| queries[*column]["query"].as_str().unwrap(), | ||
| ); | ||
| assert_eq!(found, expected, "< over Go-sealed {column}"); |
There was a problem hiding this comment.
Low: this is the only SQL comparison of Go-sealed rows, and with the probe tying row 2 it expects [1] in every column. It catches row 1 or row 2 landing on the wrong side of the probe. Row 3 only has to be not below the probe, so a Go-sealed row-3 term equal to row 2's would still pass. ORDER BY eql_v4.ord_term(col) over Go's bytes is never run.
The extra assertions are free. I ran them by hand against the committed fixtures on a local EQL v4 install: ORDER BY eql_v4.ord_term(col), id gives [1, 2, 3] in every column, > gives [3] and >= gives [2, 3]. = gives [2] on the number columns and [] on title/handle, because = reads the case-sensitive hm and row 2 is mango/Carol against Mango/carol. To cover them, derive an above set from the plaintexts the same way below is derived, assert it for > (and its complement for <=), and assert that SELECT id FROM go_rankings ORDER BY eql_v4.ord_term({column}), id returns the ids sorted by plaintext. That would make this test hold the commit message's claim that Go's rows order in PostgreSQL, rather than just split around one probe.
There was a problem hiding this comment.
Fixed in f3ddc0f. The test now gives each column an order key derived from the fixture's plaintexts and checks all four comparisons against Go's rows: < gives the rows below the query, > the rows above it, <= and >= their complements. It also checks that ORDER BY eql_v4.ord_term(col), id returns the ids sorted by plaintext. A guard fails if the fixture stops having rows on both sides of the query. = is left out, because on text it reads the case-sensitive hm, which you noted. These tests need a database, so CI's test:encryption:postgres job is what runs them. I could not run them locally this time.
| | `stored_v4_rows_v1.json` | copied from `v4_rows.json` at the next build | **never: frozen** | | ||
|
|
||
| `v4_ordered_queries.json` and `v4_ordered_rows.json` are not frozen: their | ||
| `op` bytes are the order terms of the base build, so a change that moves |
There was a problem hiding this comment.
Nit: "the order terms of the base build" will be unclear once this merges, because a reader of main has no base build to look at. Something like "the order terms Stack Encrypt writes today" would say the same thing.
There was a problem hiding this comment.
Fixed in f3ddc0f. It now says "the order terms Stack Encrypt writes today".
fa4652a to
c1bdf66
Compare
82ab168 to
31563a5
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation — automated Claude Code review (/code-review medium, then cross-checked against the PR diff) of head 31563a5.
Critical / blocking: none found.
Cross-checked findings (2, both non-blocking, inline):
- Nit: generated godoc grammar ("a int32") — fix in the
eql-codegentemplate. - Minor: Go order test hard-codes the tie row (
ID == 2).
Raised in review, not posted (unconfirmed or judgment calls):
- README table lists
=/<>for number Ord types while the prose says noEqmethod is generated. The table appears to describe SQL operators, so it is likely consistent, but worth a wording check. TextOrd/TextOrdOpereject text containing code points unassigned in Unicode 16 on the whole write. I could not confirm this in the diff. If it is true, consider noting it in the changeset.v4/targets.rsadds ~30 near-identical match arms (macro/table candidate).- The Postgres order tests are
#[ignore]d and run only via the misev4_ordersuite. Confirm CI runs--ignoredfor it.
Generated by Claude Code
|
|
||
| // IntegerOrd is the EQL type stored in public.eql_v4_integer_ord: the integer family with the ope index. | ||
| // The engine cannot produce it yet: the engine does not yet derive this family's order terms over ADR-0002's order keys (#1236). | ||
| // The engine produces it from a int32. |
There was a problem hiding this comment.
Nit: generated godoc reads "from a int32" / "a int64" (also a float32, a float64). Fix in the eql-codegen template rather than here; a hand edit trips the types:check drift gate.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 352bd11, in the eql-codegen template (go_eql.rs), not by editing the generated file. The template now picks "an" before a kind that starts with a vowel, so the godoc reads "from an int32", "from an int64", and still "from a float32" / "from a float64" / "from a string". eql_gen.go is regenerated in the same commit, and a codegen test pins the wording. The template line is older than this PR. It only shows up now because this PR makes the int32 and int64 types producible.
| for i := 0; i+1 < len(encrypted); i++ { | ||
| a := orderTerm(t, storedRanking(encrypted[i])[column]) | ||
| b := orderTerm(t, storedRanking(encrypted[i+1])[column]) | ||
| tie := rankings[i].ID == 2 |
There was a problem hiding this comment.
Minor: the tie is pinned by the literal rankings[i].ID == 2. Reordering or editing rankings would silently move the tie assertion to the wrong pair. Derive it from the fixture (e.g. compare equal plaintexts) instead.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in f3ddc0f. The tie is now written down next to the data as rankingTies, keyed by the pair of row IDs, and the loop looks pairs up there instead of checking ID == 2. The text ties need Unicode case folding to derive, and this module has no dependency for that, so the tie is recorded instead. Three guards stop it going stale. The test fails if no tie is recorded. It fails if a recorded pair is not next to each other in rankings. It fails if a recorded pair's number fields are not equal in Go. I checked that each guard fails when I break it on purpose.
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-language encrypted ordering and unsettled stacked wire-format dependencies require final human validation.
No open issues 🎉
2 files not reviewed
languages/golang/encrypt/eql/eql_gen.go: Generated filelanguages/golang/encrypt/internal/testusers/ranking_stash.go: Generated file
What changed in this PR
Connects Stack Encrypt’s order-preserving encryption to ten EQL v4 types, enabling encrypted numeric and text sorting and range comparisons through the Go SDK.
Changes:
- Enables ordered Rust types, query operands, and generated Go catalog entries.
- Adds cross-language fixtures and ordering coverage.
- Documents comparison semantics, indexing, and remaining limitations.
| File | Description |
|---|---|
| packages/eql/tests/encryption/tests/v4_types.rs | Checks stored and queried order terms match. |
| packages/eql/tests/encryption/tests/targets.rs | Updates producible-type expectations. |
| packages/eql/tests/encryption/fixtures/v4_ordered_rows.json | Adds Go-sealed interoperability rows. |
| packages/eql/tests/encryption/fixtures/v4_ordered_queries.json | Adds Rust-derived query fixtures. |
| packages/eql/tests/encryption/fixtures/README.md | Explains ordered-fixture regeneration. |
| packages/eql/mise.toml | Includes PostgreSQL ordering tests. |
| packages/eql/crates/eql-codegen/src/targets.rs | Updates availability checks and explanations. |
| packages/eql/crates/eql-codegen/src/bindings.rs | Enables ten ordered domains. |
| packages/eql/crates/eql-bindings/src/v4/text.rs | Adds ordered text encryption derives. |
| packages/eql/crates/eql-bindings/src/v4/targets.rs | Extends by-name encryption and query dispatch. |
| packages/eql/crates/eql-bindings/src/v4/real.rs | Adds ordered f32 encryption derives. |
| packages/eql/crates/eql-bindings/src/v4/integer.rs | Adds ordered i32 encryption derives. |
| packages/eql/crates/eql-bindings/src/v4/double.rs | Adds ordered f64 encryption derives. |
| packages/eql/crates/eql-bindings/src/v4/bigint.rs | Adds ordered i64 encryption derives. |
| packages/eql/crates/eql-bindings/src/encryption.rs | Encodes native OPE terms into EQL. |
| packages/eql/crates/eql-bindings/README.md | Documents ordering semantics and limitations. |
| packages/eql/crates/eql-bindings/CHANGELOG.md | Records ordered-type support. |
| languages/golang/stashgen/refusal_test.go | Uses an unsupported ORE type for refusal coverage. |
| languages/golang/encrypt/README.md | Documents ordered types and indexing. |
| languages/golang/encrypt/query_test.go | Checks advertised ordering operators. |
| languages/golang/encrypt/internal/testusers/ranking.go | Defines the ordered test model. |
| languages/golang/encrypt/internal/testusers/ranking_stash.go | Adds generated encryption and query methods. |
| languages/golang/encrypt/guest/tests/native_ops.rs | Checks guest catalog types and operators. |
| languages/golang/encrypt/guest/src/targets.rs | Updates guest catalog expectations. |
| languages/golang/encrypt/eql/eql_test.go | Checks newly producible Go types. |
| languages/golang/encrypt/eql/eql_gen.go | Regenerates type availability and documentation. |
| languages/golang/encrypt/eql_v4_order_test.go | Tests ordering, round trips, and fixture compatibility. |
| languages/golang/encrypt/doc.go | Documents generated ordering methods. |
| languages/golang/cmd/stashgen/README.md | Lists ordered targets and query support. |
| languages/golang/CHANGELOG.md | Records Go ordered-type support. |
| .changeset/eql-v4-ordered-ope-types.md | Adds the EQL minor-release entry. |
Review effort: Balanced
31563a5 to
f3ddc0f
Compare
c1bdf66 to
2d97f21
Compare
f3ddc0f to
238aee3
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Incremental re-review of f3ddc0f against the previous re-review at 82ab168 (review 5563555083). Both of the PR's commits were rewritten, and the range-diff is about 350 lines over five files: eql-codegen/src/go_eql.rs and the regenerated eql_gen.go (the godoc article), eql_v4_order_test.go (the recorded ties), the fixtures README (one sentence) and v4_order.rs (the Go-sealed PostgreSQL test).
Rebase noise versus real changes
There is none to separate. The merge base with feat/stack-encrypt-order-keys is c1bdf66c for both 31563a5 and f3ddc0f, so every line in the range-diff is the author's. #1235's branch was force-pushed since (4392b57 to ba29e03), but that push touches only packages/stack-encrypt/src/sem/mod.rs and tests/heap_hygiene.rs (the Orderable::wipe heap hygiene) and moves no order-term bytes; #1242, and so this PR, still sit on 4392b57. No fixture in this PR changed between the two heads: v4_ordered_queries.json and v4_ordered_rows.json are byte-identical to what 82ab168 carried, and neither frozen file is touched. Nothing was regenerated, and nothing needed to be.
Prior findings
- Low, the Go-sealed PostgreSQL test checked only
<: fixed.postgres_compares_rows_the_go_sdk_sealednow derives a per-columnKeyfrom the fixture plaintexts (ASCII fold for text,i64/f64for numbers), asserts<,<=,>and>=against the sets the keys predict, guards that the fixture has rows on both sides of every probe, and assertsORDER BY eql_v4.ord_term(col), idreturns the ids sorted by plaintext. Every probe inv4_ordered_queries.jsonties row 2 (Mango/mango,carol/Carol, and the eight numbers exactly), so<=expects[1, 2]and>=expects[2, 3]on every column, which is the row-3 and tie coverage the earlier review asked for. The author could not run it locally; CI did on this head: theRust workspace cratesjob's PostgreSQL step shows bothpostgres_compares_rows_the_go_sdk_sealed ... okandpostgres_compares_and_orders_ope_rows ... ok. - Nit, "the base build" in the fixtures README: fixed. Now "the order terms Stack Encrypt writes today".
The two items from the other reviewer at 31563a5 are also fixed as described: the article is chosen in the go_eql.rs template (an before a vowel) with a codegen test pinning "from an int32" and rejecting "from a int", and the only kinds the catalog emits are bool, float32, float64, int32, int64 and string, so the rule is exhaustive for what it renders. The Go tie is now rankingTies, keyed by adjacent row IDs, with the three guards the author describes (non-empty, adjacent in rankings, number fields equal in Go). -0.0 != 0.0 is false in Go, so the row 2/3 numeric guard passes for the right reason, and the text side of a recorded tie is still checked through the engine (a non-tie would store different terms and trip the tie && a != b arm).
New findings
- Nit (inline):
side()returns ids in fixture order while the SQL returnsORDER BY id, so the four comparisons pass only whilefixtureRankingsis written in ascending id order. Sortingside()'s result would make the assertion about set membership rather than row order.
Nothing else. The rest of the rewritten test (the Key enum, query_key, the sort by (Key, id) with partial_cmp over finite keys) reads correctly for the fixture it is given.
Checks run (worktree at f3ddc0f, no credentials)
packages/eql:cargo test --locked -p eql-bindings --all-features(115 unit + integration suites, all pass),-p eql-codegen(154 unit, parity, CLI,go_eql_parity, all pass),-p eql-encryption-tests(v4_order12 passed, 2 ignored without a database;v4_types13 passed, 3 ignored).mise run types:check: no drift.- Built the four encrypt guests; in
languages/golang:go vet ./...clean,go generate ./...leaves no diff,go test ./encrypt/...passes, and the threeTestV4Ordered*tests pass with-vagainst the deterministic EQL guest. gh pr checks 1266: every check green on f3ddc0f, including the PostgreSQL half ofv4_order.- Not run locally: the PostgreSQL tests (no local EQL v4 database this time); CI's run on this head covers them.
| }; | ||
| for (column, _, query_domain) in COLUMNS { | ||
| let probe = query_key(column, &queries[*column]["plaintext"]); | ||
| let side = |keep: &dyn Fn(&Key) -> bool| -> Vec<i32> { |
There was a problem hiding this comment.
Nit: side() yields ids in the order fixture.rows lists them, while every SQL it is compared against says ORDER BY id. That agrees today because fixtureRankings is written as 1, 2, 3, but a row appended out of id order (or the list reordered) would fail all four comparisons for that column with a message that reads as a wrong result from PostgreSQL. A .sort() on the collected ids (or on below/above/not_below/not_above) would make the assertion about which rows, not which order. Same shape as the ID == 2 tie that this head just removed from the Go side.
There was a problem hiding this comment.
Fixed in 3e96092. side() sorts the ids it collects, so each </<=/>/>= assertion is about which rows match, whatever order fixture.rows lists them in.
2d97f21 to
ea26f9d
Compare
238aee3 to
68cec8c
Compare
ea26f9d to
c634933
Compare
68cec8c to
3e96092
Compare
|
Restacked onto the new #1242 and force-pushed. Rebasing over #1267 (smallint, now on |
3e96092 to
548c546
Compare
c634933 to
72dc64c
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Incremental re-review of the restack to 548c546 (onto #1242's 72dc64c). Range-diff against 68cec8c: both commits were rewritten, and the diff is the smallint integration plus one response to the last review.
Rebase adaptations (smallint, from #1267 on next). The restack note is accurate: SmallintOrd and SmallintOrdOpe are producible, and every list agrees. ENCRYPTION_DOMAINS gains ("smallint","ord") and ("smallint","ord_ope") in the same position as the other families; the generated v4/smallint.rs derives both with #[stash(plaintext = i16)]; v4/targets.rs has the TARGETS rows (producible, indexes: ["ope"]) and the encrypt/decrypt/query _named arms; eql_gen.go has both rows Producible: true with the "from an int16" godoc; eql_test.go, query_test.go, the guest's targets.rs and native_ops.rs, the codegen targets.rs test loop, tests/targets.rs, v4_order.rs (ORDERED, the frame::<i16> branch, an i16 sorts_as over [MIN, -300, -1, 0, 1, 255, 256, MAX], and the Ord/OrdOpe pairing), both CHANGELOGs, both READMEs, doc.go, the stashgen README and the changeset all list them. The encryption_gap fallthrough no longer has a smallint case to reach, and the date/timestamp/numeric reason text now comes from next. eql_gen.go is the codegen's output: mise run types:check at this head no drift. No fixture changed: v4_ordered_queries.json, v4_ordered_rows.json and the frozen files are byte-identical to 68cec8c.
Substantive change. v4_order.rs's side() now sorts the ids it collects, so each </<=/>/>= assertion is about membership. That closes the nit from review 5566356476 (thread). Fixed.
New finding
- LOW / informational (inline on
ranking.go): smallint's ordered types are producible from Go but nothing cross-checks them.Rankinghas noint16field, sov4_ordered_rows.json,v4_ordered_queries.json, the threeTestV4Ordered*tests and the PostgreSQLrankings/go_rankingscolumns cover the other five families only; the Rust-sidesorts_asis the only smallint ordering coverage. The OPE term is width-dependent (anI16target gives a 17-byte term,I32a 33-byte one), so a Go field sealed at the wrong width would write terms that never compare with Rust's and no test would say so. By construction it is right (the guest sends anInt16frame andPlaintext::from_framerefuses any other kind), so this is unpinned rather than wrong. Oneint16field onRankingand a regeneration of both ordered fixtures would close it.
Nit: the PR body still says "Not done here: the Smallint family", "from 12 of 51 to 22" and lists ten types; the restack comment is right, the body is not.
Checks (no credentials): CI green at this head. mise run types:check at this head: no drift. At the top of the stack (#1280's 2189d5a, which contains both commits): cargo test --locked -p eql-codegen -p eql-bindings --all-features (162 + 116 unit, all integration suites pass), -p eql-encryption-tests (v4_order 12 passed, 2 ignored without a database), mise run types:check no drift, cargo nextest run -p stack-encrypt --all-features 695 passed, go test ./stashgen/... ./cmd/stashgen/... ok, go vet clean.
| Score float64 `stash:"score,encrypt_into=DoubleOrd"` | ||
| Mean float64 `stash:"mean,encrypt_into=DoubleOrdOpe"` | ||
| Title string `stash:"title,encrypt_into=TextOrd"` | ||
| Handle string `stash:"handle,encrypt_into=TextOrdOpe"` |
There was a problem hiding this comment.
LOW / informational: SmallintOrd and SmallintOrdOpe became producible in the restack, but no field here carries them, so v4_ordered_rows.json, v4_ordered_queries.json, the TestV4Ordered* tests and the PostgreSQL rankings/go_rankings columns never see an i16 order term; the Rust-side sorts_as in v4_order.rs is the only smallint ordering coverage. The OPE term is width-dependent (17 bytes at I16, 33 at I32), so a Go field sealed at the wrong width would write terms that never compare with Rust's and nothing would say so. By construction it is right (the guest sends an Int16 frame and Plaintext::from_frame refuses any other kind), so this is unpinned rather than wrong. One int16 field with encrypt_into=SmallintOrd (or the OrdOpe twin) plus a regeneration of both ordered fixtures would close it.
IntegerOrd, SmallintOrd, BigintOrd, RealOrd, DoubleOrd, TextOrd and their OrdOpe twins join ENCRYPTION_DOMAINS, so their v4 structs derive EncryptFrom and DecryptInto and the by-name dispatch runs them. OpeCllw implements EncryptFrom for any orderable plaintext: the hex of Stack Encrypt's OPE term over ADR-0002's order key, which EQL compares as bytea. The engine already answers the four single-bound orderings on a target through its query twin (and Eq on text, through TextEq's equality term), so Go fields typed with these get Lt, Lte, Gt and Gte from stashgen. Between is not offered: a range needs EQL v4's range query domains. Tests: the stored terms sort as the order keys do (integers, floats with -0.0 tied and NaN last, text by its fold, prefixes first), each query twin carries the stored terms, Ord and OrdOpe store one term, text with no fold is refused, and against an EQL v4 database <, <=, >, >=, = and ORDER BY eql_v4.ord_term find exactly the expected rows. A new v4_ordered_queries.json pins one query per type for the Go SDK. TextSearch stays unproducible with its own reason: its query carries a match term, which short text cannot make, so = and < on short text would be refused. The *OrdOre types still wait on block ORE.
…Go's rows in PostgreSQL A Ranking test model holds one field of each OPE-ordered type. Go seals it through generated code over the deterministic EQL guest: each value stores its catalog keys, the order terms sort as the rows do (with the -0.0 and folded-text ties), and everything opens back. Its Lt, Gte and Eq methods derive v4_ordered_queries.json byte for byte. TestV4OrderedRowsMatchTheCommittedFixture writes v4_ordered_rows.json, which the Rust suite opens field by field and inserts into eql_v4 columns, where < with the fixture's query operands finds the rows the plaintexts say. Profile and its frozen rows are untouched.
72dc64c to
6fc0c6a
Compare
548c546 to
17171df
Compare
Summary
Go developers can now store numbers and text in EQL columns they can sort and range-filter:
IntegerOrd,BigintOrd,RealOrd,DoubleOrd,TextOrdand theirOrdOpetwins. EQL is the SQL library we install into a customer's PostgreSQL database to store and search encrypted values. Stack Encrypt is our Rust encryption engine, which the Go SDK runs as a WebAssembly guest. These types store an OPE term (order-preserving encryption: an encrypted value whose bytes sort in the same order as the plaintexts), and EQL compares it as plain bytes. So<,<=,>,>=andORDER BY eql_v4.ord_term(col)work on them. #1235 and #1242 already made the engine derive these terms over ADR-0002's order keys. What was missing was the EQL side: a list in the code generator, and the step that writes the engine's OPE term into EQL'sopfield.Stacked on #1242 (which is stacked on #1235). Review those first.
Changes
eql-codegen and eql-bindings
ENCRYPTION_DOMAINS. Their v4 structs deriveEncryptFromandDecryptInto, and the by-name dispatch handles them. That takes the producible types from 12 of 51 to 22.OpeCllw(theopterm) implementsEncryptFromfor any orderable plaintext. It writes the hex of Stack Encrypt's OPE term.TextSearchstays unproducible with a new reason. Its query value has to carry a match term, and text shorter than one 3-gram can't make one. So=or<on short text would be refused. How a query that isn't a match leaves that term out still needs deciding (stashgen refuses TextMatch, TextSearch, TextOrd and TextOrdOpe — the engine derives their terms, but no EQL type is built from them #1131).v4/{integer,bigint,real,double,text,targets}.rsand Go'sencrypt/eql/eql_gen.go..changeset/eql-v4-ordered-ope-types.md(@cipherstash/eqlminor), is kept separate from the sharedeql-v4-bindings.mdso this PR and feat(eql-bindings): produce Smallint and SmallintEq from i16 #1267 don't conflict.Tests (
packages/eql/tests/encryption, newtests/v4_order.rs)The stored terms sort as the order keys do:
float8sorts them:-0.0ties0.0, and every NaN is one value after+Infinity;f32orders as thef64it widens to;Each query value carries the terms the stored value holds. A family's
OrdandOrdOpestore one term for one value. Text with no fold (an unassigned code point) is refused on both paths.PostgreSQL, against the v4 bundle built from the tree. On every new column,
<,<=,>,>=,=, the query on the left of the comparison,ORDER BY eql_v4.ord_term(col)andeql_v4.maxeach return exactly the expected rows:=reads the order term, so-0.0 = 0.0;=readsTextEq's equality term, soCaféandcafetie in order but aren't=.test:encryption:postgresnow also runsv4_order.New fixtures:
v4_ordered_queries.json: one query per type, derived in Rust and read by Go;v4_ordered_rows.json: rows sealed by Go, then opened and compared in Rust and PostgreSQL.v4_rows.json,stored_v4_rows_v1.jsonand theProfilemodel are untouched. A separateRankingmodel holds the ordered fields.Go
Lt,Lte,GtandGteon these fields, plusEqon the text ones. The guest already answered these through each type's query value, so the generator needed no change.Rankingtest model.eql_v4_order_test.gochecks:IntegerOrdOre, which is still unproducible.eql_v4.ord_term(col)with a plain btree),doc.go, the stashgen README and the CHANGELOG.Verification
packages/eql:mise run build,test:codegen,test:crates,test:encryption(432 passed),types:check(on the commit) andcodegen:parityall passed.mise run test:encryption:postgrespassed against a disposable PostgreSQL 17.5 (initdbcluster) with the v4 bundle built from the tree:text_eq1,v4_types3 andv4_order2 tests.cargo clippy -p eql-bindings -p eql-codegen -p eql-encryption-tests --all-features --all-targets -- -D warningsand eql-bindings rustdoc with-D warningswere clean, under--all-featuresand understack-encryptalone.cargo publish --dry-run --all-features -p eql-bindingscan't resolvestack-encrypt ^0.3.0from crates.io. That's the same as on the base, and CI reports it as a warning.mise run wasm:guest:testpassed (68 and 71).languages/golang:mise run go:testpassed,go vet ./...was clean andmise run go:lintreported 0 issues.go generate ./...leaves no diff.stored_rows_v1.jsonand the jobs that depend on it, by design.) Nothing here touches that fixture or its test.Related
Refs #1131: makes
TextOrdandTextOrdOpeproducible and checks step 1 for OPE (EQL's SQL compares the engine's OPE terms correctly).TextSearchis left with its reason.Refs #1130: the OPE-ordered number types.
Refs #1132: the
*OrdOretypes still wait on block ORE.Review notes
OpeCllw'sEncryptFromincrates/eql-bindings/src/encryption.rs, andencryption_gapincrates/eql-codegen/src/bindings.rs. The rest is generated or tests.Answersimpls only for single-index types, and its generator panics on an order index. Whichever of feat(eql-bindings): query EQL v4 types through Stack Encrypt's typed query chain #1244 and this PR merges second must extend that rule toope(Lt/Lte/Gt/Gte, andEqforhm+ope).Between. Once both are in, a follow-up builds the{v, i, lo, hi}range operand for these types and stashgen getsBetween. Until then the engine doesn't listbetweenfor a target, so nothing generates it.encryption_gap, an indexing note in the Go README, a fixtures README note that the two ordered fixtures aren't frozen and regenerate together, and a separate changeset.EQL_UPDATE_FIXTURES=1: Go'sTestV4OrderedRows…writes the rows and Rust'sv4_orderwrites the queries. Otherwise four cross-language tests fail at once.stored_rowsquestion. Once those bytes are final,stored_v4_rows_v2.json(or astored_v4_ordered_rows_v1.json) should freeze them.Eqmethods on the numberOrdtypes. SQL's=works on them through the order term, but the engine answersEqonly through an equality index.Smallintfamily.TextSearchand the*OrdOretypes.