Correct how field retrieval handles multifields and copy_to. #61309

jtibshirani · 2020-08-19T05:34:54Z

Before when a value was copied to a field through a parent field or copy_to,
we parsed it using the FieldMapper from the source field. Instead we should
parse it using the target FieldMapper. This ensures that we apply the
appropriate mapping type and options to the copied value.

To implement the fix cleanly, this PR refactors the value parsing strategy. Now
instead of looking up values directly, field mappers produce a helper object
ValueFetcher. The value fetchers are responsible for almost all aspects of
fetching, including looking up the right paths in the _source.

The PR is fairly big but each commit can be reviewed individually.

Fixes #61033.

jtibshirani · 2020-08-19T05:43:26Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldValueRetriever.java

+                    continue;
+                }
+
+                if (mapper instanceof FieldAliasMapper) {


This is an unfortunate workaround. I think we should consider moving value fetching support from FieldMapper to MappedFieldType in a follow-up. That would clean this up, because when looking up a field's MappedFieldType we will already resolve aliases to their targets (and can avoid doing it manually here).

Before these cases asserted the wrong result. Now they capture the right behavior, but currently fail.

This will help to implement the fix in a clean way.

Before when a value was copied to a field through a parent field or copy_to, we parsed it using the FieldMapper from the source field. Instead we should parse it using the target FieldMapper. This fix works but is a bit messy. The next commit will clean up the approach.

Now SourceValueFetcher owns all aspects of looking up values from _source, instead of requiring some information to be passed in externally.

This is no longer needed after the refactor to use ValueFetcher.

elasticmachine · 2020-08-19T19:09:37Z

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

nik9000

LGTM

nik9000 · 2020-08-19T19:33:38Z

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

+                return List.of();
+            }
+
+            if (parsesArrayValue) {


I think it'd be cleaner if this were handled by a differently named subclass. I know we have the boolean in the mapper, but it just feels funny to pass a boolean swapping out the guts of the implementation.

It definitely seems cleaner in the abstract, but this approach matches the logic DocumentParser where we check FieldMapper#parsesArrayValue to decide what value to pass. I also think using parsesArrayValue in both places encourages field mappers to have a consistent approach between parseCreateField and parseSourceValue.

I agree this is not ideal, I really wish there were a solid way to tie together the index-time parsing and value fetching logic...

nik9000 · 2020-08-19T19:34:21Z

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

+     * format. This parsing logic should closely mirror the value parsing in
+     * {@link FieldMapper#parseCreateField} or {@link FieldMapper#parse}.
+     */
+    protected abstract Object parseSourceValue(Object value);


I don't really think this is cleaner than passing the CheckedFunction but if you like it better that is cool with me.

I agree, I think passing a function argument is readable too. It seems totally fine if the implementation evolved in that direction.

I had a slight preference for this approach since it associates a name with the function argument: when you look at an implementation like KeywordFieldMapper#valueFetcher, the purpose of the function is very obvious.

nik9000 · 2020-08-19T19:35:05Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/FieldValueRetriever.java

+                    continue;
+                }
+
+                if (mapper instanceof FieldAliasMapper) {


nik9000 · 2020-08-19T19:42:13Z

...src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java

        if (format != null) {
            throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
        }

-        return fieldType().value == null
+        return lookup -> fieldType().value == null


fieldType.value == null ? lookup -> List.of() : lookup -> List.of(fieldType.value)? It isn't really important, but it'd make me feel good.

👍 will update this, feeling good is worth it !

…cher.

jtibshirani · 2020-08-19T23:50:08Z

Thanks @nik9000 for the feedback! I incorporated some of your suggestions -- I'm going to merge but can happily make follow-up changes if you feel strongly about some point.

…#61309) Before when a value was copied to a field through a parent field or `copy_to`, we parsed it using the `FieldMapper` from the source field. Instead we should parse it using the target `FieldMapper`. This ensures that we apply the appropriate mapping type and options to the copied value. To implement the fix cleanly, this PR refactors the value parsing strategy. Now instead of looking up values directly, field mappers produce a helper object `ValueFetcher`. The value fetchers are responsible for almost all aspects of fetching, including looking up the right paths in the _source. The PR is fairly big but each commit can be reviewed individually. Fixes elastic#61033.

jtibshirani added >bug :Search/Search Search-related issues that do not fall into other categories labels Aug 19, 2020

jtibshirani changed the title ~~Correct how 'fields' option handles multifields and copy_to.~~ Correct how field retrieval handles multifields and copy_to. Aug 19, 2020

jtibshirani commented Aug 19, 2020

View reviewed changes

jtibshirani force-pushed the fields-source-path branch from a2b3164 to b8a70be Compare August 19, 2020 17:04

Fix multifield and copy_to cases in FieldValueRetrieverTests.

81b9db2

Before these cases asserted the wrong result. Now they capture the right behavior, but currently fail.

jtibshirani force-pushed the fields-source-path branch from b8a70be to aa87bde Compare August 19, 2020 17:43

jtibshirani added 3 commits August 19, 2020 12:01

Refactor FieldMapper#lookupValues to return a ValueFetcher object.

fc34f4a

This will help to implement the fix in a clean way.

Move source path lookup to SourceValueFetcher.

aacd390

Now SourceValueFetcher owns all aspects of looking up values from _source, instead of requiring some information to be passed in externally.

jtibshirani force-pushed the fields-source-path branch from aa87bde to 49c2c75 Compare August 19, 2020 19:03

Remove FieldMapper#nullValue().

fa66569

This is no longer needed after the refactor to use ValueFetcher.

jtibshirani force-pushed the fields-source-path branch from 49c2c75 to fa66569 Compare August 19, 2020 19:06

jtibshirani added v7.10.0 v8.0.0 labels Aug 19, 2020

jtibshirani marked this pull request as ready for review August 19, 2020 19:09

elasticmachine added the Team:Search Meta label for search team label Aug 19, 2020

jtibshirani requested a review from nik9000 August 19, 2020 19:11

nik9000 approved these changes Aug 19, 2020

View reviewed changes

Only check constant value once in ConstantKeywordFieldMapper#valueFet…

f353567

…cher.

jtibshirani merged commit 5457b34 into elastic:master Aug 19, 2020

jtibshirani deleted the fields-source-path branch August 19, 2020 23:50

jtibshirani added the backport pending label Aug 20, 2020

jtibshirani mentioned this pull request Aug 20, 2020

Correct how field retrieval handles multifields and copy_to. #61391

Merged

jtibshirani removed the backport pending label Sep 4, 2020

jakelandis removed the v8.0.0 label Jul 26, 2021

jakelandis added the v8.0.0-alpha1 label Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Correct how field retrieval handles multifields and copy_to. #61309

Correct how field retrieval handles multifields and copy_to. #61309

Uh oh!

jtibshirani commented Aug 19, 2020 •

edited

Loading

Uh oh!

jtibshirani Aug 19, 2020

Uh oh!

nik9000 Aug 19, 2020

Uh oh!

elasticmachine commented Aug 19, 2020

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 Aug 19, 2020

Uh oh!

jtibshirani Aug 19, 2020

Uh oh!

nik9000 Aug 19, 2020

Uh oh!

jtibshirani Aug 19, 2020

Uh oh!

nik9000 Aug 19, 2020

Uh oh!

nik9000 Aug 19, 2020

Uh oh!

jtibshirani Aug 19, 2020

Uh oh!

jtibshirani commented Aug 19, 2020

Uh oh!

Uh oh!

Correct how field retrieval handles multifields and copy_to. #61309

Correct how field retrieval handles multifields and copy_to. #61309

Uh oh!

Conversation

jtibshirani commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 19, 2020

Uh oh!

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

jtibshirani commented Aug 19, 2020

Uh oh!

Uh oh!

jtibshirani commented Aug 19, 2020 •

edited

Loading