Correct how field retrieval handles multifields and copy_to.#61309
Correct how field retrieval handles multifields and copy_to.#61309jtibshirani merged 6 commits intoelastic:masterfrom
Conversation
There was a problem hiding this comment.
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).
a2b3164 to
b8a70be
Compare
Before these cases asserted the wrong result. Now they capture the right behavior, but currently fail.
b8a70be to
aa87bde
Compare
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.
aa87bde to
49c2c75
Compare
This is no longer needed after the refactor to use ValueFetcher.
49c2c75 to
fa66569
Compare
|
Pinging @elastic/es-search (:Search/Search) |
| return List.of(); | ||
| } | ||
|
|
||
| if (parsesArrayValue) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
| * format. This parsing logic should closely mirror the value parsing in | ||
| * {@link FieldMapper#parseCreateField} or {@link FieldMapper#parse}. | ||
| */ | ||
| protected abstract Object parseSourceValue(Object value); |
There was a problem hiding this comment.
I don't really think this is cleaner than passing the CheckedFunction but if you like it better that is cool with me.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| return fieldType().value == null | ||
| return lookup -> fieldType().value == null |
There was a problem hiding this comment.
fieldType.value == null ? lookup -> List.of() : lookup -> List.of(fieldType.value)? It isn't really important, but it'd make me feel good.
There was a problem hiding this comment.
👍 will update this, feeling good is worth it !
|
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.
Before when a value was copied to a field through a parent field or
copy_to,we parsed it using the
FieldMapperfrom the source field. Instead we shouldparse it using the target
FieldMapper. This ensures that we apply theappropriate 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 offetching, including looking up the right paths in the _source.
The PR is fairly big but each commit can be reviewed individually.
Fixes #61033.