kaiwangleo commented on code in PR #997:
URL: https://github.com/apache/flink-agents/pull/997#discussion_r3764207664
##########
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:
Thanks for checking the helper implementation in detail. You are right:
Elasticsearch dynamic mapping creates .keyword sub-fields only for string
metadata values, while the equality DSL always targets
<metadataField>.<key>.keyword. I added the limitation and the raw ilter_query
workaround to the get, delete, and queryEmbedding Javadocs in commit 41c78db8.
This documents the current non-string behavior without changing runtime
semantics.
##########
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:
Thanks for the clarification and for opening issue #999. I agree the current
post-filter wording accurately describes today's behavior, so I left that part
unchanged in this PR. The two remaining documentation clarifications are
included in commit 41c78db8.
##########
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:
Good point. I restored the ceiling wording and clarified that
MAX_RESULT_WINDOW is the Elasticsearch result-window limit, not an automatic
clamp. The Javadoc now states that the combined offset and limit must not
exceed the window and that an explicit limit above it is rejected by
Elasticsearch rather than truncated. Updated in commit 41c78db8.
--
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]
