Synthetic _source: support `field` in many cases by nik9000 · Pull Request #89950 · elastic/elasticsearch

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 jdconrad, romseygeek 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

Conversation

nik9000 commented Sep 8, 2022

Uh oh!

elasticsearchmachine commented Sep 9, 2022

Uh oh!

elasticsearchmachine commented Sep 9, 2022

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!

elasticsearchmachine commented Sep 13, 2022

Uh oh!

nik9000 commented Sep 14, 2022

Uh oh!

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Nov 7, 2022

Uh oh!

nik9000 commented Nov 7, 2022

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Nov 8, 2022

Uh oh!

romseygeek 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!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants