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

martijnvg · 2024-05-14T09:10:27Z

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

+ *
+ * @return {@link SyntheticSourceMode}
+ */
+ protected SyntheticSourceMode syntheticSourceMode() {


This will be temporary until all fields support synthetic source natively or via fallback mechanism? If so maybe include this in the java docs here.

This method will remain but maybe FALLBACK variant will be removed once we apply fallback everywhere. Just for context - we need this separate piece of information because syntheticFieldLoader throws exceptions and we can't call it to determine if mapper supports synthetic source natively. If we do that, that exception will be wrapped in document parsing exception which is different from how it's currently thrown.

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

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

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

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

elasticsearchmachine commented May 2, 2024

elasticsearchmachine commented May 2, 2024

lkts May 2, 2024 •

edited

lkts May 2, 2024

lkts May 2, 2024

lkts May 2, 2024 •

edited

lkts commented May 3, 2024

lkts commented May 3, 2024

martijnvg May 6, 2024

lkts May 13, 2024

lkts commented May 13, 2024

martijnvg May 14, 2024

lkts May 14, 2024

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

		@Override
		protected SyntheticSourceSupport syntheticSourceSupport(boolean ignoreMalformed) {

Add generic fallback implementation for synthetic source #108222

Are you sure you want to change the base?

Add generic fallback implementation for synthetic source #108222

Conversation

lkts commented May 2, 2024 • edited

elasticsearchmachine commented May 2, 2024

elasticsearchmachine commented May 2, 2024

lkts May 2, 2024 • edited

Choose a reason for hiding this comment

lkts May 2, 2024

Choose a reason for hiding this comment

lkts May 2, 2024

Choose a reason for hiding this comment

lkts May 2, 2024 • edited

Choose a reason for hiding this comment

lkts commented May 3, 2024

lkts commented May 3, 2024

martijnvg May 6, 2024

Choose a reason for hiding this comment

lkts May 13, 2024

Choose a reason for hiding this comment

lkts commented May 13, 2024

martijnvg May 14, 2024

Choose a reason for hiding this comment

lkts May 14, 2024

Choose a reason for hiding this comment

lkts commented May 2, 2024 •

edited

lkts May 2, 2024 •

edited

lkts May 2, 2024 •

edited