weiqingy commented on code in PR #997:
URL: https://github.com/apache/flink-agents/pull/997#discussion_r3763532940
##########
integrations/vector-stores/elasticsearch/src/main/java/org/apache/flink/agents/integrations/vectorstores/elasticsearch/ElasticsearchVectorStore.java:
##########
@@ -336,23 +336,25 @@ public Map<String, Object> getStoreKwargs() {
/**
* Retrieve documents from the vector store.
*
- * <p>If ids is not provided, this method will retrieve documents
according to {@code limit},
- * {@code offset}, and {@code filter_query} in additional arguments. If
{@code limit} is null,
- * up to {@link ElasticsearchVectorStore#MAX_RESULT_WINDOW} documents are
returned (an
- * Elasticsearch ceiling).
+ * <p>When {@code ids} is non-empty, documents are retrieved directly by
ID and the filter,
+ * limit, offset, and {@code filter_query} arguments are not applied.
*
- * <p>The unified {@code filters} DSL parameter is not yet translated to
Elasticsearch's native
- * query DSL — callers needing structured filtering should pass a raw
{@code filter_query} via
- * {@code extraArgs}. TODO: implement equality-DSL translation parallel to
the Python Chroma
- * implementation.
+ * <p>Otherwise, {@code filters} provides equality-only matching against
document metadata. Each
+ * entry is translated to an Elasticsearch {@code term} query on {@code
+ * <metadataField>.<key>.keyword}, and multiple entries are combined with
AND semantics. A raw
+ * Elasticsearch JSON query may also be supplied as {@code filter_query}
in {@code extraArgs};
+ * when both forms are present, they are combined with AND semantics.
*
- * @param ids The ids of the documents.
- * @param collection The name of the collection to be retrieved. If is
null, retrieve the
- * default collection.
- * @param filters Unified filter DSL. Currently ignored — see method
Javadoc.
- * @param limit Maximum number of documents to return; falls back to {@link
- * ElasticsearchVectorStore#MAX_RESULT_WINDOW} when null.
- * @param extraArgs Additional arguments. (offset, filter_query, etc.)
+ * <p>The {@code limit} parameter takes precedence over a {@code limit}
value in {@code
+ * extraArgs}. If neither is provided, up to {@link
ElasticsearchVectorStore#MAX_RESULT_WINDOW}
Review Comment:
nit: The text this replaces called `MAX_RESULT_WINDOW` an Elasticsearch
ceiling, and the constant's own Javadoc still describes it as "the maximum
number of documents that can be retrieved in get" (115). The new sentence reads
more like a default that applies when `limit` is absent. Nothing clamps `size`
(467), so someone who takes it that way and passes `limit = 50000` gets a
rejection from Elasticsearch rather than 10000 rows.
Is the ceiling sense worth keeping? Perhaps "…up to `MAX_RESULT_WINDOW`
documents are returned; `offset` plus `limit` cannot exceed it either", though
you may see a neater way to word it.
##########
integrations/vector-stores/elasticsearch/src/main/java/org/apache/flink/agents/integrations/vectorstores/elasticsearch/ElasticsearchVectorStore.java:
##########
@@ -336,23 +336,25 @@ public Map<String, Object> getStoreKwargs() {
/**
* Retrieve documents from the vector store.
*
- * <p>If ids is not provided, this method will retrieve documents
according to {@code limit},
- * {@code offset}, and {@code filter_query} in additional arguments. If
{@code limit} is null,
- * up to {@link ElasticsearchVectorStore#MAX_RESULT_WINDOW} documents are
returned (an
- * Elasticsearch ceiling).
+ * <p>When {@code ids} is non-empty, documents are retrieved directly by
ID and the filter,
+ * limit, offset, and {@code filter_query} arguments are not applied.
*
- * <p>The unified {@code filters} DSL parameter is not yet translated to
Elasticsearch's native
- * query DSL — callers needing structured filtering should pass a raw
{@code filter_query} via
- * {@code extraArgs}. TODO: implement equality-DSL translation parallel to
the Python Chroma
- * implementation.
+ * <p>Otherwise, {@code filters} provides equality-only matching against
document metadata. Each
+ * entry is translated to an Elasticsearch {@code term} query on {@code
+ * <metadataField>.<key>.keyword}, and multiple entries are combined with
AND semantics. A raw
Review Comment:
One detail from the helper's own Javadoc didn't make it up here:
Elasticsearch only creates `.keyword` sub-fields for strings (776). The
translation loop adds `.keyword` to every key whatever the value type
(787-789), and metadata is stored as a plain object with dynamic mapping on
(284, 294), so a number or boolean is mapped as `long`/`boolean` and never gets
a `.keyword` at all. A `term` query against a field that isn't in the mapping
doesn't error, it just matches nothing.
The practical effect is that `Map.of("year", 2024)` comes back empty, with
nothing to tell the caller the filter was never satisfiable. (Nothing alarming
on `delete`, though. The helper still returns non-null, so the `match_all`
branch at 551-552 stays untaken and a filter like that deletes nothing rather
than everything.)
Callers read this Javadoc rather than the private helper's, so would it be
worth pulling that string-only qualifier up into it? Something like this, if it
helps:
> Because ES dynamic mapping only creates `.keyword` sub-fields for string
values, filters on non-string metadata values will not match; use a raw
`filter_query` for those.
The same paragraph appears on `delete` (389-391) and `queryEmbedding`
(576-578), so it would be three copies of the clause.
##########
integrations/vector-stores/elasticsearch/src/main/java/org/apache/flink/agents/integrations/vectorstores/elasticsearch/ElasticsearchVectorStore.java:
##########
@@ -571,17 +573,22 @@ private void deleteDocuments(
* Executes a KNN vector search using a pre-computed embedding.
*
* <p>The method prepares a KNN search request using the supplied {@code
embedding} and merges
- * default arguments from the store with the provided {@code args}.
Optional filter queries
- * (JSON DSL) are applied as a post filter.
+ * default arguments from the store with the provided {@code args}. {@code
filters} provides
+ * equality-only matching against metadata fields. Each entry targets
{@code
+ * <metadataField>.<key>.keyword}; multiple entries are combined with AND
semantics and applied
+ * as a post-filter.
Review Comment:
Calling it a post-filter is accurate (623-625), and that word carries more
weight than it might appear. Elasticsearch applies `post_filter` after the KNN
phase has already chosen its `k` nearest hits, so it can only remove things
from that set. It can't pull in matching documents that fell outside the top
`k`.
So `queryEmbedding(embedding, 5, coll, Map.of("user_id", "alice"), args)`
can return nothing at all, even with hundreds of alice's documents indexed, if
the five nearest vectors happen to belong to other people. That's different
from `get` (476-481) and `delete` (542-549), where the same map becomes a real
query clause and is exhaustive. The three paragraphs now read almost
identically, which makes it easy to assume the behavior matches too.
Would a clause noting that fewer than `k` documents can come back be worth
adding? Whether the KNN clause's own `filter` option would suit this better
than `post_filter` is an implementation question rather than a docs one, so
I'll open a separate issue for that.
--
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]
