sangkyoonnam commented on code in PR #1280:
URL: https://github.com/apache/flink-agents/pull/1280#discussion_r4313680922
##########
integrations/vector-stores/elasticsearch/src/main/java/org/apache/flink/agents/integrations/vectorstores/elasticsearch/ElasticsearchVectorStore.java:
##########
@@ -318,8 +318,8 @@ public Map<String, Object> getStoreKwargs() {
m.put("vector_field", this.vectorField);
m.put("dims", this.dims);
if (this.k != null) {
- // 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);
Review Comment:
`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 39212123 and 3 at bc62aa89, 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`](https://github.com/apache/flink-agents/blob/3921212369d9c33e75366a98aed45215ab061f20/integrations/vector-stores/elasticsearch/src/main/java/org/apache/flink/agents/integrations/vectorstores/elasticsearch/ElasticsearchVectorStore.java#L626-L629),
outside the changed hunks:
```diff
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.
##########
docs/content/docs/development/vector-search/vector_stores/_index.md:
##########
@@ -328,7 +328,7 @@ vectorStore.delete(null, "my_collection", null, Map.of());
{"user_id": "u1", "run_id": "r1"}
```
-`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.
Review Comment:
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.
```suggestion
`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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
