Add generic fallback implementation for synthetic source #108222

lkts · 2024-05-02T22:22:29Z

This PR uses infrastructure from #107567 to implement a fallback implementation of synthetic source for field mappers that don't support it natively. In that case we will store source of such field as is in a separate stored field.

Pending work:

Tests for more fields that now support synthetic source
There are differences between native synthetic source that is usually generated using doc values and fallback. The fallback is "too precise", f.e. if field value is an empty array, there is no way to know that when using doc values. When source is stored as is, it will be precisely an empty array. That leads to assumptions done in synthetic source tests not matching reality. We may want to create a different set of tests or try to replicate the behavior.
Unification of copy_to handling. copy_to is currently not allowed on fields when synthetic source is used, should this be applied uniformly?
Documentation

Note that this PR does not cover nested as it is not a FieldMapper however it can be done using similar approach separately.

elasticsearchmachine · 2024-05-02T22:22:53Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

elasticsearchmachine · 2024-05-02T22:23:16Z

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

lkts · 2024-05-02T22:27:32Z

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

+     */
+    @Override
+    public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() {
+        return SourceLoader.SyntheticFieldLoader.NOTHING;


This is nothing because ObjectMapper already nicely adds all ignored source fields to the source.

lkts · 2024-05-02T22:29:20Z

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

@@ -219,7 +229,138 @@ protected Object generateRandomInputValue(MappedFieldType ft) {

    @Override
    protected SyntheticSourceSupport syntheticSourceSupport(boolean ignoreMalformed) {


I started adding tests here because it looks like the most complex type. I am not sure how should we proceed - if this PR contains tests for all fields that now have support, it will be quite big.

lkts · 2024-05-02T22:30:15Z

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

+
+    @Override
+    protected BlockReaderSupport getSupportedReaders(MapperService mapper, String loaderFieldName) {
+        // TODO are we okay that we don't support synthetic source here?


My understanding of block loader stuff is still quite weak, i am not sure what's expected here.

lkts · 2024-05-02T22:30:41Z

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

+    protected boolean supportsCopyTo() {
+        // TODO this is so that `testSyntheticSourceInvalid` does not complain
+        // in theory geo_shape could support copy_to
+        // should we fail to construct synthetic source in `FieldMapper` if there is copy_to ?


Another question here.

lkts · 2024-05-03T20:50:13Z

Here is another interesting observation. Currently geo_shape mapper (all of them) defines a block loader that only works with _source. It does not look like that infrastructure supports synthetic source to me. So i think that means that while we can "seamlessly" support synthetic source for geo_shape we will end up breaking ESQL.

lkts · 2024-05-03T20:51:56Z

I am thinking we should opt-in into this one field at a time by setting supportsSyntheticSourceNatively (maybe should rename this too).

martijnvg · 2024-05-06T14:18:00Z

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

+     * If a mapper opts in to use fallback synthetic source implementation.
+     * @return true or false
+     */
+    protected boolean fallbackSyntheticSourceOptIn() {


If implementations overwrite this method, why would these implementations implement synthetic source natively? It is the same as supportsSyntheticSourceNatively() returning false?

Maybe we can just have supportsSyntheticSourceNatively()?

This is a temporary flag just to be able to introduce support on a per-field basis instead of one big PR. We will remove this when we enable synthetic source support for the last field.

server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java

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

lkts · 2024-05-13T18:46:35Z

@elasticmachine update branch

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

server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java

kkrik-es · 2024-05-20T12:01:10Z

...ras/src/yamlRestTest/resources/rest-api-spec/test/search-as-you-type/30_synthetic_source.yml

+        index: test
+        id: "2"
+
+  - match: { _source.a_field: null }


Add an array test too?

Great catch! I for some reason assumed that this field does not support arrays.

I'll actually do that in a small follow-up.

kkrik-es · 2024-05-20T12:06:29Z

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

@@ -54,6 +54,11 @@ public class IgnoredSourceFieldMapper extends MetadataFieldMapper {
     *  - the value, encoded as a byte array
     */
    public record NameValue(String name, int parentOffset, BytesRef value) {
+        public static NameValue fromContext(DocumentParserContext context, String name, BytesRef value) {


This only applies to fields, not objects.. Maybe rename to fromContextField or something like this, and add a short comment describing what it does.

I am not sure i understand why would it not work for objects. I'll rename in a follow-up.

kkrik-es · 2024-05-20T12:14:03Z

Looks good. I'm not sure if array fields are covered too, maybe add a few tests for it. It's fine to deal with them in a follow-up if needed, though.

lkts · 2024-05-21T16:37:29Z

@elasticmachine update branch

This PR extends #108222 adding support for arrays.

Add generic fallback implementation for synthetic source

00b35b0

lkts added >feature :StorageEngine/Mapping The storage related side of mappings labels May 2, 2024

elasticsearchmachine added v8.15.0 Team:StorageEngine labels May 2, 2024

Update docs/changelog/108222.yaml

c6115cd

lkts commented May 2, 2024

View reviewed changes

Fix tests

72589fb

lkts requested a review from a team as a code owner May 3, 2024 22:00

Add opt-in and opt in an easier field

d14b959

lkts force-pushed the feature/synthetic_source_catch_all branch from cb1626e to d14b959 Compare May 3, 2024 22:02

lkts requested review from martijnvg and kkrik-es and removed request for a team May 3, 2024 22:02

martijnvg reviewed May 6, 2024

View reviewed changes

kkrik-es reviewed May 8, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java Outdated Show resolved Hide resolved

kkrik-es reviewed May 8, 2024

View reviewed changes

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

elasticmachine and others added 3 commits May 13, 2024 19:46

Merge branch 'main' into feature/synthetic_source_catch_all

9c5458c

iter

01c791e

revert comment

6b3b1cb

martijnvg reviewed May 14, 2024

View reviewed changes

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

server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java Outdated Show resolved Hide resolved

iter

6653af5

lkts added 7 commits May 14, 2024 12:15

style

80e94b5

Add rest tests

88a6bd4

Handle copy_to

1731429

Docs

5d4d8a2

Merge branch 'main' into feature/synthetic_source_catch_all

2c67a03

iter

6b7ec93

Fix test

01f260a

lkts requested review from martijnvg and kkrik-es May 17, 2024 20:17

kkrik-es reviewed May 20, 2024

View reviewed changes

kkrik-es approved these changes May 20, 2024

View reviewed changes

Merge branch 'main' into feature/synthetic_source_catch_all

0ed36ed

lkts merged commit 91d502c into elastic:main May 21, 2024
15 checks passed

lkts deleted the feature/synthetic_source_catch_all branch May 21, 2024 18:30

This was referenced May 21, 2024

Support arrays in fallback synthetic source implementation #108878

Merged

Support synthetic source for all field types #106460

Closed

lkts added a commit that referenced this pull request May 23, 2024

Support arrays in fallback synthetic source implementation (#108878)

1ec137d

This PR extends #108222 adding support for arrays.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generic fallback implementation for synthetic source #108222

Add generic fallback implementation for synthetic source #108222

lkts commented May 2, 2024 •

edited

Loading

elasticsearchmachine commented May 2, 2024

elasticsearchmachine commented May 2, 2024

lkts May 2, 2024 •

edited

Loading

lkts May 2, 2024

lkts May 2, 2024

lkts May 2, 2024 •

edited

Loading

lkts commented May 3, 2024

lkts commented May 3, 2024

martijnvg May 6, 2024

lkts May 13, 2024

lkts commented May 13, 2024

kkrik-es May 20, 2024

lkts May 21, 2024

lkts May 21, 2024

kkrik-es May 20, 2024

lkts May 21, 2024 •

edited

Loading

kkrik-es commented May 20, 2024

lkts commented May 21, 2024

		@@ -219,7 +229,138 @@ protected Object generateRandomInputValue(MappedFieldType ft) {

		@Override
		protected SyntheticSourceSupport syntheticSourceSupport(boolean ignoreMalformed) {

Add generic fallback implementation for synthetic source #108222

Add generic fallback implementation for synthetic source #108222

Conversation

lkts commented May 2, 2024 • edited Loading

elasticsearchmachine commented May 2, 2024

elasticsearchmachine commented May 2, 2024

lkts May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkts May 2, 2024 • edited Loading

Choose a reason for hiding this comment

lkts commented May 3, 2024

lkts commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkts commented May 13, 2024

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

lkts May 21, 2024 • edited Loading

Choose a reason for hiding this comment

kkrik-es commented May 20, 2024

lkts commented May 21, 2024

lkts commented May 2, 2024 •

edited

Loading

lkts May 2, 2024 •

edited

Loading

lkts May 2, 2024 •

edited

Loading

lkts May 21, 2024 •

edited

Loading