Treat . as a nested field in field_map of text embedding processor #488

Sanjana679 · 2023-11-09T09:22:56Z

Description

These changes will allow embeddings to be computed when using a nested source field for a text embedding processor as shown in the configuration below:

PUT /_ingest/pipeline/nlp-ingest-pipeline
{
  "description": "An NLP ingest pipeline",
  "processors": [
    {
      "text_embedding": {
        "model_id": "",
        "field_map": {
          “message.text": “message_embedding”
        }
      }
    }
  ]
}

PUT /my-nlp-index
{
    "settings": {
        "index.knn": true,
        "default_pipeline": “nlp-ingest-pipeline”
    },
    "mappings": {
        "properties": {
            "message_embedding": {
                "type": "knn_vector",
                "dimension": 384,
                "method": {
                    "name": "hnsw",
                    "engine": "lucene"
                }
            },
            "message.text": { 
                "type": "text"            
            },
            "color": {
                "type": "text"
            }
        }
    }
}

PUT /my-nlp-index/_doc/1
{
  "message: {"text": "Blueberry smoothie"},
  "color": "blue"
}

GET /my-nlp-index/_doc/1

Issues Resolved

#110

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Sanjana679 <[email protected]>

…ing processor Signed-off-by: Sanjana679 <[email protected]>

martin-gaievski · 2023-11-10T22:59:55Z

how many nested levels are we going to support? We need to state that if it's more than 2 levels, e.g.
my_object.some_property_group.some_field. Currently with mapping we can do multiple levels. @navneet1v what do you think?

codecov · 2023-11-10T23:01:07Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (b3c73bd) 84.65% compared to head (2c7f491) 84.10%.

❗ Current head 2c7f491 differs from pull request most recent head 2177ba2. Consider uploading reports for the commit 2177ba2 to get more accurate results

Files	Patch %	Lines
...rch/neuralsearch/processor/InferenceProcessor.java	16.66%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #488      +/-   ##
============================================
- Coverage     84.65%   84.10%   -0.55%     
+ Complexity      508      498      -10     
============================================
  Files            40       40              
  Lines          1505     1497       -8     
  Branches        234      229       -5     
============================================
- Hits           1274     1259      -15     
- Misses          128      137       +9     
+ Partials        103      101       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

martin-gaievski · 2023-11-10T23:00:51Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

