Completion types with multi-fields support #34081

pgomulka · 2018-09-26T13:01:38Z

Mappings with completion type and multi-fields, were not able to index array or
object format on completion fields. Only string format was supproted.
This is fixed by providing multiField parser with externalValueContext with already parsed object

closes #15115

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?
If submitting code, have you built your formula locally prior to submission with gradle check?
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
If submitting code, have you checked that your submission is for an OS that we support?
If you are submitting this code for a class then read our policy for that.

elasticmachine · 2018-09-26T14:08:57Z

Pinging @elastic/es-search-aggs

jimczi

Thanks @pgomulka , it's a good start. I left some comments.

jimczi · 2018-09-26T14:07:03Z

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

-        // ignore null values
-        if (token == Token.VALUE_NULL) {
+        if (context.externalValueSet()) {
+            inputMap = (Map<String, CompletionInputMetaData>) context.externalValue();


The external value is not always set by another completion mapper so you need to check the type.
I think it would be simpler to just add an input field in the CompletionInputMetaData class and do something like:

if (context.externalValueSet()) { if (context.externalValue().getClass().equals(CompletionInputMetaData.class)) { CompletionInputMetaData inputAndMeta = (CompletionInputMetaData) context.externalValue(); inputMap = Collections.singletonMap(inputAndMeta.input(), inputAndMeta); } else { inputMap = Collections.singletonMap(context.externalValue().toString(), new CompletionInputMetaData(Collections.<String, Set<CharSequence>>emptyMap(), 1)); }...

well spotted, indeed missed these scenarios where external value is set by GeoPointFIeldMapper. Will add test cases and cover that.

good suggestion. Will extend CompletionInputMetaData to include inputField and return that as a toString

jimczi · 2018-09-26T14:11:26Z

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

+        assertSuggestFields(indexableFields.getFields("completion"), 1);
+        assertThat(indexableFields.getFields("completion.analyzed"), arrayWithSize(2));
+    }
+


We need a test with a context completion suggester as well.

true, will add additional test cases to cover that

pgomulka · 2018-09-27T13:01:40Z

retest this please

pgomulka · 2018-10-01T07:49:11Z

@jimczi I have updated the PR, could you please take a look?

jimczi

It looks good @pgomulka . I left some comments regarding the tests.

jimczi · 2018-10-01T07:54:15Z

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

+            inputMap = Collections.singletonMap(inputAndMeta.inputField, inputAndMeta);
+        } else {
+            String fieldName = context.externalValue().toString();
+            inputMap = Collections.singletonMap(fieldName, new CompletionInputMetaData(fieldName, Collections.<String, Set<CharSequence>>emptyMap(), 1));


You can simplify this line with:

inputMap = Collections.singletonMap(fieldName, new CompletionInputMetaData(fieldName, Collections.emptyMap(), 1));

?

absolutely, will simplify another place in that file to use jdk8 type inference as well

jimczi · 2018-10-01T07:55:26Z

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

@@ -570,13 +593,20 @@ private void parse(ParseContext parseContext, Token token, XContentParser parser
    }

    static class CompletionInputMetaData {
+        public final String inputField;


Maybe input, inputField is misleading I think.

good suggestion, this contains the actual value of the input field, so should be more clear

jimczi · 2018-10-01T08:00:04Z

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

+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertSuggestFields(indexableFields.getFields("completion"), 1);
+        assertThat(indexableFields.getFields("completion.analyzed"), arrayWithSize(2));


Can you check that the field has the expected content?:

assertEquals("suggestion", indexableFields.getFields("completion.analyzed")[0].stringValue());

jimczi · 2018-10-01T08:00:33Z

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

+            XContentType.JSON));
+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertThat(indexableFields.getFields("keywordfield"), arrayWithSize(6));


Can you check that the field has the expected content?

jimczi · 2018-10-01T08:00:39Z

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

+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertFieldsOfType(indexableFields.getFields("suggest"), ContextSuggestField.class, 2);
+        assertFieldsOfType(indexableFields.getFields("suggest.subsuggest"), ContextSuggestField.class, 2);


Can you check that the field has the expected content?:

jimczi · 2018-10-01T08:00:43Z

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

+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertFieldsOfType(indexableFields.getFields("suggest"), ContextSuggestField.class, 2);
+        assertFieldsOfType(indexableFields.getFields("suggest.subsuggest"), ContextSuggestField.class, 2);


Can you check that the field has the expected content?:

jimczi · 2018-10-01T08:00:48Z

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

+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertThat(indexableFields.getFields("geofield"), arrayWithSize(2));
+        assertSuggestFields(indexableFields.getFields("geofield.analyzed"), 1);


Can you check that the field has the expected content?:

jimczi · 2018-10-01T08:00:51Z

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

+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertSuggestFields(indexableFields.getFields("completion"), 1);
+        assertThat(indexableFields.getFields("completion.analyzed"), arrayWithSize(1));


Can you check that the field has the expected content?:

jimczi · 2018-10-01T08:00:56Z

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

+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertSuggestFields(indexableFields.getFields("completion"), 2);
+        assertThat(indexableFields.getFields("completion.analyzed"), arrayWithSize(2));


Can you check that the field has the expected content?:

jimczi · 2018-10-01T08:00:59Z

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

+
+        ParseContext.Document indexableFields = parsedDocument.rootDoc();
+        assertSuggestFields(indexableFields.getFields("completion"), 2);
+        assertThat(indexableFields.getFields("completion.analyzed"), arrayWithSize(4));


Can you check that the field has the expected content?:

Mappings with completion type and multi-fields, were not able to index array or object format on completion fields. Only string format was supproted. This is fixed by providing multiField parser with externalValueContext with already parsed object closes elastic#15115

refactor to reuse CompletionInputMetaData in externalValue Context add test cases with context completion

pgomulka · 2018-10-02T12:06:12Z

@jimczi updated the tests and added an additional REST test cover scenarios I couldn't test in a junit test. Can you please have a look?

jimczi

LGTM, thanks @pgomulka

pgomulka · 2018-10-02T12:34:16Z

Thank you @jimczi

pgomulka · 2018-10-02T12:49:16Z

going to back port to v6.5.0

Mappings with completion type and multi-fields, were not able to index array or object format on completion fields. Only string format was supported. This is fixed by providing multiField parser with externalValueContext with already parsed object closes elastic#15115

) Mappings with completion type and multi-fields, were not able to index array or object format on completion fields. Only string format was supported. This is fixed by providing multiField parser with externalValueContext with already parsed object * Adapt test after backport of #34081 closes #15115

