Skip to content

[hotfix][integrations][python][docs] Align vector stores with their declared contracts - #1280

Merged
yunfengzhou-hub merged 4 commits into
apache:mainfrom
yunfengzhou-hub:fix/elasticsearch-k-num-candidates
Oct 20, 2026
Merged

yunfengzhou-hub merged 4 commits into
apache:mainfrom
yunfengzhou-hub:fix/elasticsearch-k-num-candidates

Conversation

@yunfengzhou-hub

@yunfengzhou-hub yunfengzhou-hub commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Linked issue: none, every commit here is a hotfix.

Purpose of change

Three vector store defects where the code does not do what its own declaration promises.

  • Elasticsearch. The constructor rejected num_candidates >= k, which Elasticsearch requires, and accepted the reverse. A store-level k never took effect, although its javadoc and the parameter table call it an overridable default. Exporting it exposed a third defect: the KNN search set no size, so hit counts came from k and Elasticsearch's default of 10, not from limit.
  • Mem0. pyproject.toml declares mem0ai>=0.1.43, but _query_embedding calls store.search(query=..., vectors=[...]) and VectorStoreBase.search only takes vectors from 0.1.74 on, so an install inside [0.1.43, 0.1.74) fails every vector query with TypeError.
  • Filter DSL. The documentation promised that any implementation receiving an unsupported operator raises NotImplementedError or UnsupportedOperationException. Chroma and Milvus reject only some shapes; the other four backends do not check.

Runtime flow

These two sub-headings cover the Elasticsearch change.

The constructor reads k and num_candidates and rejects one combination. At query time BaseVectorStore.query merges getStoreKwargs() with the query's arguments and passes them to queryEmbedding, which resolves k against limit and builds one KNN request. A store-level k therefore reaches a query only under the key queryEmbedding reads, and limit bounds hits only if the request carries a size.

Key decisions

The validation now rejects num_candidates < k, the combination Elasticsearch documents as invalid.

getStoreKwargs exposes the store-level k as k rather than top_k, which only S3VectorsVectorStore reads, from its own arguments.

The KNN request sets size to min(limit, k), so k picks the neighbours and limit bounds the result. The sibling getDocuments(index, filters, extraArgs) already sets a size.

Mem0 raises the floor rather than adding a compatibility branch, since only search differs across the boundary. The Filter DSL documentation is corrected rather than the four backends, since local validation would change which exception a caller catches across four modules in two languages.

Behavioral Semantics

Applies to the Elasticsearch change; the Mem0 floor is packaging metadata and the Filter DSL edit is prose.

Interaction decisions

store k store num_candidates query limit Result
unset any 3 no check; k falls back to limit, at most 3 hits
set, 8 unset or >= 8 3 k 8 picks the neighbours, at most 3 hits
set, 8 set, < 8 any constructor throws IllegalArgumentException
unset set, 5 8 constructs; Elasticsearch rejects the pair at search time
unset unset 12 at most 12 hits, not the default size of 10

Row 4 is the gap the constructor cannot close: with no store k, the effective value comes from limit at search time.

Behavioral contracts

  1. A descriptor with num_candidates >= k constructs successfully.
  2. A descriptor with num_candidates < k is rejected at construction.
  3. A store-level k reaches queryEmbedding when the query does not override it.
  4. A k in the query's extraArgs overrides the store-level one.
  5. Omitting either argument skips the check.
  6. getStoreKwargs no longer contains top_k.
  7. The documents returned never exceed the query's limit.
  8. A limit above 10 is not truncated to Elasticsearch's default size.

Failure behavior

Contract 2 raises IllegalArgumentException from the constructor, the same point in the lifecycle as before with the condition inverted, so an invalid combination fails when the resource is built rather than as an Elasticsearch 400 at query time. Nothing else raises, falls back or retries.

Compatibility impact

A configuration that previously failed at query time now fails at construction, and the reverse now works. A caller who set a store-level k and saw it ignored now gets it applied. A limit above 10 now returns up to limit documents instead of 10, so a caller relying on the implicit cap sees more results. getStoreKwargs is public, so reading top_k from the Elasticsearch store now yields nothing; no code here did.

An install that would resolve mem0ai inside [0.1.43, 0.1.74) now fails dependency resolution instead of failing at query time; installs on 0.1.74 or later are unaffected.

Tests

Contract Tests
1, num_candidates > k acceptsNumCandidatesGreaterThanK
1, num_candidates == k acceptsNumCandidatesEqualToK
2 rejectsNumCandidatesLessThanK
3 and 6 exposesStoreLevelKUnderTheKeyQueryReads
4 none
5 none
7 testStoreLevelKDoesNotExceedQueryLimit
8 testQueryLimitAboveElasticsearchDefaultSizeIsHonored

Contracts 1 to 3 and 6 ran against the unfixed source first: the two acceptance tests and the store-kwargs test failed, and the rejection test reported no throwable. Restoring top_k fails only the last.

