kaiwangleo commented on issue #999: URL: https://github.com/apache/flink-agents/issues/999#issuecomment-5263760287
I reviewed the current implementation and the proposed fix against the Elasticsearch Java client API.\n\nThe issue is reproducible from the code path:\n\n- ilters and rgs.filter_query are combined by combineQueryJson.\n- get and delete attach that JSON as the main query.\n- queryEmbedding currently attaches the same JSON through SearchRequest.Builder.postFilter(...), after the KNN phase has selected k hits.\n- The Elasticsearch client version is 8.19.0, whose KNN request builder exposes a filter on the KNN clause.\n\nProposed implementation:\n\n1. Keep the existing iltersToBoolMustJson and combineQueryJson behavior unchanged, so unified filters and raw ilter_query retain their current AND semantics.\n2. When combined != null, attach the parsed JSON to the KNN clause via KnnSearch.Builder.filter(...) instead of SearchRequest.Builder.postFilter(...).\n3. Remove the separate postFilter assignment; leave unfiltered KNN requests unchanged.\n4. Preserve the existing k and um_candidates handling and exception behavior.\n\nCompatibility impact:\n\n- This is an intentional behavior fix for filtered queryEmbedding calls.\n- Unfiltered calls are unchanged.\n- Filtered calls will return up to k matching documents, instead of an arbitrary subset of the unfiltered top k; callers may therefore receive more results than before, up to the requested k.\n- The raw JSON filter remains caller-supplied, so unsupported or invalid Elasticsearch DSL continues to fail through the client.\n\nTesting plan:\n\n- Enable or add a focused test with multiple documents whose nearest vectors belong to a different metadata group, then assert that a filtered KNN query returns matching documents up to k.\n- Cover the unified ilters map, raw ilter_query, and both forms combined.\n- Keep the existing no-filter KNN path covered.\n- The current Elasticsearch test class is disabled because it requires a running server; I would either add a focused request-construction unit test with a mocked client or extend the integration setup so the regression runs in CI rather than remaining disabled.\n\nThis is a user-visible behavior change, so I would appreciate committer confirmation that this approach and compatibility expectation are acceptable before I open the PR. -- 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]
