Repository navigation
feat(stack-encrypt): ordering operators Lt, Lte, Gt, Gte and Between, and remove query_index - #1208
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 configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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. |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 134 | 0 | 45 | 0 |
Every mutant in the changed lines was caught by a test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ef7b3c35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
freshtonic
left a comment
There was a problem hiding this comment.
Review of this PR's diff against #1206.
Ordering on folded text. My earlier ADR-0009 concern was that a strict < on folded or collated text can drop real matches, and verify cannot bring back rows the server did not return. This PR does not implement that rule. op::Ordered covers only the integer types, and an ordering on a text plan does not compile (tests/ui/op_order_on_text.rs). On integers, CLLW ORE and OPE order is exact. So Lt, Lte, Gt, Gte and an inclusive Between give no false negatives and no false positives, and no verify is needed.
Other checks. Between derives both bounds under one label and returns one term. InvertedRange is refused before any keyset load, also in an all batch, and (v, v) is accepted. The signed i64 test covers negative values. Mutants: 2 caught, 0 missed. Codex already raised the loss of a query path for Match<O> without MatchCase and for ORE/OPE over text; I do not repeat it.
No blocking defects from me.
Suggested follow-up PR
Orderedis public and not sealed. See the inline comment atsrc/op/mod.rs:256.- ADR-0009 line 265 still lists "ORE and OPE on every kind, text included (
Lt,Between)" as exact, and line 839 says the same forAtPath<Lt>on a text leaf. This PR excludes text. Reword both so the text-order decision stays open for the catalog (my earlier ADR point, still open). - CI: the Test EQL PG17 shards fail on SteVec selector drift, the same as the base. This PR does not cause it.
b05caf6 to
9dbb6b5
Compare
69ef7b3 to
9895211
Compare
|
Rebased with #1190 onto the current #1177. ADR-0009 was merged by hand in three places: decision 2 now sketches |
|
Correction to the note above: decision 7 keeps this PR's |
9895211 to
909eb50
Compare
9dbb6b5 to
a9f42c1
Compare
909eb50 to
20513db
Compare
This comment has been minimized.
This comment has been minimized.
a9f42c1 to
c8a3934
Compare
20513db to
63331c7
Compare
Generated by Claude Code |
… no query_index ADR-0009 decisions 1 and 7. `op::Lt`, `Lte`, `Gt` and `Gte` take one value and `op::Between` a pair of bounds, inclusive at both ends. The ORE and OPE indexes answer them over the integer types (`op::Ordered`), ORE by default and OPE through `.via(Ope)`; an ordering on text does not compile, since text has no order term this crate produces. A comparison's query term is the index's own term for the value: this crate's ORE is CLLW, whose ciphertext has no left half to send alone, so what is sent is what is stored, as for an HMAC, and `Supports` is where the left-only term of block ORE (#1140) goes when it lands. A range's query term is one `sem::RangeQuery` carrying both bounds' terms, derived under the one label, read with `lo()` and `hi()`. A range whose low bound is above its high bound is refused when the query is awaited, before either term is derived and before any keyset is loaded (`TermError::InvertedRange`). An operator now says what a query takes through `op::Arity`: `One` plaintext or a `Pair` of them. `op::Input<A>` relates the value a query is given to the plaintext the plan is checked against, and derives one term per plaintext. `Supports`, `Answers` and `DefaultIndex` are keyed by that input, so `Supports<Eq, S>` reads as before and `Between` is `Supports<Between, (S, S)>`; `Supports::query` takes the input's terms. `StackCipher::query_index` goes, with `QueryIndexBuilder` and `QueryIndex`: every index a plan can declare is reachable through an operator now, and the chain holds its own input, index and label. `Resolves::resolve` takes the index and the call's context and returns the index and the label. ADR-0009's decision 2 shows the shape that ships, and its mentions of `query_index` are past tense. Closes #1186 Claude-Session: https://claude.ai/code/session_01VYKAWbd361xDLmqBdLoUcB
Ordered was public and not sealed, so a downstream crate could implement it for its own type, such as a newtype over folded text that also implements the CLLW traits. Between refuses an inverted range with the type's PartialOrd while the server compares ORE terms, so such a type could refuse a valid range or accept an empty one, and it would open ordering on text, which ADR-0009 leaves to the catalog. Ordered now requires op::sealed::Sealed, which the macro implements for the integer types. A compile_fail doctest pins it with a type that meets every other bound, so the seal is the only reason it fails; without the seal the same snippet compiles. A trybuild test would pin rustc's list of sample implementors, which changes whenever a later layer seals another type.
- The CHANGELOG's `query_index` migration note no longer says `query(op::Matches, text)` is the same call. It returns a `MatchQuery`, refuses text that cuts no token, and compiles only for a configuration with `op::MatchCase`. `query_index(Ore, text)` and `query_index(Ope, text)` have no replacement until the order of text is settled, and the note says so. - `op::MatchCase`'s doc no longer says a configuration without it can be queried by index: that path is gone, and the doc names the one-line impl that makes such a configuration queryable. - `op::Ordered` has a curated diagnostic: an ordering over text, or over any type outside the integers, now says the orderings are integer-only. - `query` carries the `Input<Op::Arity>` bound and `op::Input` has a curated diagnostic, so `query(op::Between, &40u32)` fails at the value and says to pass `&(lo, hi)`, not only with an E0599 on `on`.
c8a3934 to
c816046
Compare
63331c7 to
8daa5ae
Compare
|
Thanks @tobyhede. All four are addressed in 8daa5ae:
|
Summary
Stacked on #1206. This PR implements ADR-0009 decisions 1 and 7 for the orderings: the operators
Lt,Lte,Gt,GteandBetween,Betweenas one term carrying both bounds, and the removal ofquery_index, since every index a plan can declare is now reachable through an operator.op, likeEq. The ORE and OPE indexes answer them over the integer types, through a new markerop::Ordered; ORE is the default and OPE is named with.via(Ope). An ordering on text does not compile: text has no order term this crate produces, and the ADR leaves that to the catalog.Supportsis where block ORE's left-only term goes when stack-encrypt: deletedynamic::ValueandScalar, derive terms through vitaminc, add block ORE, dropBoolorder terms #1140 lands, and the ADR says so now.Betweenis one term.sem::RangeQuery<T>carries both bounds' terms, derived under the one label and settled together, read withlo(),hi()andinto_bounds(). A range whose low bound is above its high bound is refused when the query is awaited, before either term is derived and before any keyset is loaded, with the newTermError::InvertedRange;(v, v)is one value.Operatorgainstype Arity,OneorPair.op::Input<A>relates the value a query is given to the plaintext the plan is checked against, and derives one term per plaintext.Supports,AnswersandDefaultIndexare keyed by that input, soSupports<Eq, S>reads as before andBetweenisSupports<Between, (S, S)>.Supports::querytakes the input's terms. A query given the wrong shape (op::Betweenwith one value,op::Ltwith a pair) does not compile.query_indexis gone, withQueryIndexBuilderandQueryIndex.query_index(Ore, &v)isquery(op::Lt, &v)(any ordering gives the same term) andquery_index(Match, text)isquery(op::Matches, text). The chain holds its own input, index and label;Resolves::resolvetakes the index and the call's context and returns the index and the label.ADR-0009's decision 2 now shows the shape that ships, decision 7 names
RangeQueryandInvertedRange, and thequery_indexsentences are past tense.Tests
tests/op.rs: every ordering sends the stored ORE term and orders as the values; OPE throughvia; a typed target answers through its index;Betweenequals theGteandLteterms of its bounds, brackets a stored value, and(v, v)is one value; an inverted range is refused throughonand in anallbatch with no keyset loaded; a fields plan answers the orderings under the field's label, including an index-onlyi64field with negative values, refuses the defaultOrewhere onlyOpeis declared, and refuses a value of the wrong type.op: the range check accepts(1, 2)and(2, 2)and refuses(3, 2).Betweengiven one value orLtgiven a pair.tests/plan.rsandtests/plan_grammar.rsmigrated fromquery_indexto the operators, andcodes.rscarries the new error.Test plan
cargo nextest run -p stack-encrypt --all-features: 583 passed.-D warningswith and without--all-features, rustdoc with warnings as errors,cargo fmt --check.cargo check --workspace --all-features --all-targets, clippy on the Go guest witheql,deterministic-kms, andcargo check -p eql-bindings --features stack-encrypt: nothing outside the crate usedquery_index.#1140 is listed as blocking this issue. What it blocks is the left-ciphertext query term, not the operators: the operators ship here over CLLW, and block ORE changes
Supports<Lt, S>::Queryfor its own index when it arrives.Closes #1186
https://claude.ai/code/session_01VYKAWbd361xDLmqBdLoUcB