semantic_text field mapping #107519

carlosdelest · 2024-04-16T11:15:05Z

This PR contains changes related to just the semantic_text field mapping, including some specific YAML mapping tests.

Automatic Inference on ingestion will be included as a separate PR. This change deals with just the mapping side:

Provides the mapping definition for semantic_text
On document ingestion, checks for the model settings in its mapping. If it's not present, updates it to ensure documents adhere to the mapping in the future.
Creates the appropriate mappings for dense_vector or sparse_vector fields internally
Validates and indexes the inference results as part of the document ingestion using these internal mappings

…e branch

Fix the merging of the object field within the semantic_text mapper, the merge context should be set at the parent level (was at the object/child level before merging).

…-text-field-mapping-specifics # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/SemanticTextFeature.java

elasticsearchmachine · 2024-04-16T13:09:56Z

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

benwtrent

First pass complete. Need to go through the tests now :)

benwtrent · 2024-04-16T13:39:18Z

server/src/test/java/org/elasticsearch/index/mapper/CopyToMapperTests.java

@@ -22,6 +22,7 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;


Could you add a test here that ensures the InferenceFieldMapper logic is correct? Generally this is done by making a small test only plugin that provides a mapper that satisfies this InferenceFieldMapper interface.

Makes sense. I've added them in a new test class - MappingLookupInferenceFieldMapperTests. LMKWYT!

benwtrent · 2024-04-16T13:42:08Z

x-pack/plugin/inference/src/main/java/module-info.java

@@ -17,6 +17,7 @@
    requires org.apache.httpcomponents.httpasyncclient;
    requires org.apache.httpcomponents.httpcore.nio;
    requires org.apache.lucene.core;
+    requires org.elasticsearch.logging;


I don't understand this inclusion.

Good catch - it seems we're not using logging. I'm removing it 👍

...ugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextField.java

benwtrent · 2024-04-16T13:45:38Z

...ugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextField.java

+        args -> new InferenceResult((String) args[0], (ModelSettings) args[1], (List<Chunk>) args[2])
+    );
+
+    @SuppressWarnings("unchecked")


Suggested change

@SuppressWarnings("unchecked")

This doesn't need the suppression

...ugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextField.java

benwtrent · 2024-04-16T13:50:03Z

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

+public class SemanticTextFieldMapper extends FieldMapper implements InferenceFieldMapper {
+    public static final String CONTENT_TYPE = "semantic_text";
+
+    private static final Logger logger = LogManager.getLogger(SemanticTextFieldMapper.class);


This logger is unused?

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

benwtrent · 2024-04-16T13:56:29Z

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

+        if (current == null) {
+            conflicts.addConflict("model_settings", "");
+            return false;
+        }
+        conflicts.addConflict("model_settings", "");
+        return false;


These two branches are exactly the same, is this on purpose?

Makes no sense, I'm simplifying it

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

...lugin/inference/src/yamlRestTest/java/org/elasticsearch/xpack/inference/InferenceRestIT.java

carlosdelest · 2024-04-17T07:50:23Z

@elasticsearchmachine update branch

server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java

server/src/main/java/org/elasticsearch/index/mapper/Mapping.java

… PRs

original-brownbear

Didn't get through all of this yet but left a couple of things that should get fixed IMO. I'll continue later

server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java

...ugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextField.java

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

benwtrent

Some comments on the tests. But I think this is almost ready to go :)

...e/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping.yml

...s/rest-api-spec/test/inference/20_semantic_text_field_mapping_incompatible_field_mapping.yml

benwtrent · 2024-04-29T11:39:18Z

...e/src/yamlRestTest/resources/rest-api-spec/test/inference/10_semantic_text_field_mapping.yml

@@ -0,0 +1,105 @@
+setup:


Could you add some tests for the following cases? Yaml tests give us a ton more coverage.

Multi-field (itself as a multi-field and multi-fields UNDER it)

nested fields

copy to

Missing inference chunk text, missing inference results (you already have good tests around bad params or mixed types, huzzah!)

Thanks Ben, those were definitely good tests to have:

Multi-field (itself as a multi-field and multi-fields UNDER it)

It can't be used as a multifield (added test)

For now, we're disabling multi-fields for semantic_text and provide support later (added check and test). It would imply checking the field content to understand if it's coming from a reindex or an index operation in order to access the appropriate content.

nested fields
Added test to check it can be used as a nested field

copy to

For the same reasons than multi-fields, disabling to use it as the origin of copy_to.

It can be used as a copy_to target, but this support will be added for the inference PR to come next.

Missing inference chunk text, missing inference results
Added them 👍

the copy-to check will be interesting, we need to be sure to handle it correctly :)

…-text-field-mapping-specifics

carlosdelest · 2024-04-30T07:51:29Z

