Uh oh!
There was an error while loading. Please reload this page.
feat(engine): Vector index and SearchVectors contract for storage backends - #243
Conversation
bf34f6e to
d184b1bCompared184b1b to
7857db7Compare
yesyayen
left a comment
There was a problem hiding this comment.
I believe capacity metering is not yes implemented. (VectorWriteRequestBytes / VectorSearchRequestBytes)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…xpression Review finding from yesyayen on #243: `vector_relevant_assignments` decided what to validate by matching expression FORM, and only matched a bare `SET emb = :v`. The doc comment admitted the rest ("complex right-hand sides (arithmetic, `if_not_exists`, nested paths) are left for the storage layer"), but the storage layer was not validating them either, so `SET emb = list_append(:a, :b)`, `SET emb = other_attr` and `SET emb = if_not_exists(emb, :v)` all skipped validation entirely and could store a malformed or wrong-dimension list under a vector attribute. The consequence is worse than a missing 400. At propagation delay 0 the user gets a 500 instead of a ValidationException; at delay > 0 the write returns 200 and the propagation worker silently drops the pending row, leaving the index permanently stale with nothing surfaced to anyone. The reviewer's proposed fix is the right one and is adopted here: validate the computed post-update image, so no current or future SET syntax can bypass the check. Vector validity is a property of the stored value, not of the expression that produced it. Two findings while implementing it, beyond what was reported: - The same hole exists on the TransactWriteItems update path via `assigned_vector_attributes`, which was not mentioned in the review. - `apply_update` has EIGHT call sites across the three backends (update_item and transactions in sqlite and postgres, four in mongodb), so adding the check at each would have been eight copies of a rule that must not be forgotten -- the duplication problem raised separately in #244's review. So the check is enforced in one place instead. `expression::apply_update` gains a sibling, `apply_update_validated`, which applies the actions and then validates the resulting image, and all eight backend call sites now use it. Forgetting the check is no longer possible for a future backend that applies an UpdateExpression, which is the property worth having. The engine-level form check in UpdateItem is removed rather than left in place, because keeping it would mean two competing models for the same property and it implied coverage it did not have. The comment there now says why validation deliberately does not live in the engine. The TransactWriteItems pre-flight is KEPT, and its doc comment now says what it is for: it produces the correct per-item CancellationReason vector for the bare-placeholder case, which the storage layer cannot reproduce because it aborts at the first failing operation. It is explicitly not the authoritative check, and the guarantee is documented as living in apply_update_validated. Placement follows existing convention rather than inventing one: sqlite and mongodb already validate secondary-index key types on the post-update image in these same functions, with comments noting that the post-update item is what actually gets written. Vector validation was the outlier. Tests: 9 new cases in the evaluator, covering each form that previously bypassed validation (bare placeholder, list_append, if_not_exists, attribute copy, append overflowing an already-full vector) plus the cases that must still be ALLOWED (list_append reaching the exact dimension, REMOVE of the vector attribute, an unrelated update, absent vector), and search-schema validation on the image. Negative control run: with the helper reverted to plain `apply_update`, exactly the 5 rejection tests fail and the 4 allow-tests still pass, so they discriminate on the fix rather than passing incidentally. One earlier draft test was discarded for failing this bar: `ADD emb :bad` errored inside apply_update regardless of validation, so it proved nothing, and it was replaced with the append-overflow case which apply_update accepts. Gates: fmt --check exit 0, clippy --all-targets -W clippy::pedantic exit 0 with 0 errors, 809 lib tests passed / 0 failed / 0 filtered out.
…city shape Review finding from yesyayen on #243: "capacity metering is not yet implemented (VectorWriteRequestBytes / VectorSearchRequestBytes)". Half right, and the half that is right was completely absent. VectorSearchRequestBytes IS implemented, with a model measured on 2026-08-05 including the service's two-mode non-determinism. VectorWriteRequestBytes had zero occurrences anywhere in the tree, and ConsumedCapacity had no VectorIndexes field at all, so no write operation could have reported it. This lands the model and the response shape. Wiring it into the five write operations follows, and needs VectorIndexKeyInfo to carry the projection. MEASURED, not derived. 20 probes against the real service in us-east-1 (account 964157134968) on 2026-08-13: VectorWriteRequestBytes = max(dimensions * 4 + projected_non_vector_bytes, 1024) doubled when the search-schema HASH value changes, because the entry moves partition and is charged as a delete plus an insert. Every one of the 20 probes matches this EXACTLY, not approximately. Three further predictions were computed before measuring and matched to the byte, which is stronger evidence than fitting. Unlike the search figure, the write figure is deterministic: four identical writes reported an identical value at both 1024 and 2048 dimensions. - 4 bytes per dimension exactly, from (8207-4111)/(2048-1024). That is one f32 per component: the index stores the raw vector, so the wire form is irrelevant, confirmed by writing the same vector with 1-character and 20-character numbers for an identical figure. - The vector attribute contributes only its NAME to the byte term. This is not a simplification; it is what makes the arithmetic come out exact. - 1024-byte floor, measured on an 8-dimension index whose unfloored model gives 47. The charging rule contradicts the public documentation, and the code follows the measurement. The docs say "writes that do not change an indexed attribute do not incur vector write capacity". The service actually charges whenever the PROJECTED entry changes: - setting the vector to a byte-identical value is NOT charged; - changing a NON-indexed attribute IS charged under ProjectionType ALL, because the projection includes it, and is NOT charged under KEYS_ONLY; - an item with no vector attribute is never in the index, so neither writing nor deleting it is charged; - a delete of an indexed item is charged on the deleted image. Both projection cases were measured, so this is not inference from one. Response shape, confirmed against the SDK service model and measured: the map is `ConsumedCapacity.VectorIndexes` keyed by index name, each entry a VectorCapacity carrying VectorSearchRequestBytes or VectorWriteRequestBytes. It is reported for INDEXES only, NOT for TOTAL (which returns TableName and CapacityUnits alone, without even the Table breakdown), and it is omitted entirely rather than zero-filled when nothing is charged. An earlier draft of this commit claimed TOTAL carried it; that claim was measured and removed. The 17 unit tests pin the measured figures themselves rather than our own arithmetic, so they are regression tests on real service behaviour. One expectation in them was wrong on first write (6111 where the service says 6115); it was corrected and then re-verified against the live service rather than against the model, so the test and the service agree independently. Full evidence table, including the two divergences from the docs, is in /home/lhnng/.meshclaw/workspace/vector-write-capacity-model.md. Gates: fmt --check 0, clippy --all-targets -W clippy::pedantic 0 errors, 826 lib tests passed / 0 failed / 0 filtered out.
Review finding from yesyayen on #243: validations missing on the vector index update path, listing four things that were possible: adding a vector index to a PROVISIONED table, adding a sixth index, using the table partition key as the vector attribute, and declaring SearchSchema attributes with no definition. All four confirmed, and the finding understates it: none of the four were enforced on the CREATE path either, so the gap was both paths rather than just the update one. Rather than guess the wording, all four were probed against the live service in us-east-1 (account 964157134968) on 2026-08-13, and the constants here are byte-identical to what it returned. Two of the rules turned out to differ BETWEEN the paths in ways that a shared implementation would have got wrong: 1. The count limit changes ERROR CLASS as well as text. CreateTable with six indexes returns ValidationException "...VectorIndex count exceeds the per-table limit of 5". Adding a sixth via UpdateTable returns LimitExceededException "Subscriber limit exceeded: Number of vector secondary indexes exceeds per-table limit of 5". Five are accepted on both. Hence two constants, and a new LimitExceededException variant, which the error enum did not have. 2. Using the partition key as the vector attribute produces DIFFERENT messages per path, because the underlying rule is "the vector attribute must not appear in AttributeDefinitions". On CreateTable the key must be declared, so that rule fires and reports the conflicting definition. On UpdateTable, where the key is not re-declared, the service instead reports "Attributes cannot be redefined ... Existing schema: Schema:[SchemaElement: key{pk:S:HASH}] New schema: VectorIndexSchema:[VectorAttribute: key{pk:L:8}]", embedding both schemas and reporting the vector as L with its dimension count. Both messages are recorded as constructors. What lands here is the request-only half, which is everything decidable without reading the table: billing mode and count on CreateTable, and on both paths the two attribute-definition rules (the vector attribute must NOT be declared, every SearchSchema element MUST be). On UpdateTable the search-schema definition must be in THAT request even when the attribute is already on the table, which was measured and is what the test asserts. The split follows the convention this function already documented for itself: request-only rules in core, state-dependent rules in the layer that reads the catalog, because reporting the wrong error class is worse than deferring. The state-dependent remainder is the count limit on UpdateTable (needs the existing index count), the key-collision message (needs the existing key schema), and rejecting a switch to PROVISIONED (needs to know the table holds vector indexes). Those follow in the backends. One probe in this set was CONFOUNDED and was re-run: the first UpdateTable attempt at the key-collision rule omitted `tenant` from the request's AttributeDefinitions, so it failed on the search-schema rule instead and proved nothing about the rule under test. The message above comes from the re-run with the confound removed. The rendered constants were checked against the captured service strings programmatically, not by eye, because Rust string continuations elide leading whitespace and a mismatch there would be invisible in review. Full evidence in /home/lhnng/.meshclaw/workspace/vector-validation-rules.md. Gates: fmt --check 0, clippy --all-targets -W clippy::pedantic 0 errors, 833 lib tests passed / 0 failed / 0 filtered out.
43c2577 to
f0b0298Compare…update-path rules Brings the three review-response commits from #243 onto this branch. The contract branch was rewritten before this merge so create-path validation stays HERE (this branch's versions win: its billing-mode check treats an absent BillingMode as PROVISIONED, which #243's dropped version got wrong), and the merge resolution reflects that: - crates/core/src/validation/mod.rs auto-merged after the rewrite; the one manual dedupe is the vector index count limit, which had two homes (this branch's private const, #243's pub const in types). One remains. - crates/engine/src/update_item.rs and transact_write_items.rs take #243's side: `vector_relevant_assignments` is deleted in favour of `apply_update_validated` on the evaluated image. This branch had evolved that function to cover `if_not_exists`, but image validation subsumes it: the evaluated image catches every expression form including those added later, which is the property the expression-matcher could never have. The transact pre-flight stays, documented as the CancellationReason-shape helper it is, not the authoritative check. - Two test reconciliations: this branch's probes captured the newer wrong-type message ("Expected: 32-bit floating point number list"), so the imported image-validation test now asserts that text rather than the older "a list of numbers"; and the two same-named `vi_spec` test helpers (different signatures from each side) are disambiguated. Gates on the merged tree (CI flags): fmt --check 0, clippy --all-targets -D warnings 0, 893 lib tests / 0 failed / 0 filtered.
LeeroyHannigan
commented
Aug 14, 2026
Re the review note on capacity metering: |
…kends Adds the vector index and SearchVectors contract across the engine and all storage backends: index declaration on CreateTable/UpdateTable, the SearchVectors operation, write-time validation of vector attributes on the evaluated item image, the measured VectorWriteRequestBytes capacity model, and the SearchCondition expression surface. Linearised onto main. The branch previously carried four merge commits from main; because the repository's merge queue uses the REBASE method, it replays a branch's individual commits, and the earliest vector commit predates main's sort-key-definition validation (#259). Replaying it re-fought a conflict those merges had already resolved, so the queue could not take the branch. Collapsing the work to one commit on top of main leaves the queue nothing to re-resolve. The tree is byte-identical to the reviewed branch merged with main, so no content changed in the linearisation. Original commits: 7857db7 feat(vector): vector index and SearchVectors contract for storage backends 4ecae5a fix(vector): validate vector writes on the evaluated image, not the expression 9f30727 feat(vector): measured VectorWriteRequestBytes model and ConsumedCapacity shape f0b0298 fix(vector): UpdateTable attribute-definition rules + update-path error scaffolding 2e5517a fix(vector): enforce table-level create rules; close review scaffolding gaps ec8ca29 fix(vector): write-time SearchSchema type mismatch matches the service; validate only changed values on update 60603af fix(vector): clippy cloned_ref_to_slice_refs in the changed-only tests
5b6204b to
71b615fCompareThere was a problem hiding this comment.
Approving, but Fable did highlight a gap in the search handler that seems reasonable to fix, at lease as a fast follow. Left that comment in line
A few smaller things, none blocking:
validate_vector_index_attribute_definitionssays it applies on both paths, but onlyvalidate_vector_index_updatescalls it. On the CreateTable path the search schema rule is covered indirectly through the key correspondence check, which produces different error text, and the conflicting vector attribute rule does not run at all there. Either the doc or the call sites should change.- Nothing validates the shape of the
SearchSchemaelement list at creation time. There is no element count cap and nothing rejects a second HASH element, while the search handler assumes at most one (it takes the first with.find(), and a condition on any second HASH element would quietly land infilters). Worth tightening, or documenting as deliberate, before a backend lands. - The module doc on
vector_opt_out_testsinstorage/src/lib.rsstill describes the earlier boolean design, talking about a default capability of false and a defaultedsearch_vectorsthat must fail. The tests themselves exercise the accessor design, so just the prose is stale.
| ))); | ||
| } | ||
| if !conditions.is_empty() { |
There was a problem hiding this comment.
This guard means the required HASH check never runs when the caller supplies no SearchConditionExpression at all. An index that declares a HASH element, searched with no expression, passes through here and reaches the backend with hash_key: None.
That breaks two promises this PR makes elsewhere. The comment further down says the index having a HASH element guarantees the search supplied it, and the VectorSearch::hash_key doc in storage/src/lib.rs tells backend authors it is always populated when the index declares one, so they may treat Some as a mandatory predicate. A backend written against that doc, including the stacked SQLite one, would serve an unscoped search where a scoped one was promised.
I think the fix is just to drop the guard and call validate_conditions_against_search_schema unconditionally. With empty conditions the membership loop does nothing and the HASH presence loop correctly rejects, and the case with no schema and no expression still passes. Unreachable in tree today since no backend implements search, but this file is the contract the next PR builds on, so better fixed now.
Uh oh!
There was an error while loading. Please reload this page.
Implements vector indexes and SearchVectors on the SQLite backend, making it the first backend to declare the optional VectorSearchEngine capability added by the contract in #243. Working end to end: CreateTable with VectorIndexes; UpdateTable create and delete with a real backfill of items already in the table; DescribeTable reporting index status; exact-scan SearchVectors for COSINE, EUCLIDEAN and DOT_PRODUCT with per-metric ordering, partition scoping, TopK, inline equality filters and ProjectionExpression; index maintenance at all six write sites including the three TransactWriteItems branches; crash recovery that drops and rebuilds an index left CREATING; and table drop taking its vector data tables with it. Storage layout is one row per vector, chosen on write amplification rather than read speed. A packed per-partition blob reads 2 to 4 times faster but would rewrite the whole blob to insert one vector, roughly 390 MB per PutItem for a 100k-vector partition at 1024 dimensions. Recorded in docs/adr/0004-vector-search-exact-scan.md. The backfill status sequence follows what the service was measured to report rather than what seemed reasonable: CREATING with Backfilling false, then CREATING with true, then ACTIVE with the member absent. So presence does not imply backfilling and a client must read the value rather than test for the member. Also carries the measured missing-HASH refusal messages and their check precedence: the service distinguishes two distinct messages and validates HASH completeness before out-of-schema attributes. Probed live against us-east-1 rather than inferred. Breaking change, on-disk format: the SQLite catalog version moves to the version carrying the vector_indexes table, so an existing deployment must run `extenddb migrate` before the server will start. Linearised onto main as a single commit. The merge queue is configured to REBASE, which discards merge commits and replays the branch's original commits one at a time onto current main. Commit 9d97d04 predates #286 and re-fought a conflict in crates/engine/src/search_vectors.rs that the branch's merge of main had already resolved, so the queue refused the PR while the PR page reported no conflict. One commit has nothing to replay. Tree is byte-identical to the reviewed branch merged with main. Original commits, preserved here for reference: 8c53454 fix(vector): match the service's missing-HASH messages and check precedence 14b4fdf style: rustfmt after conflict resolution dc1d8d3 Merge remote-tracking branch 'origin/main' into vs244-fix 9d97d04 feat(sqlite): vector search index Verified before push: cargo fmt --all --check exit 0; cargo clippy --all-targets -D warnings exit 0 on both the default and sqlite feature sets; cargo test --workspace 1015 passed, 0 failed, 0 filtered out.
Implements vector indexes and SearchVectors on the SQLite backend, making it the first backend to declare the optional VectorSearchEngine capability added by the contract in ExtendDB#243. Working end to end: CreateTable with VectorIndexes; UpdateTable create and delete with a real backfill of items already in the table; DescribeTable reporting index status; exact-scan SearchVectors for COSINE, EUCLIDEAN and DOT_PRODUCT with per-metric ordering, partition scoping, TopK, inline equality filters and ProjectionExpression; index maintenance at all six write sites including the three TransactWriteItems branches; crash recovery that drops and rebuilds an index left CREATING; and table drop taking its vector data tables with it. Storage layout is one row per vector, chosen on write amplification rather than read speed. A packed per-partition blob reads 2 to 4 times faster but would rewrite the whole blob to insert one vector, roughly 390 MB per PutItem for a 100k-vector partition at 1024 dimensions. Recorded in docs/adr/0004-vector-search-exact-scan.md. The backfill status sequence follows what the service was measured to report rather than what seemed reasonable: CREATING with Backfilling false, then CREATING with true, then ACTIVE with the member absent. So presence does not imply backfilling and a client must read the value rather than test for the member. Also carries the measured missing-HASH refusal messages and their check precedence: the service distinguishes two distinct messages and validates HASH completeness before out-of-schema attributes. Probed live against us-east-1 rather than inferred. Breaking change, on-disk format: the SQLite catalog version moves to the version carrying the vector_indexes table, so an existing deployment must run `extenddb migrate` before the server will start. Linearised onto main as a single commit. The merge queue is configured to REBASE, which discards merge commits and replays the branch's original commits one at a time onto current main. Commit 9d97d04 predates ExtendDB#286 and re-fought a conflict in crates/engine/src/search_vectors.rs that the branch's merge of main had already resolved, so the queue refused the PR while the PR page reported no conflict. One commit has nothing to replay. Tree is byte-identical to the reviewed branch merged with main. Original commits, preserved here for reference: 8c53454 fix(vector): match the service's missing-HASH messages and check precedence 14b4fdf style: rustfmt after conflict resolution dc1d8d3 Merge remote-tracking branch 'origin/main' into vs244-fix 9d97d04 feat(sqlite): vector search index Verified before push: cargo fmt --all --check exit 0; cargo clippy --all-targets -D warnings exit 0 on both the default and sqlite feature sets; cargo test --workspace 1015 passed, 0 failed, 0 filtered out.
What this does
Models DynamoDB's vector index surface in
extenddb-coreand makes it implementable by a storage backend, without implementing it for any. Both in-tree backends still refuse every vector operation, and those refusals are proven over the wire rather than by calling the validator directly.CreateTableandUpdateTableacceptVectorIndexes/VectorIndexUpdates,DescribeTablereports them,SearchVectorsis a first-class operation with its own consumed-capacity shape, and item writes validate the vector attribute and the search-schema attributes against the index definition.Design docs:
docs/adr/0004-vector-search-exact-scan.md(added on the stacked implementation branch), RFC #236.Vector search is an optional capability
This is the first optional feature on a trait surface where every other method is mandatory, so the shape is worth a look:
Declaring support therefore requires handing over an implementation. An earlier revision used a
supports_vector_indexes() -> bool, which let a backend returntrueand never implement the method: an honour system where the type system should be doing the work.The accessor deliberately sits on
DataEngine, notStorageEngine.StorageEnginecomes from a blanket impl over the six focused traits, so a defaulted method there could never be overridden by a backend. It would have looked correct and been unoverridable.StorageError::Unsupportedis added so a backend declining a feature is not reported as an internal fault. If we ever want a backend that declines transactions or streams, this is the template: peel the feature into its own trait with no defaults, add one accessor to a trait backends already implement, gate in core so the refusal never reaches storage.Measured against the live service
Most specifics were measured against real DynamoDB rather than inferred, and in nearly every case the measurement contradicted the inference:
DistanceFunctionorder[DOT_PRODUCT, COSINE, EUCLIDEAN], neitherSearchSchemaHASH elementsSearchConditionExpressionf32;f32::MAXexactly is acceptedNoutput formTableThroughputModeCreateTablememberThat last one mattered: an alias for it made ExtendDB accept a request AWS ignores.
Error messages are pinned by whole-string equality, not by fragment. Fragment assertions that began after the
One or more parameter values were invalidprefix hid three divergences: a colon where the service uses a full stop (twice), and one message missing the prefix entirely.Deliberately absent
Please read these as scope, not omissions:
UpdateTablecreate/delete paths are modelled but unvalidated, because validating them needs a backend that actually performs a backfill. This is the portion most likely to move, and reviewer time is probably better spent elsewhere.vectorIndexes.1.member.distanceFunction. A serde deserializer for the enum cannot know its position, so closing this means deserialising permissively and validating positionally, as theProjectioncheck already does. Its own change.Verification
Refusals are asserted over the wire against a live server, using a SigV4 raw-request helper because no SDK version carries the vector types. That suite immediately earned its place: it found that
UpdateTable's at-least-one check omittedVectorIndexUpdates, so a request carrying only vector index changes was rejected as empty, contradicting what the live service accepts. The unit tests call the validator directly and never reach that check, so it was invisible to them.Both vector suites self-skip on the wrong kind of backend, and the expectation is pinned per CI job (
EXTENDDB_EXPECT_VECTORS), so a backend that silently lost or gained the capability fails rather than skipping green.cargo fmt --checkclean,clippy --all-targets -D warningsclean on both feature setsWhat I would most like reviewed
DataEngineor somewhere better.