@@ -154,6 +154,16 @@ Map<String, Object> buildMapWithProcessorKeyAndOriginalValue(IngestDocument inge
        for (Map.Entry<String, Object> fieldMapEntry : fieldMap.entrySet()) {
            String originalKey = fieldMapEntry.getKey();
            Object targetKey = fieldMapEntry.getValue();
+
+            int nestedDotIndex = originalKey.indexOf('.');


This is a user-provided info, can we add basic validation if it's not already done as part of the processor/pipeline definition.

if multiple levels of nested fields are needed this code may need a rework

There is some basic validation done in validateEmbeddingConfiguration which is run on fieldMap in the constructor in this file. However, I will add some extra validation for nested fields.

martin-gaievski · 2023-11-10T23:01:31Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

+
+            int nestedDotIndex = originalKey.indexOf('.');
+            if (nestedDotIndex != -1) {
+                Map<String, Object> temp = new LinkedHashMap<>();


why do we need this map object? Can you please use more meaningful name for map variable

I added a more meaningful name for the map variable.

Signed-off-by: Sanjana679 <[email protected]>

navneet1v · 2023-11-13T17:52:59Z

@Sanjana679 thanks for raising the PR. Can you please add output for this case.

PUT /_ingest/pipeline/nlp-ingest-pipeline
{
  "description": "An NLP ingest pipeline",
  "processors": [
    {
      "text_embedding": {
        "model_id": "",
        "field_map": {
          “message.text": “message.message_embedding”
        }
      }
    }
  ]
}

Also I don't see any integration test added, can we add the integration tests.

navneet1v · 2023-11-13T17:53:50Z

@Sanjana679 can you also run ./gradlew spotlessJavaCheck before raising the PR. It will fix the formatting issues.

navneet1v · 2023-11-13T17:55:50Z

@Sanjana679 I can within the diff there are lines which are coming because of the different formatter you might have in your local. Please fix that too and make sure that the lines which are actually modified are coming in the diff.

navneet1v · 2023-11-13T17:59:08Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

+            if (nestedDotIndex != -1) {
+                Map<String, Object> newTargetKey = new LinkedHashMap<>();
+                newTargetKey.put(originalKey.substring(nestedDotIndex + 1), targetKey);
+                targetKey = newTargetKey;
+
+                originalKey = originalKey.substring(0, nestedDotIndex);
+            }


@Sanjana679 can you please provide details how we are handling multiple level of nesting here?

At the moment, I'm not currently handling multiple levels of nesting, as I initially thought it was only for one level. However, I will work on handling multiple levels of nesting.

navneet1v · 2023-11-20T22:28:16Z

@Sanjana679 can you also see with your changes this issue will be resolved or not: https://forum.opensearch.org/t/need-neural-search-plugin-to-support-nested-field-type-array-of-objects/16760 ?

…ocessor-nested-fields

Sanjana679 · 2023-11-22T18:10:54Z

@Sanjana679 can you also see with your changes this issue will be resolved or not: https://forum.opensearch.org/t/need-neural-search-plugin-to-support-nested-field-type-array-of-objects/16760 ?

At the moment, my changes don't resolve this issue but if I can easily resolve this issue I will work on it.

navneet1v · 2023-11-22T22:49:15Z

@Sanjana679 can you also see with your changes this issue will be resolved or not: https://forum.opensearch.org/t/need-neural-search-plugin-to-support-nested-field-type-array-of-objects/16760 ?

At the moment, my changes don't resolve this issue but if I can easily resolve this issue I will work on it.

I checked the question the way user is using is not correct.

can we check if the processor is created like this below:

{
	“description”: “MLsearchtestpipeline”,
	“processors”: [
		{
			“text_embedding”: {
				“model_id”: “8-S0XYsBFQTip4T18k-U”,
				“field_map”: {
					“question_embeddings.text”: "question_embeddings.text_embedding"
				}
			}
		}
	]
}

will it work or not.

sam-herman

Can you please add tests to cover the new logic described here for nested fields ingestion?

sam-herman · 2023-12-07T21:35:05Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

        super(tag, description);
        this.type = type;
-        if (StringUtils.isBlank(modelId)) throw new IllegalArgumentException("model_id is null or empty, cannot process it");
+        if (StringUtils.isBlank(modelId))


why did the formatting change? did you run ./gradlew :spotlessApply prior?

sam-herman · 2023-12-07T21:47:22Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

@@ -154,9 +164,20 @@ Map<String, Object> buildMapWithProcessorKeyAndOriginalValue(IngestDocument inge
        for (Map.Entry<String, Object> fieldMapEntry : fieldMap.entrySet()) {
            String originalKey = fieldMapEntry.getKey();
            Object targetKey = fieldMapEntry.getValue();
+
+            int nestedDotIndex = originalKey.indexOf('.');


nit: use a static constant and avoid magic characters in code.
e.g.

private static final char FIELD_SEPARATOR = '.';

yuye-aws · 2024-03-05T03:22:41Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

+            String parentKey,
+            Object processorKey,
+            Map<String, Object> sourceAndMetadataMap,
+            Map<String, Object> treeRes) {


Please remove these indents.

yuye-aws · 2024-03-05T03:23:37Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

+                        nestedFieldMapEntry.getKey(),
+                        nestedFieldMapEntry.getValue(),
+                        (Map<String, Object>) sourceAndMetadataMap.get(parentKey),
+                        next);


Keep ) consistent

yuye-aws · 2024-03-05T03:25:11Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

@@ -211,18 +233,21 @@ private void validateEmbeddingFieldsValue(IngestDocument ingestDocument) {
    private void validateNestedTypeValue(String sourceKey, Object sourceValue, Supplier<Integer> maxDepthSupplier) {


Can we replace Supplier<Integer> with a simple int?

yuye-aws · 2024-03-05T03:30:28Z

src/main/java/org/opensearch/neuralsearch/processor/InferenceProcessor.java

+                    .stream()
+                    .filter(Objects::nonNull)
+                    .forEach(x -> validateNestedTypeValue(sourceKey, x, () -> maxDepth + 1));


Remove these indents

martin-gaievski · 2024-07-26T18:39:02Z

implemented same logic in #841, we can close this PR

Sanjana679 requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei, martin-gaievski, sean-zheng-amazon, model-collapse, zane-neo, ylwu-amzn and jngz-es as code owners November 9, 2023 09:22

Sanjana679 added 2 commits November 9, 2023 04:36

Treat dot as nested field in field_map for inference processor

7006c97

Signed-off-by: Sanjana679 <[email protected]>

Treat dot in field name as a nested field in field_map of text embedd…

2c7f491

…ing processor Signed-off-by: Sanjana679 <[email protected]>

Sanjana679 force-pushed the feature/text-embedding-processor-nested-fields branch from a428d30 to 2c7f491 Compare November 9, 2023 09:37

martin-gaievski reviewed Nov 10, 2023

View reviewed changes

martin-gaievski added the skip-changelog label Nov 10, 2023

Sanjana679 added 2 commits November 11, 2023 21:10

More meaningful name for new targetKey value

a5b1c4a

Signed-off-by: Sanjana679 <[email protected]>

Added validation for nested fields in field_map

0e38fea

Signed-off-by: Sanjana679 <[email protected]>

navneet1v reviewed Nov 13, 2023

View reviewed changes

Merge branch 'opensearch-project:main' into feature/text-embedding-pr…

2177ba2

…ocessor-nested-fields

sam-herman reviewed Dec 7, 2023

View reviewed changes

yuye-aws reviewed Mar 5, 2024

View reviewed changes

martin-gaievski closed this Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat . as a nested field in field_map of text embedding processor #488

Treat . as a nested field in field_map of text embedding processor #488

Sanjana679 commented Nov 9, 2023 •

edited

Loading

martin-gaievski commented Nov 10, 2023

codecov bot commented Nov 10, 2023 •

edited

Loading

martin-gaievski Nov 10, 2023

Sanjana679 Nov 12, 2023

martin-gaievski Nov 10, 2023

Sanjana679 Nov 12, 2023

navneet1v commented Nov 13, 2023 •

edited

Loading

navneet1v commented Nov 13, 2023

navneet1v commented Nov 13, 2023

navneet1v Nov 13, 2023

Sanjana679 Nov 13, 2023

navneet1v commented Nov 20, 2023

Sanjana679 commented Nov 22, 2023

navneet1v commented Nov 22, 2023

sam-herman left a comment

sam-herman Dec 7, 2023

sam-herman Dec 7, 2023

yuye-aws Mar 5, 2024

yuye-aws Mar 5, 2024

yuye-aws Mar 5, 2024

yuye-aws Mar 5, 2024

martin-gaievski commented Jul 26, 2024

		@@ -211,18 +233,21 @@ private void validateEmbeddingFieldsValue(IngestDocument ingestDocument) {
		private void validateNestedTypeValue(String sourceKey, Object sourceValue, Supplier<Integer> maxDepthSupplier) {

Treat . as a nested field in field_map of text embedding processor #488

Treat . as a nested field in field_map of text embedding processor #488

Conversation

Sanjana679 commented Nov 9, 2023 • edited Loading

Description

Issues Resolved

Check List

martin-gaievski commented Nov 10, 2023

codecov bot commented Nov 10, 2023 • edited Loading

Codecov Report

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

navneet1v commented Nov 13, 2023 • edited Loading

navneet1v commented Nov 13, 2023

navneet1v commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navneet1v commented Nov 20, 2023

Sanjana679 commented Nov 22, 2023

navneet1v commented Nov 22, 2023

sam-herman 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

martin-gaievski commented Jul 26, 2024

Sanjana679 commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 10, 2023 •

edited

Loading

navneet1v commented Nov 13, 2023 •

edited

Loading