Mappings with completion type and multi-fields, were not able to index array or object format on completion fields. Only string format was supported. This is fixed by providing multiField parser with externalValueContext with already parsed object closes #15115

pgomulka requested a review from jimczi September 26, 2018 13:02

pgomulka force-pushed the fix/15115-support-completion-multi-fields branch from 107d745 to 59f7bca Compare September 26, 2018 13:11

jimczi added >enhancement :Search Relevance/Suggesters "Did you mean" and suggestions as you type v7.0.0 v6.5.0 labels Sep 26, 2018

jimczi reviewed Sep 26, 2018

View reviewed changes

pgomulka force-pushed the fix/15115-support-completion-multi-fields branch from 17ad68d to 55a72b5 Compare September 27, 2018 18:05

jimczi reviewed Oct 1, 2018

View reviewed changes

pgomulka added 5 commits October 1, 2018 10:24

cover scenarios where external value is set by other type of mappers

5716451

refactor to reuse CompletionInputMetaData in externalValue Context add test cases with context completion

remove sout

1489285

remove sout

efc0beb

assert about the content of fields

0770db9

pgomulka force-pushed the fix/15115-support-completion-multi-fields branch 5 times, most recently from a795fd7 to 3bf447e Compare October 1, 2018 16:26

add REST testcase to cover context, weight and geofield searches

e683594

pgomulka force-pushed the fix/15115-support-completion-multi-fields branch from 3bf447e to e683594 Compare October 1, 2018 17:35

skip test for versions < master

b3be7e0

pgomulka force-pushed the fix/15115-support-completion-multi-fields branch from 40f868b to b3be7e0 Compare October 2, 2018 09:06

jimczi approved these changes Oct 2, 2018

View reviewed changes

pgomulka merged commit 3f8cc89 into elastic:master Oct 2, 2018

pgomulka added the backport pending label Oct 2, 2018

pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Oct 2, 2018

Adapt test after backport of elastic#34081

d65cf62

pgomulka mentioned this pull request Oct 2, 2018

Back porting Completion types with multi-fields support (#34081) #34228

Merged

jimczi removed the backport pending label Jan 31, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

jtibshirani mentioned this pull request May 1, 2020

Remove support for 'external values' in document parsing? #56063

Open

5 tasks

jtibshirani mentioned this pull request Apr 26, 2021

Remove 'external values', and replace with swapped out XContentParsers #72203

Merged

Completion types with multi-fields support #34081

Completion types with multi-fields support #34081

Uh oh!

Conversation

pgomulka commented Sep 26, 2018

Uh oh!

elasticmachine commented Sep 26, 2018

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Sep 27, 2018

Uh oh!

pgomulka commented Oct 1, 2018

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Oct 2, 2018

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Oct 2, 2018

Uh oh!

pgomulka commented Oct 2, 2018

Uh oh!

Uh oh!

pgomulka Sep 27, 2018 •

edited

Loading