Semantic text - field mapper #102971

carlosdelest · 2023-12-05T11:32:58Z

This creates a new semantic_text field mapper, that receives a model_id parameter.

model_id will be used to perform inference on document ingestion and query time.

PUT test-index
{
    "mappings": {
        "properties": {
            "my-inference-field": {
                "type": "semantic_text",
                "model_id": "my-model-id"
            }
      }
}

This field mapper does not index values - that will be taken care of by a metadata field mapper, as the inference results will be included in a different top level field.

A new getInferenceModel() method has been added to MappedFieldType so we'll be able to get information of the models used in the different fields from the MappingLookup and related structures in future PRs.

carlosdelest · 2023-12-05T12:26:06Z

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

@@ -203,6 +203,14 @@ public List<String> dimensions() {
        return Collections.emptyList();
    }

+    public String getInferenceModel() {


Method added for future retrieval from MappingLookup

@carlosdelest I would prefer a new interface & instanceof to check that something satisfies this new API. I don't like the idea of having it at the base MappedFieldType.

I am unclear on where the method would be called / where would the instanceof check go. Could you clarify?

@javanna I don't think we should update any core methods until we know 100% where they are going to be used. Consequently, I am against adding anything like this to MappedFieldType.

If anything, it seems like no matter where this is called an instanceof check against some other interface would work. @carlosdelest we should defer any changes here until we know where and how its going to be used.

I agree on the sentiment, I just could not see what the impact is of an instanceof because the method is currently only exposed but never used. It is also true that this kind of special cases complicate the code in the long run, but I see what you mean.

@javanna I think @carlosdelest is trying to lay the ground work for gathering field capabilities in some way to determine if a model is used and what model.

I agree, it's way too early here to make any changes to wide expansive interfaces like this one.

As Ben said, this will be used to gather the fields that are using an ML model so they can be included in IndexMetadata in a later stage.

I'm OK with using a new interface and defer the decision to include this method as part of the base class.

@Mikep86, please be aware that I'll be moving getInferenceModel() from MappedFieldType to a new inteface.

Addressed in 048779b

carlosdelest · 2023-12-05T12:46:30Z

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

@@ -447,7 +447,10 @@ public void onIndexModule(IndexModule indexModule) {

    @Override
    public Function<String, Predicate<String>> getFieldFilter() {
-        List<Function<String, Predicate<String>>> items = filterPlugins(MapperPlugin.class).stream().map(p -> p.getFieldFilter()).toList();
+        List<Function<String, Predicate<String>>> items = filterPlugins(MapperPlugin.class).stream()


Adding a new MapperPlugin broke this check. Check that an actual filter has been used.

This doesn't make sense to me. We shouldn't change this. Why do the other plugins that satisfy the mapping plugin don't suffer from this?

What is the actual issue here?

Thanks for the heads up Ben!

Not doing this made some tests fail (see this test result).

My understanding was that we were adding another MapperPlugin that was not accounted for - thus the change to understand that the MapperPlugin did something useful.

I've been checking and after merging with main this is no longer an issue, so I've withdrawn the changes.

OK, but I still see the change here, you are going to revert this in a future commit here?

Same case as the other one - fixed in d7d8d5e

OK - removing this caused tests to fail. I've looked deeper into this issue:

LocalStateCompositeXPackPlugin is used for testing

No other subclasses of LocalStateCompositeXPackPlugin use a MapperPlugin as part of its plugins

LocalStateMachineLearning has as plugins Security (which is a MapperPlugin) and MachineLearning. Making MachineLearning another MapperPlugin implied that there are 2 MapperPlugins

The existing code does not check whether the actual getFieldsFilter() method has been overriden or not, but just the numbrer of MapperPlugins

So I think the changes I've done actually preserve the original intention - we're not looking for how many MapperPlugins there are, but how many of them actually override the method (that is, do not return NOOP_FIELD_FILTER.

Does it make sense? Happy to discuss if not

@carlosdelest I see now. Seems good to me

++ I say this is ok.

elasticsearchmachine · 2023-12-06T10:40:35Z

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

benwtrent · 2023-12-06T21:07:38Z

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

@@ -203,6 +203,14 @@ public List<String> dimensions() {
        return Collections.emptyList();
    }

+    public String getInferenceModel() {


@carlosdelest I would prefer a new interface & instanceof to check that something satisfies this new API. I don't like the idea of having it at the base MappedFieldType.

benwtrent · 2023-12-06T21:15:20Z

...k/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java

@@ -447,7 +447,10 @@ public void onIndexModule(IndexModule indexModule) {

    @Override
    public Function<String, Predicate<String>> getFieldFilter() {
-        List<Function<String, Predicate<String>>> items = filterPlugins(MapperPlugin.class).stream().map(p -> p.getFieldFilter()).toList();
+        List<Function<String, Predicate<String>>> items = filterPlugins(MapperPlugin.class).stream()


This doesn't make sense to me. We shouldn't change this. Why do the other plugins that satisfy the mapping plugin don't suffer from this?

What is the actual issue here?

benwtrent · 2023-12-06T21:20:59Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java

+
+    public static class Builder extends FieldMapper.Builder {
+
+        private final Parameter<String> modelId = Parameter.stringParam("model_id", false, m -> builder(m).modelId.get(), null)


Suggested change

private final Parameter<String> modelId = Parameter.stringParam("model_id", false, m -> builder(m).modelId.get(), null)

private final Parameter<String> modelId = Parameter.stringParam("model_id", false, m -> toType(m).fieldType().modelId, null)

Or at least don't create a builder here. That seems unnecessary.

Good call - removing it

This reverts commit 55c7ae1.

# Conflicts: # test/test-clusters/src/main/java/org/elasticsearch/test/cluster/FeatureFlag.java # x-pack/plugin/src/yamlRestTest/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java

This reverts commit fd235f5

original-brownbear

Added a couple of smaller points on cleaning up the code a little :)

original-brownbear · 2023-12-11T13:26:41Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java

+
+    public static final TypeParser PARSER = new TypeParser((n, c) -> new Builder(n), notInMultiFields(CONTENT_TYPE));
+
+    public static class SemanticTextFieldType extends SimpleMappedFieldType implements InferenceModelFieldType {


Could we move this nested class to the bottom of the file please? I know we have it like this in other mappers but it makes the code very hard to follow as can be seen by the accidental introduction of duplicate state across the field and mapper here IMO :)

Makes sense - done in 4b06655

original-brownbear · 2023-12-11T13:27:02Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java

+        }
+    }
+
+    private final String modelId;


We already have the modelId on the field, I don't think we also need it on the mapper redundantly?

Good call - we could always retrieve it from the field type. Addressed in 39e061f

original-brownbear · 2023-12-11T13:28:12Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/mapper/SemanticTextFieldMapper.java

+
+    private final String modelId;
+
+    private final Builder builder;


Retaining the builder has been causing trouble with memory in the past. It shouldn't matter too much here but still, could we just retain the explicit data we need from the builder instead?

Thanks for the context. I removed the need for storing the Builder previously, so I removed builder refs entirely in 4b06655

benwtrent · 2023-12-11T16:40:46Z

test/test-clusters/src/main/java/org/elasticsearch/test/cluster/FeatureFlag.java

-    LEARNING_TO_RANK("es.learning_to_rank_feature_flag_enabled=true", Version.fromString("8.12.0"), null),
+    LEARN_TO_RANK("es.learn_to_rank_feature_flag_enabled=true", Version.fromString("8.10.0"), null),


seems like an unnecessary change? Merge in main?

It is - reverting some past commits negated the merge from main. Thanks for the catch

Fixed in 07613ae

benwtrent · 2023-12-11T16:42:00Z

x-pack/plugin/src/yamlRestTest/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java

-        .feature(FeatureFlag.LEARNING_TO_RANK)
+        .feature(FeatureFlag.LEARN_TO_RANK)


again, seems unnecessary? Merge in main?

Same as before - fixed in 07613ae

carlosdelest added 3 commits December 4, 2023 18:28

Feature Flag implementation

ffd86f7

SemanticTextFieldMapper implementation

085751e

Added tests

03423e3

elasticsearchmachine added the v8.12.0 label Dec 5, 2023

Spotless

64efbeb

carlosdelest commented Dec 5, 2023

View reviewed changes

carlosdelest added 3 commits December 5, 2023 13:31

Added javadoc and comments

34360e2

Use feature flag to add new field type

8280753

Fix check when adding a new MapperPlugin

66fa2cc

carlosdelest commented Dec 5, 2023

View reviewed changes

carlosdelest marked this pull request as ready for review December 5, 2023 13:50

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Dec 5, 2023

carlosdelest added Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label labels Dec 5, 2023

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Search Meta label for search team labels Dec 5, 2023

carlosdelest added >non-issue Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label v8.12.0 labels Dec 5, 2023

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Search Meta label for search team labels Dec 5, 2023

carlosdelest requested review from a team, benwtrent and jimczi December 5, 2023 13:52

carlosdelest added Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label labels Dec 5, 2023

elasticsearchmachine added needs:triage Requires assignment of a team area label and removed Team:Search Meta label for search team labels Dec 5, 2023

carlosdelest removed the needs:triage Requires assignment of a team area label label Dec 5, 2023

mark-vieira added v8.12.0 and removed v8.13.0 labels Dec 5, 2023

thecoop added :Search Foundations/Mapping Index mappings, including merging and defining field types and removed needs:triage Requires assignment of a team area label labels Dec 6, 2023

elasticsearchmachine added the Team:Search Meta label for search team label Dec 6, 2023

benwtrent reviewed Dec 6, 2023

View reviewed changes

brianseeders added v8.13.0 and removed v8.12.0 labels Dec 6, 2023

carlosdelest added 7 commits December 11, 2023 10:11

Extracted a new interface for getInferenceModel()

048779b

Removing changes to LocalStateCompositeXPackPlugin

55c7ae1

Removed unnecessary builder

e448d3d

Add semantic text feature flag to REST IT for completeness

fd235f5

Revert "Removing changes to LocalStateCompositeXPackPlugin"

b93cf5d

This reverts commit 55c7ae1.

Merge branch 'main' into carlosdelest/semantic-text-field-mapper

43c5cdb

# Conflicts: # test/test-clusters/src/main/java/org/elasticsearch/test/cluster/FeatureFlag.java # x-pack/plugin/src/yamlRestTest/java/org/elasticsearch/xpack/test/rest/XPackRestIT.java

Revert "Add semantic text feature flag to REST IT for completeness"

c0c1125

This reverts commit fd235f5

original-brownbear reviewed Dec 11, 2023

View reviewed changes

carlosdelest added 2 commits December 11, 2023 16:00

Moved FieldType to the bottom of the file

4b06655

Removed redundant modelId

39e061f

carlosdelest requested review from benwtrent and original-brownbear December 11, 2023 16:22

benwtrent reviewed Dec 11, 2023

View reviewed changes

carlosdelest added 2 commits December 11, 2023 17:44

Fix merge from main

07613ae

Fix merge from main

d7d8d5e

benwtrent approved these changes Dec 11, 2023

View reviewed changes

carlosdelest added 2 commits December 11, 2023 20:00

Add back filtering for actual getFieldFilter() overriding

fd83fe9

Spotless

d8ae624

original-brownbear approved these changes Dec 12, 2023

View reviewed changes

carlosdelest merged commit 8ee4721 into elastic:main Dec 12, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semantic text - field mapper #102971

Semantic text - field mapper #102971

carlosdelest commented Dec 5, 2023

carlosdelest Dec 5, 2023

benwtrent Dec 6, 2023

javanna Dec 7, 2023

benwtrent Dec 7, 2023

javanna Dec 7, 2023

benwtrent Dec 7, 2023

carlosdelest Dec 11, 2023

carlosdelest Dec 11, 2023

carlosdelest Dec 5, 2023

benwtrent Dec 6, 2023

carlosdelest Dec 11, 2023

benwtrent Dec 11, 2023

carlosdelest Dec 11, 2023

carlosdelest Dec 11, 2023

benwtrent Dec 11, 2023

benwtrent Dec 11, 2023

elasticsearchmachine commented Dec 6, 2023

benwtrent Dec 6, 2023

benwtrent Dec 6, 2023

benwtrent Dec 6, 2023

carlosdelest Dec 11, 2023

original-brownbear left a comment

original-brownbear Dec 11, 2023

carlosdelest Dec 11, 2023

original-brownbear Dec 11, 2023

carlosdelest Dec 11, 2023

original-brownbear Dec 11, 2023

carlosdelest Dec 11, 2023

benwtrent Dec 11, 2023

carlosdelest Dec 11, 2023

carlosdelest Dec 11, 2023

benwtrent Dec 11, 2023

carlosdelest Dec 11, 2023


		public static class Builder extends FieldMapper.Builder {

		private final Parameter<String> modelId = Parameter.stringParam("model_id", false, m -> builder(m).modelId.get(), null)


		public static final TypeParser PARSER = new TypeParser((n, c) -> new Builder(n), notInMultiFields(CONTENT_TYPE));

		public static class SemanticTextFieldType extends SimpleMappedFieldType implements InferenceModelFieldType {

		LEARNING_TO_RANK("es.learning_to_rank_feature_flag_enabled=true", Version.fromString("8.12.0"), null),
		LEARN_TO_RANK("es.learn_to_rank_feature_flag_enabled=true", Version.fromString("8.10.0"), null),

		.feature(FeatureFlag.LEARNING_TO_RANK)
		.feature(FeatureFlag.LEARN_TO_RANK)

Semantic text - field mapper #102971

Semantic text - field mapper #102971

Conversation

carlosdelest commented Dec 5, 2023

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

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

elasticsearchmachine commented Dec 6, 2023

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

original-brownbear 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment