Synthetic _source: support `field` in many cases #89950

nik9000 · 2022-09-08T20:20:00Z

This adds support for the field scripting API in many but not all cases. Before this change numbers, dates, and IPs supported the field API when running with _source in synthetic mode because they always have doc values. This change adds support for match_only_text, stored keyword fields, and stored text fields. Two remaining field configurations work with synthetic _source and do not work with field:

A text field with a sub-keyword field that has doc_values
A text field with a sub-keyword field that is stored

This adds support for the `field` scripting API in many but not all cases. Before this change numbers, dates, and IPs supported the `field` API when running with _source in synthetic mode because they always have doc values. This change adds support for `match_only_text`, `store`d `keyword` fields, and `store`d `text` fields. Two remaining field configurations work with synthetic _source and do not work with `field`: * A `text` field with a sub-`keyword` field that has `doc_values` * A `text` field with a sub-`keyword` field that is `store`d

elasticsearchmachine · 2022-09-09T13:46:40Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine · 2022-09-09T13:47:03Z

Hi @nik9000, I've created a changelog YAML for you.

romseygeek · 2022-09-09T13:47:19Z

server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java

+    public boolean alwaysEmpty() {
+        return sourceProvider.alwaysEmpty();
+    }
+


Can we do this via the FieldDataContext somehow? Putting it on the SourceLookup feels a bit weird to me.

I can move it, but, like, it's the source lookup that is empty. I think maybe moving and renaming it is a good idea. I just don't know the right spot.

nik9000 · 2022-09-09T13:47:54Z

...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java