@elasticmachine update branch

…cifics

carlosdelest · 2024-04-30T15:12:58Z

@elasticmachine update branch

…cifics

carlosdelest and others added 9 commits April 16, 2024 10:06

Remove code remnants from semantic_text previous iterations in featur…

db9a383

…e branch

Remove unnecessary interface

78f8880

Remove SemanticTextModelSettings

7c42501

Fix test

faef7e2

Add semantic_text field mapper specifics

3359c9a

Add mapping specific tests

d4fdeb7

Revert changes to inference service

59bb7ae

Fix redundant path in semantic text field merging (elastic#107507)

3b8c49e

Fix the merging of the object field within the semantic_text mapper, the merge context should be set at the parent level (was at the object/child level before merging).

Merge remote-tracking branch 'origin/main' into carlosdelest/semantic…

3859fad

…-text-field-mapping-specifics # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/SemanticTextFeature.java

elasticsearchmachine added the v8.14.0 label Apr 16, 2024

carlosdelest added >non-issue :Search/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team labels Apr 16, 2024

carlosdelest mentioned this pull request Apr 16, 2024

semantic_text field mapper and inference #107262

Closed

carlosdelest marked this pull request as ready for review April 16, 2024 13:09

carlosdelest requested a review from a team as a code owner April 16, 2024 13:09

benwtrent reviewed Apr 16, 2024

View reviewed changes

carlosdelest added 4 commits April 16, 2024 16:39

Remove logging dependency as it's unused

eacd6ad

Remove unneeded unchecked warning

92cdfa8

PR feedback

914ce1c

PR feedback

db36d14

mark-vieira reviewed Apr 16, 2024

View reviewed changes

...lugin/inference/src/yamlRestTest/java/org/elasticsearch/xpack/inference/InferenceRestIT.java Outdated Show resolved Hide resolved

Using integration test distribution for YAML tests

b2ab726

javanna reviewed Apr 17, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java Show resolved Hide resolved

javanna reviewed Apr 17, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/Mapping.java Outdated Show resolved Hide resolved

PR review

b307a76

elasticsearchmachine added v8.15.0 and removed v8.14.0 labels Apr 17, 2024

carlosdelest added 3 commits April 18, 2024 17:01

Improve detection of illegal values and testing

d565c55

PR review - moving back methods to field mapper to be reused in later…

b3246a6

… PRs

PR review - add validations for ModelSettings

6a32de2

original-brownbear reviewed Apr 22, 2024

View reviewed changes

carlosdelest added 3 commits April 22, 2024 17:18

Add tests for InferenceFieldMapper integration in MappingLookup

c2ab40f

Change ParseField for Strings to improve reading

11d6b3c

Ensure parser closing

f8dfd9c

carlosdelest requested a review from benwtrent April 24, 2024 07:19

benwtrent reviewed Apr 29, 2024

View reviewed changes

carlosdelest added 2 commits April 29, 2024 13:48

Merge remote-tracking branch 'origin/main' into carlosdelest/semantic…

251ddb5

…-text-field-mapping-specifics

PR feedback on tests

0417268

benwtrent approved these changes Apr 29, 2024

View reviewed changes

elasticmachine and others added 2 commits April 30, 2024 08:51

Merge branch 'main' into carlosdelest/semantic-text-field-mapping-spe…

b382b3b

…cifics

toSemanticTextFieldChunks() method belongs to SemanticTextField - fixing

60f5e72

Merge branch 'main' into carlosdelest/semantic-text-field-mapping-spe…

fad7ab6

…cifics

carlosdelest merged commit aad04b1 into elastic:main Apr 30, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

semantic_text field mapping #107519

semantic_text field mapping #107519

carlosdelest commented Apr 16, 2024

elasticsearchmachine commented Apr 16, 2024

benwtrent left a comment

benwtrent Apr 16, 2024

carlosdelest Apr 22, 2024

benwtrent Apr 16, 2024

carlosdelest Apr 16, 2024

benwtrent Apr 16, 2024

carlosdelest Apr 16, 2024

benwtrent Apr 16, 2024

benwtrent Apr 16, 2024

carlosdelest Apr 16, 2024

carlosdelest commented Apr 17, 2024

original-brownbear left a comment

benwtrent left a comment

benwtrent Apr 29, 2024

carlosdelest Apr 29, 2024

benwtrent Apr 29, 2024

carlosdelest commented Apr 30, 2024

carlosdelest commented Apr 30, 2024

semantic_text field mapping #107519

semantic_text field mapping #107519

Conversation

carlosdelest commented Apr 16, 2024

elasticsearchmachine commented Apr 16, 2024

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest commented Apr 17, 2024

original-brownbear left a comment

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosdelest commented Apr 30, 2024

carlosdelest commented Apr 30, 2024