The Elasticsearch module runs 12 tests and api 769, both with 0 failures. Spotless, Apache RAT and a Hugo 0.110.0 build pass.

Not verified:

  • Contracts 7 and 8 are in ElasticsearchVectorStoreTest, gated on ES_HOST. They compile and are registered but were not executed here; CI runs that class against the compose server.
  • Contracts 4 and 5 have no test; both live in queryEmbedding, which needs a live cluster. No query ran against a server, so the num_candidates < k claim rests on the documented requirement, not an observed 400, and no install was resolved against a mem0ai in [0.1.43, 0.1.74).
  • The Filter DSL wording was checked by reading all six backends, not by exercising them.
Supporting evidence
  • mem0ai boundary: the published wheels for 0.1.43, 0.1.50, 0.1.60, 0.1.66, 0.1.70 and 0.1.73 all declare VectorStoreBase.search(self, query, limit=5, filters=None); 0.1.74 and 1.0.11 declare search(self, query, vectors, limit=5, filters=None). The base class is the right layer to check because the integration resolves a configurable provider through VectorStoreFactory. Only search differs: the six other methods the integration calls, and VectorStoreFactory itself, are identical on both sides, so a compatibility branch would wrap one call to support versions where vector search cannot work.
  • Elasticsearch num_candidates: the vendor documentation states it must be greater than or equal to k.
  • Elasticsearch size: omitted from a search request it defaults to 10, and queryEmbedding returns the hits it iterates, so size bounds the result.
  • Filter DSL audit: Chroma raises NotImplementedError for a $-prefixed key or a dict value, so a list under a plain key passes. Milvus raises UnsupportedOperationException for a null or Map value and never inspects the key, so a list value becomes an equality clause against its stringified form. Elasticsearch and OpenSearch serialise the value into a term query without inspecting it. S3Vectors applies String.valueOf to it, so an operator form becomes a literal that silently matches nothing. Mem0 passes filters through to a provider the caller chooses, so its semantics are not ours to enforce.
  • Elasticsearch null values: filtersToBoolMustJson builds Map.of(field, value), which throws NullPointerException on a null value. Not addressed here, and independent of the operator-form contract.

API

No public API signatures change. getStoreKwargs keeps its signature and returns one different key for the Elasticsearch store.

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

Was this patch authored or co-authored using generative AI tooling?

  • Yes
  • No

Generated-by: Qoder 0.4.4 (Qwen 3.8-Max)

yunfengzhou-hub and others added 3 commits October 19, 2026 17:21
…didates

Elasticsearch requires num_candidates to be at least k for a KNN search,
but the constructor rejected exactly that combination and accepted
num_candidates below k, which the server then refuses at query time.

A store-level k also never took effect: getStoreKwargs exposed it as
top_k while queryEmbedding reads k from the merged arguments, so the
configured default was silently dropped in favour of the query limit.

AI-Contributed/Feature: 0/8
AI-Contributed/UT: 0/72
Mem0VectorStore calls store.search(query="", vectors=[embedding], ...),
but VectorStoreBase.search only takes a vectors argument from 0.1.74
onward. The declared floor of 0.1.43 admits versions where every vector
query raises TypeError, while insert, update, get, list, delete and
delete_col are unaffected because their signatures are identical on both
sides of that boundary.

Generated-by: Qoder 0.4.4 (Qwen 3.8-Max)

Co-Authored-By: Qoder <noreply@qoder.com>
AI-Contributed/Feature: 2/2
AI-Contributed/UT: 0/0
The Filter DSL section promised that any implementation receiving an
unsupported operator raises NotImplementedError or
UnsupportedOperationException. Only Chroma and Milvus check the filter
before building a request. Elasticsearch, OpenSearch, S3Vectors and Mem0
forward the value, so a caller writing the documented catch block never
reaches it, and S3Vectors stringifies the value rather than failing.

Describe what each backend actually does instead of a uniform exception.

Generated-by: Qoder 0.4.4 (Qwen 3.8-Max)

@sangkyoonnam sangkyoonnam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these fixes. Exporting the store-level k lets it override the query's limit in Java query(): a store with k=8 queried with limit=3 returns 8 documents. Details inline. The Mem0 floor checks out against the published wheels: 0.1.73 declares search(self, query, limit=5, filters=None) and 0.1.74 adds vectors. I also left a wording suggestion on the Filter DSL paragraph.

// Expose as top_k (preferred name); keep legacy key for compatibility if referenced
m.put("top_k", this.k);
// queryEmbedding() reads this under "k"; expose it under the same key
m.put("k", this.k);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queryEmbedding prefers args.k over limit, and the search request sets no size, so exporting the store default here lets it exceed the query's bound. Against Elasticsearch 8.19.0 from tools/docker/elasticsearch with 12 indexed documents, a store with k=8 queried through VectorStoreQuery with limit=3 returns 8 documents at 3921212 and 3 at bc62aa8, where the exported top_k entry was ignored. VectorStoreQuery documents limit as the maximum number of documents to return.