+                throw new IllegalArgumentException(CONTENT_TYPE + " fields do not support sorting and aggregations");
+            }
+            SourceLookup sourceLookup = fieldDataContext.lookupSupplier().get().source();
+            if (sourceLookup.alwaysEmpty()) {


I've been using alwaysEmpty to signal that you should try synthetic source things. But maybe it'd be better to have it actually have a syntheticSource method instead. Always empty is a fairly broad concept.

I think we could have source providers have a syntheticSource method instead - the only non-test place I believe we explicitly use the "null" source provider is from MappingLookup which checks if we have synthetic source.

elasticsearchmachine · 2022-09-13T19:46:15Z

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

nik9000 · 2022-09-14T19:16:41Z

@romseygeek could you have another look? It's much bigger now....

… synthetic_source_fields_3

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

nik9000

@romseygeek this is ready for you again.

nik9000 · 2022-11-07T11:42:59Z

run
elasticsearch-ci/part-1

nik9000 · 2022-11-07T11:43:05Z

run elasticsearch-ci/part-1

martijnvg

I'm happy with this change 👍 . I didn't do a line by line review, but how it works looks good to me.

romseygeek · 2022-11-08T12:34:35Z

So I know that I asked for the information about synthetic source to be moved to FieldDataContext but I have, annoyingly, changed my mind about that again. FDC holds information at the context in which field data is being asked for, and synthetic source is entirely orthogonal to that. I've opened #91400 to move this to MapperBuilderContext, which I think will fit better here as well - field types can get a constructor parameter telling them if they need to load things from a secret stored field or just use source.

romseygeek

Nearly there, I left a couple of questions.

romseygeek · 2022-11-10T08:27:20Z

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

+            if (hasDocValues()) {
+                return fieldDataFromDocValues();
+            }
+            if (isSyntheticSource && isStored()) {


What happens if synthetic source is enabled and the data isn't stored? As I read it we fall through to using normal source, which will fail a bit unpredictably I think? Is it worth throwing an explicit Exception here in that case?

It isn't possible to configure synthetic _source without doc values or stored fields. I'll add a hard test.

Maybe add an assertion as well so its more obvious when reading the code?

romseygeek · 2022-11-10T08:28:34Z

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

+            if (operation != FielddataOperation.SCRIPT) {
+                throw new IllegalStateException("unknown field data operation [" + operation.name() + "]");
+            }
+            if (isSyntheticSource && isStored()) {


Same question here

This one has a different answer! This will be possible in a follow up but isn't yet. I'll throw a helpful exception here too.

romseygeek · 2022-11-10T08:30:55Z

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

+
+    public void testDocValues() throws IOException {
+        MapperService mapper = createMapperService(fieldMapping(b -> b.field("type", "text")));
+        assertScriptDocValues(mapper, "foo", equalTo(List.of("foo")));


I think it's worth testing that an input that would otherwise be tokenized is returned whole here. Basically test that we get "foo bar" back, rather than a list of "foo" and "bar".

romseygeek · 2022-11-10T08:32:32Z

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

+        assertScriptDocValues(mapper, "foo", equalTo(List.of("foo")));
+    }
+
+    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/86603")


This links to the master synthetic source issue, is there a more specific one that this relates to?

I'll un-awaitsfix it and have it catch the fancy new exception I'm throwing.

romseygeek

LGTM! Thanks for iterating.

nik9000 added >enhancement :StorageEngine/TSDB You know, for Metrics v8.5.0 labels Sep 8, 2022

nik9000 requested review from romseygeek, jdconrad and tmgordeeva September 8, 2022 20:20

nik9000 mentioned this pull request Sep 8, 2022

Synthetic Source #86603

Closed

50 tasks

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 9, 2022

Update docs/changelog/89950.yaml

ab6c34b

romseygeek reviewed Sep 9, 2022

View reviewed changes

nik9000 commented Sep 9, 2022

View reviewed changes

Fixup

1c647c7

nik9000 added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Sep 13, 2022

elasticsearchmachine added the Team:Search Meta label for search team label Sep 13, 2022

Merge branch 'main' into synthetic_source_fields_3

702e91e

nik9000 requested a review from romseygeek September 14, 2022 19:16

nik9000 added 2 commits September 14, 2022 15:29

Oh boy thats a lot of files

bdc3fde

Merge remote-tracking branch 'nik9000/synthetic_source_fields_3' into…

f466e97

… synthetic_source_fields_3

romseygeek reviewed Sep 20, 2022

View reviewed changes

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

nik9000 added 3 commits September 20, 2022 16:42

Merge branch 'main' into synthetic_source_fields_3

9dc7fc0

No param

16a9925

Merge branch 'main' into synthetic_source_fields_3

489b134

nik9000 requested a review from martijnvg September 21, 2022 15:15

test pass plz k thx

1e4e216

csoulios removed the v8.5.0 label Sep 21, 2022

csoulios added the v8.6.0 label Sep 21, 2022

Merge branch 'main' into synthetic_source_fields_3

cc887e3

nik9000 commented Sep 27, 2022

View reviewed changes

nik9000 added 2 commits November 4, 2022 10:38

Merge branch 'main' into synthetic_source_fields_3

d45850e

Docs

1778585

nik9000 requested a review from romseygeek November 7, 2022 15:35

martijnvg approved these changes Nov 8, 2022

View reviewed changes

nik9000 added 2 commits November 9, 2022 10:38

Merge branch 'main' into synthetic_source_fields_3

976b74c

Move

b79b033

romseygeek reviewed Nov 10, 2022

View reviewed changes

fixup

6ca4596

nik9000 requested a review from romseygeek November 10, 2022 14:51

romseygeek approved these changes Nov 10, 2022

View reviewed changes

format

ac7f8a1

nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 10, 2022

elasticsearchmachine merged commit 74d0d19 into elastic:main Nov 10, 2022

nik9000 deleted the synthetic_source_fields_3 branch November 10, 2022 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synthetic _source: support `field` in many cases #89950

Synthetic _source: support `field` in many cases #89950

nik9000 commented Sep 8, 2022

elasticsearchmachine commented Sep 9, 2022

elasticsearchmachine commented Sep 9, 2022

romseygeek Sep 9, 2022

nik9000 Sep 9, 2022

nik9000 Sep 9, 2022

tmgordeeva Sep 13, 2022

elasticsearchmachine commented Sep 13, 2022

nik9000 commented Sep 14, 2022

nik9000 left a comment

nik9000 commented Nov 7, 2022

nik9000 commented Nov 7, 2022

martijnvg left a comment

romseygeek commented Nov 8, 2022

romseygeek left a comment

romseygeek Nov 10, 2022

nik9000 Nov 10, 2022

romseygeek Nov 10, 2022

romseygeek Nov 10, 2022

nik9000 Nov 10, 2022

nik9000 Nov 10, 2022

nik9000 Nov 10, 2022

romseygeek Nov 10, 2022

romseygeek Nov 10, 2022

nik9000 Nov 10, 2022

romseygeek left a comment

Synthetic _source: support field in many cases #89950

Synthetic _source: support field in many cases #89950

Conversation

nik9000 commented Sep 8, 2022

elasticsearchmachine commented Sep 9, 2022

elasticsearchmachine commented Sep 9, 2022

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 Sep 13, 2022

nik9000 commented Sep 14, 2022

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 commented Nov 7, 2022

nik9000 commented Nov 7, 2022

martijnvg left a comment

Choose a reason for hiding this comment

romseygeek commented Nov 8, 2022

romseygeek 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

romseygeek left a comment

Choose a reason for hiding this comment

Synthetic _source: support `field` in many cases #89950

Synthetic _source: support `field` in many cases #89950