Skip to content

[hotfix][vector-store][java] Update Elasticsearch filters documentation - #997

Open
kaiwangleo wants to merge 10 commits into
apache:mainfrom
kaiwangleo:hotfix/elasticsearch-filter-javadoc
Open

[hotfix][vector-store][java] Update Elasticsearch filters documentation#997
kaiwangleo wants to merge 10 commits into
apache:mainfrom
kaiwangleo:hotfix/elasticsearch-filter-javadoc

Conversation

@kaiwangleo

@kaiwangleo kaiwangleo commented Aug 11, 2026

Copy link
Copy Markdown

Linked issue: N/A (hotfix)

Purpose of change

Update stale Elasticsearch Vector Store Javadoc that incorrectly said the unified filters parameter was ignored.

The documentation now describes the implemented behavior for get, delete, and queryEmbedding:

  • equality-only metadata matching;
  • mapping keys to <metadataField>.<key>.keyword term queries;
  • AND semantics across multiple entries;
  • AND composition with a raw JSON filter_query;
  • direct-ID behavior, where filters are not applied;
  • delete-all behavior when no IDs or filters are supplied.

Tests

  • git diff --check
  • The changed ElasticsearchVectorStore.java passes Spotless formatting.
  • The module-level Spotless command continues to report only the unmodified ElasticsearchVectorStoreTest.java, whose checkout has CRLF line endings in the Windows-mounted workspace.

No runtime tests were added because this is a documentation-only correction of behavior already covered by testFiltersDsl.

API

No public API or runtime behavior changes.

Documentation

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

Document the implemented equality-only metadata filter translation for get, delete, and queryEmbedding, including metadata field mapping, AND semantics, raw filter_query composition, and ID-path behavior.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)
@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. labels Aug 11, 2026

@weiqingy weiqingy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for taking this on. A few questions inline, all about what the new text leaves unsaid rather than anything it gets wrong.

* 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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 ..keyword. I added the limitation and the raw ilter_query workaround to the get, delete, and queryEmbedding Javadocs in commit 41c78db. This documents the current non-string behavior without changing runtime semantics.

* 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Withdrawing the doc ask here. Rather than have you write down a limitation we intend to remove, I filed #999 for the pre-filter change and will take a first pass at the fix. Your post-filter sentence describes today's behavior accurately, so there is nothing to change in this PR for it.

The other two comments are unaffected.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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 41c78db.

* 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}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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 41c78db.

Leo Wang added 5 commits August 12, 2026 06:51
Document that non-string metadata filters do not match keyword sub-fields and clarify the MAX_RESULT_WINDOW result-window ceiling.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)
Retrigger checks after Code Style Check and macOS Python setup failed on a self-signed certificate before project code ran.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)
Retrigger CI after the Code Style Check failed during the runner certificate setup before check-license.sh executed.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)
Align the result-window and post-filter documentation with google-java-format and remove an accidental duplicate sentence.

Generated-by: OpenAI Codex Desktop 26.803.41515 (GPT-5.6 Sol)
@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 Aug 12, 2026
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.

2 participants