Semantic Query #108483

Mikep86 · 2024-05-09T19:14:37Z

Add the semantic query to the Query DSL, which is used to query semantic_text fields:

GET <index>/_search
{
    "query": {
        "semantic": {
            "field": "inference_field",
            "query": "my query text"
        }
    }
}

Add the semantic query to the Query DSL, which is used to query semantic_text fields --------- Co-authored-by: carlosdelest <carlos.delgado@elastic.co> Co-authored-by: Jim Ferenczi <jim.ferenczi@elastic.co>

Mikep86 · 2024-05-13T13:53:06Z

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

+                // No inference results have been indexed yet
+                childQueryBuilder = new MatchNoneQueryBuilder();
+            } else if (inferenceResults instanceof TextExpansionResults textExpansionResults) {
+                // TODO: Use WeightedTokensQueryBuilder


We can use WeightedTokensQueryBuilder here once #108254 is merged

elasticsearchmachine · 2024-05-13T14:50:16Z

Pinging @elastic/es-search (Team:Search)

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java

...inference/src/yamlRestTest/resources/rest-api-spec/test/inference/40_semantic_text_query.yml

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java

benwtrent · 2024-05-13T19:43:43Z

test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java

@@ -449,6 +449,7 @@ private static class ServiceHolder implements Closeable {
            List<NamedWriteableRegistry.Entry> entries = new ArrayList<>();
            entries.addAll(IndicesModule.getNamedWriteables());
            entries.addAll(searchModule.getNamedWriteables());
+            pluginsService.forEach(plugin -> entries.addAll(plugin.getNamedWriteables()));


Are there named writeables that aren't queries or anything? I don't understand the need for this?

I originally added this because early on in test development, the inference call used named writeables that weren't registered. Since we are properly simulating the inference call in the test now, the unregistered named writeables aren't used and this can be removed. Good catch!

... And I needed to add this back when I extended the serialization tests in 27dff4b. rewriteQuery now checks that the query can be serialized in between when it is rewritten on the coordinator & data nodes. For the semantic query, this means serializing InferenceResults, which in turn requires registering the named writeables for that class.

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java

...inference/src/yamlRestTest/resources/rest-api-spec/test/inference/40_semantic_text_query.yml

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

Mikep86 · 2024-05-14T20:01:14Z

@elasticsearchmachine run elasticsearch-ci/part-1

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java

benwtrent · 2024-05-14T20:07:35Z

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java

+                    InferenceAction.INSTANCE,
+                    inferenceRequest,
+                    listener.delegateFailureAndWrap((l, inferenceResponse) -> {
+                        inferenceResultsSupplier.set(inferenceResponse.getResults());


Should we check for warning inference results size & supported kinds early and fail?

This is a good point and also made me realize we should probably convert from InferenceServiceResults to InferenceResults in doRewriteGetInferenceResults, which is executed on the coordinator node.

As currently written, each shard will do this conversion individually (when doRewriteBuildSemanticQuery is executed). By checking & converting earlier, we prevent duplicate work and do (some of) the error checking in the coordinator node, avoiding an unnecessary serialization/deserialization when the inference results are clearly invalid.

@carlosdelest WDYT?

Makes sense to me - if we can avoid doing work that can be preempted in the coordinating node, let's do it

I addressed this, but it required more changes than I anticipated. This is due to the fact that prior to addressing this, the query enforced that inferenceResults is always non-null after re-writing on the coordinator node. With the changes in bceeb84, this is no longer the case, which created a couple more edge cases.

I also want to add a test to SemanticQuerBuilderTests to validate that the query can be serialized after re-writing on the coordinator node, particularly when no inference ID can be looked up for the field queried. This is not covered by the YAML tests because they all run on single-node clusters.

Added the test in 27dff4b

Thanks for adding the tests, Mike!

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java

...rTest/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilderMultiClusterIT.java

Mikep86 · 2024-05-15T21:51:02Z

@elasticsearchmachine run elasticsearch-ci/part-2

mayya-sharipova

@Mikep86 Great job and nice test coverage in yml tests. This PR overall LGTM, I have several small minor points and they are also subject to a discussion and can be addressed later:

I am not sure if this query should respect: index.query.parse.allow_unmapped_fields, as to my mind the primary use for this setting is for CCS and the query is not supported in CCS.
Not starting a separate SemanticQueryBuilderMultiClusterIT.java just to check that this query in not supported in CCS, but integrating into other CCS integration tests.
I assume documentation PR will be added later

Mikep86 · 2024-05-17T19:06:15Z

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java

+        if (inferenceResults instanceof ErrorInferenceResults errorInferenceResults) {
+            throw new IllegalStateException(
+                "Field [" + fieldName + "] query inference error: " + errorInferenceResults.getException().getMessage(),
+                errorInferenceResults.getException()
+            );
+        } else if (inferenceResults instanceof WarningInferenceResults warningInferenceResults) {
+            throw new IllegalStateException("Field [" + fieldName + "] query inference warning: " + warningInferenceResults.getWarning());


@davidkyle When would the inference service return ErrorInferenceResults or WarningInferenceResults? Should these result types be handled as user errors or server-side errors?

Mikep86 · 2024-05-17T20:12:31Z

test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

+        // The second rewriteAndFetch call simulates rewriting on the shard
+        QueryBuilder rewritten = rewriteAndFetch(queryBuilder, coordinatorRewriteContext);
+        // extra safety to fail fast - serialize the rewritten version to ensure it's serializable.
+        assertSerialization(rewritten);


This checks that the query can be serialized after being rewritten on the coordinator node, which is an important check that would otherwise require setting up a multi-node integration test environment.

carlosdelest

This LGTM - Awesome work

Mikep86 and others added 3 commits May 9, 2024 14:51

Semantic text query (elastic#106181)

71ad50f

Add the semantic query to the Query DSL, which is used to query semantic_text fields --------- Co-authored-by: carlosdelest <carlos.delgado@elastic.co> Co-authored-by: Jim Ferenczi <jim.ferenczi@elastic.co>

Semantic query YAML test fixes

37db731

Remove unnecessary package requirement

0f5131e

Mikep86 added >non-issue :Search/Search Search-related issues that do not fall into other categories :Search/Vectors Vector search v8.15.0 labels May 9, 2024

Mikep86 added 3 commits May 13, 2024 09:15

Merge branch 'main' into merge-semantic-query-to-main

c306caf

Move semantic query YAML tests

c905000

Fix YAML tests

6ef5054

Mikep86 commented May 13, 2024

View reviewed changes

Mikep86 marked this pull request as ready for review May 13, 2024 14:49

Mikep86 requested review from carlosdelest and jimczi May 13, 2024 14:49

elasticsearchmachine added the Team:Search Meta label for search team label May 13, 2024

Mikep86 requested review from benwtrent and mayya-sharipova May 13, 2024 14:50

benwtrent reviewed May 13, 2024

View reviewed changes

carlosdelest reviewed May 14, 2024

View reviewed changes

Mikep86 added 4 commits May 14, 2024 14:04

Make inferenceResultsSupplier ephemeral

d732844

Updated YAML test version check

cd5fb7a

Don't register plugin named writeables

8b5c678

Merge branch 'main' into merge-semantic-query-to-main

63e7ee5

benwtrent reviewed May 14, 2024

View reviewed changes

mayya-sharipova reviewed May 15, 2024

View reviewed changes

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java Show resolved Hide resolved

mayya-sharipova reviewed May 15, 2024

View reviewed changes

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java Outdated Show resolved Hide resolved

mayya-sharipova reviewed May 15, 2024

View reviewed changes

.../inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java Show resolved Hide resolved

mayya-sharipova reviewed May 15, 2024

View reviewed changes

...rTest/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilderMultiClusterIT.java Outdated Show resolved Hide resolved

Mikep86 added 4 commits May 15, 2024 15:41

Make inferenceResultsSupplier & inferenceResults final

6d86eae

Updated YAML tests to use multiple chunks

7516325

Added invalid inference ID YAML test

ade696a

Test querying multiple indices that use the same inference ID

a0645dd

mayya-sharipova approved these changes May 16, 2024

View reviewed changes

Mikep86 added 6 commits May 16, 2024 14:55

Check that query inference results match model settings

4467e39

Added inference endpoint changes YAML tests

e080fb7

Fix spotless violations

2a83e2c

Merge branch 'main' into merge-semantic-query-to-main

30430b9

Converted multi-cluster integration tests to YAML tests

a66a45c

Validate & convert inference results on coordinator node

bceeb84

Mikep86 commented May 17, 2024

View reviewed changes

Test serializing the query when no inference ID could be resolved

27dff4b

Mikep86 commented May 17, 2024

View reviewed changes

carlosdelest approved these changes May 20, 2024

View reviewed changes

Merge branch 'main' into merge-semantic-query-to-main

cef0a3a

Mikep86 merged commit 8bc1a47 into elastic:main May 20, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semantic Query #108483

Semantic Query #108483

Mikep86 commented May 9, 2024 •

edited

Mikep86 May 13, 2024

elasticsearchmachine commented May 13, 2024

benwtrent May 13, 2024

Mikep86 May 14, 2024

Mikep86 May 17, 2024

Mikep86 commented May 14, 2024

benwtrent May 14, 2024

Mikep86 May 16, 2024

carlosdelest May 16, 2024

Mikep86 May 17, 2024 •

edited

Mikep86 May 17, 2024

carlosdelest May 20, 2024

Mikep86 commented May 15, 2024

mayya-sharipova left a comment •

edited

Mikep86 May 17, 2024

Mikep86 May 17, 2024

carlosdelest left a comment

Semantic Query #108483

Semantic Query #108483

Conversation

Mikep86 commented May 9, 2024 • edited

Choose a reason for hiding this comment

elasticsearchmachine commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikep86 commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikep86 May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikep86 commented May 15, 2024

mayya-sharipova left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest left a comment

Choose a reason for hiding this comment

Mikep86 commented May 9, 2024 •

edited

Mikep86 May 17, 2024 •

edited

mayya-sharipova left a comment •

edited