Could this PR keep that bound? The change is one line in queryEmbedding, outside the changed hunks:

             SearchRequest.Builder builder =
                     new SearchRequest.Builder()
                             .index(index)
+                            .size(Math.min(limit, k))
                             .knn(

That leaves k in charge of neighbor selection and bounds the returned hits. It also fixes an older truncation: Elasticsearch defaults size to 10, so a query with no store k and limit=12 returns 10 documents at both commits. With that line added locally, the two cases return 3 and 12, and the four new tests and ElasticsearchVectorStoreTest still pass. A test through query() for each case would cover them.

The constructor check doesn't validate the effective pair. With no store k, num_candidates=5 and a query limit=8 fail at search time with [num_candidates] cannot be less than [k] at both commits. That predates this PR and can be a follow-up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — queryEmbedding now sets .size(Math.min(limit, k)), so k picks neighbours and limit bounds the hits. Added both cases through query(): store k=8 with limit=3 expects 3, no store k with limit=12 expects 12. Not run locally (no Elasticsearch here); CI executes that class against the compose server. Effective-pair validation left as a follow-up.

```

`None` / `null` means "no filter". Richer operators (ranges, set membership, OR, NOT, etc.) are out of scope here. Callers needing backend-specific operators should pass them through `extra_args` (Python `VectorStoreQuery.extra_args` or `**kwargs`) or `extraArgs` (Java) — for example, ChromaDB's native `where` dict. Implementations that receive an unsupported operator via `filters` raise `NotImplementedError` (Python) or `UnsupportedOperationException` (Java).
`None` / `null` means "no filter". Richer operators (ranges, set membership, OR, NOT, etc.) are out of scope here. Callers needing backend-specific operators should pass them through `extra_args` (Python `VectorStoreQuery.extra_args` or `**kwargs`) or `extraArgs` (Java) — for example, ChromaDB's native `where` dict. An unsupported operator is not portable. Chroma and Milvus reject it locally with `NotImplementedError` (Python) or `UnsupportedOperationException` (Java); the other backends forward the value, where the service may reject the query or it may silently match nothing. Do not rely on catching a particular exception.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both checks cover only some shapes. Milvus rejects null and map values, so {"$or": [...]} passes and becomes an equality clause against the stringified list. Chroma's helper lets a list value under a plain key through to the ChromaDB client.

Suggested change
`None` / `null` means "no filter". Richer operators (ranges, set membership, OR, NOT, etc.) are out of scope here. Callers needing backend-specific operators should pass them through `extra_args` (Python `VectorStoreQuery.extra_args` or `**kwargs`) or `extraArgs` (Java) — for example, ChromaDB's native `where` dict. An unsupported operator is not portable. Chroma and Milvus reject it locally with `NotImplementedError` (Python) or `UnsupportedOperationException` (Java); the other backends forward the value, where the service may reject the query or it may silently match nothing. Do not rely on catching a particular exception.
`None` / `null` means "no filter". Richer operators (ranges, set membership, OR, NOT, etc.) are out of scope here. Callers needing backend-specific operators should pass them through `extra_args` (Python `VectorStoreQuery.extra_args` or `**kwargs`) or `extraArgs` (Java) — for example, ChromaDB's native `where` dict. An unsupported operator is not portable. Chroma and Milvus reject some operator forms locally with `NotImplementedError` (Python) or `UnsupportedOperationException` (Java); other forms, and the other backends, can fail later or silently match nothing. Do not rely on catching a particular exception.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied. Confirmed both gaps: Milvus checks only null and Map, Chroma only a $-prefixed key or a dict value.

@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue. and removed doc-included Your PR already contains the necessary documentation updates. labels Oct 19, 2026
…y limit

queryEmbedding built the KNN search without a size, so Elasticsearch
defaulted it to 10 while k decided how many hits came back. A limit above
10 was truncated, and exporting the store-level k let it widen the result
past the caller's limit. Set size to min(limit, k), so k selects the
neighbours and limit bounds what is returned.

Also narrow the Filter DSL note: Chroma and Milvus reject only some
operator shapes, so the previous wording promised a local exception that a
list value never triggers.

Generated-by: Qoder 0.4.4 (Qwen 3.8-Max)

Co-Authored-By: Qoder <noreply@qoder.com>
AI-Contributed/Feature: 4/4
AI-Contributed/UT: 88/88
@github-actions github-actions Bot added doc-included Your PR already contains the necessary documentation updates. and removed doc-included Your PR already contains the necessary documentation updates. labels Oct 20, 2026

@wenjin272 wenjin272 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yunfengzhou-hub for the fixes, and @sangkyoonnam for the careful review. LGTM

@yunfengzhou-hub
yunfengzhou-hub merged commit b670283 into apache:main Oct 20, 2026
56 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-included Your PR already contains the necessary documentation updates. fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants