Implement fields fetch for runtime fields #61995

nik9000 · 2020-09-04T14:05:04Z

This implements the fields API in _search for runtime fields using
doc values. Most of that implementation is stolen from the
docvalue_fields fetch sub-phase, just moved into the same API that the
fields API uses. At this point the docvalue_fields fetch phase looks
like a special case of the fields API.

While I was at it I moved the "which doc values sub-implementation
should I use for fetching?" question from a bunch of instanceofs to a
method on LeafFieldData so we can be much more flexible with what is
returned and we're not forced to extend certain classes just to make the
fetch phase happy.

Relates to #59332

This implements the `fields` API in `_search` for runtime fields using doc values. Most of that implementation is stolen from the `docvalue_fields` fetch sub-phase, just moved into the same API that the `fields` API uses. At this point the `docvalue_fields` fetch phase looks like a special case of the `fields` API. While I was at it I moved the "which doc values sub-implementation should I use for fetching?" question from a bunch of `instanceof`s to a method on `LeafFieldData` so we can be much more flexible with what is returned and we're not forced to extend certain classes just to make the fetch phase happy. Relates to elastic#59332

nik9000 · 2020-09-04T14:08:12Z

server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java

@@ -299,6 +299,18 @@ public SearchLookup lookup() {
        return this.lookup;
    }

+    private SearchLookup fetchLookup = null;
+
+    public SearchLookup fetchLookup() {


I think this is the most controversial part of this change. Previously we were building the SourceLookup in the fetch phase and letting things that needed doc values at fetch time use the standard search lookup. This starts us down a road to removing that. I think we like this because we don't want things like the explain sub-phase to modify the rest of the fetch phases.

Could we just create a SearchLookup within FetchPhase? I think we could store it on HitContext instead of storing SourceLookup directly. That would make its scope and lifetime clearer. It also feels weird to me to put this on QueryShardContext, since it's not meant to be used during the query phase.

Hmmmmm. Let me dig into this again! I did it this way when I was building things really early and needed something in a much more "global" place to do the building.

I was trying not to modify HitContext so much - maybe it can be something we pass in to the phases when we build them. I'll take another look.

We can create SearchLookup just for the FetchPhase. I'll push a change that does that. Sadly, we still have to delegate to QueryShardContext to create it because it is the thing with all of the right dependencies.

I've pushed the change.

I guess I need to read this discussion because I have been asking questions around this as part of my review :)

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java

nik9000 · 2020-09-04T14:10:41Z

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

                }
            }
        };
    }

-    private abstract static class DocValueField {


All of the "brains" of this stuff has moved into the LeafFieldData implementations.

nik9000 · 2020-09-04T14:13:15Z

...lds/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/RuntimeScriptFieldMapper.java

-    public ValueFetcher valueFetcher(MapperService mapperService, String format) {
-        throw new UnsupportedOperationException();
+    public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup lookup, String format) {
+        return new DocValueFetcher(fieldType().docValueFormat(format, null), () -> lookup.doc().getForField(fieldType()));


Given that everything else is in the LeafFieldData implemenations, this is all we need in runtime fields!

elasticmachine · 2020-09-04T14:13:54Z

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

nik9000 · 2020-09-04T14:28:46Z

ooooh! This is broken for inner hits. I'll see what I can find!

nik9000 · 2020-09-04T15:33:49Z

Nested works now! New error having to do with date_nanos. Looks like I screwed something up there. I'll track it down!

nik9000 · 2020-09-04T16:12:14Z

And now data nanos!

jtibshirani

I left a first round of comments. (I'm still thinking through the details around the SearchLookup changes.)

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

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

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

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

jtibshirani

I've now read the full change and left some comments !

jtibshirani · 2020-09-04T21:01:54Z

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

-    public FieldValueRetriever fieldValueRetriever() {
-        return fieldValueRetriever;
+    public FieldValueRetriever fieldValueRetriever(String indexName, MapperService mapperService, SearchLookup searchLookup) {
+        if (mapperService.documentMapper().sourceMapper().enabled() == false) {


Small comment, it feels more natural to do this in FetchFieldsContext rather than this factory method ?

I inherited its location from where it was before - I dunno where it feels better. Would you prefer I move it into the factory method?

Sure if you don't mind I think it's nicer to create it directly in FetchFieldsContext.

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

jtibshirani · 2020-09-08T18:41:57Z

server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java

@@ -299,6 +299,18 @@ public SearchLookup lookup() {
        return this.lookup;
    }

+    private SearchLookup fetchLookup = null;
+
+    public SearchLookup fetchLookup() {


Could we just create a SearchLookup within FetchPhase? I think we could store it on HitContext instead of storing SourceLookup directly. That would make its scope and lifetime clearer. It also feels weird to me to put this on QueryShardContext, since it's not meant to be used during the query phase.

jtibshirani · 2020-09-08T18:53:44Z

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

@@ -42,5 +44,10 @@
     * @param lookup a lookup structure over the document's source.
     * @return a list a standardized field values.
     */
-    List<Object> fetchValues(SourceLookup lookup);
+    List<Object> fetchValues(SourceLookup lookup) throws IOException;


I'm not sure about this idea, but it feels natural to only pass the doc ID here:

List<Object> fetchValues(SourceLookup lookup) throws IOException;

Then SourceValueFetcher could hang onto the SearchLookup and use that to access SourceLookup ?

I was thinking about that too! I didn't want to do it as part if this PR because it'd make it huge, but I think its a good idea.

Hmm - this doesn't look like it'll make the PR much bigger. I'll do that.

I've pushed a refactor for this too.

This reverts commit 4e5bd65.

nik9000 · 2020-09-10T20:15:42Z

@jtibshirani and I talked about this some over video today and settled on two things:

Revert the change from SourceLookup to docId being passed to the DocValueFetcher. We're not 100% sure it makes sense yet.
Rename newSearchLookup on QueryShardContext to newFetchLookup to make it clear that it is just for the fetch phase. We think we should build a more fetch-phase-specific version of SearchLookup, but that that effort should wait.

nik9000 · 2020-09-14T13:23:24Z

@jtibshirani and I talked about this some over video today and settled on two things:

And I pushed changes to do those two things.

jtibshirani

This looks good to me, thanks again for the many iterations!

For context, @nik9000 and I decided that this approach is best for the initial PR, but the SearchLookup sharing logic is still a bit confusing. We're thinking through some follow-up refactors to help solidify + clarify it.

jtibshirani · 2020-09-14T18:25:33Z

I just realized that we should also remove the REST test exclusions in runtime-fields/qa/rest/build.gradle:

'search/330_fetch_fields/*', // The 'fields' option is not yet supported
'search/110_field_collapsing/field collapsing, inner_hits, and fields', // Also fails because of the 'fields' option

nik9000 · 2020-09-14T18:43:34Z

I just realized that we should also remove the REST test exclusions in runtime-fields/qa/rest/build.gradle:

Oh! I thought I had. I'll do it before merging. Thanks!

javanna

LGTM

javanna · 2020-09-15T09:39:25Z

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

+    @Override
+    public List<Object> fetchValues(int docId) throws IOException {
+        if (false == leaf.advanceExact(docId)) {
+            return List.of();


javanna · 2020-09-15T09:40:37Z

server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java

        }
        return this.lookup;
    }

+    /**
+     * Build a lookup customized for the fetch phase. Use {@link #lookup()}
+     * in other phases.


shall we open an issue to track finding an alternative for this?

It looks like @romseygeek is working on this now.

This implements the `fields` API in `_search` for runtime fields using doc values. Most of that implementation is stolen from the `docvalue_fields` fetch sub-phase, just moved into the same API that the `fields` API uses. At this point the `docvalue_fields` fetch phase looks like a special case of the `fields` API. While I was at it I moved the "which doc values sub-implementation should I use for fetching?" question from a bunch of `instanceof`s to a method on `LeafFieldData` so we can be much more flexible with what is returned and we're not forced to extend certain classes just to make the fetch phase happy. Relates to elastic#59332

In elastic#61995 I moved the `docvalue_field` fetch code into a place where I could share it with the fancy new `fields` fetch API. Specifically, runtime fields can use it all that doc values code now. But I broke `scaled_floats` by switching them how they are fetched from `double` to `string`. This adds the override you need to switch them back.

This implements the `fields` API in `_search` for runtime fields using doc values. Most of that implementation is stolen from the `docvalue_fields` fetch sub-phase, just moved into the same API that the `fields` API uses. At this point the `docvalue_fields` fetch phase looks like a special case of the `fields` API. While I was at it I moved the "which doc values sub-implementation should I use for fetching?" question from a bunch of `instanceof`s to a method on `LeafFieldData` so we can be much more flexible with what is returned and we're not forced to extend certain classes just to make the fetch phase happy. Relates to #59332

In #61995 I moved the `docvalue_field` fetch code into a place where I could share it with the fancy new `fields` fetch API. Specifically, runtime fields can use it all that doc values code now. But I broke `scaled_floats` by switching them how they are fetched from `double` to `string`. This adds the override you need to switch them back.

nik9000 added 2 commits September 4, 2020 10:02

Doc

9be09d7

nik9000 commented Sep 4, 2020

View reviewed changes

nik9000 marked this pull request as ready for review September 4, 2020 14:13

nik9000 requested review from jtibshirani and javanna September 4, 2020 14:13

nik9000 added :Search/Search Search-related issues that do not fall into other categories v7.10.0 v8.0.0 labels Sep 4, 2020

elasticmachine added the Team:Search Meta label for search team label Sep 4, 2020

nik9000 added >enhancement and removed Team:Search Meta label for search team labels Sep 4, 2020

elasticmachine added the Team:Search Meta label for search team label Sep 4, 2020

Merge branch 'master' into runtime_fields_fetch_fields_take_two

186a911

Use clean nested context

9b4b344

Date nanos!

9565d7d

Update test now that we force the right resolution in the formatter

ef3b2a4

jtibshirani reviewed Sep 4, 2020

View reviewed changes

Delay building all fetchers

af9da06

jtibshirani reviewed Sep 4, 2020

View reviewed changes

server/src/main/java/org/elasticsearch/search/fetch/subphase/FetchFieldsContext.java Outdated Show resolved Hide resolved

nik9000 requested a review from jtibshirani September 4, 2020 20:52

nik9000 added 2 commits September 4, 2020 16:53

Fixup

56405f5

Map

e494412

jtibshirani reviewed Sep 8, 2020

View reviewed changes

nik9000 added 2 commits September 8, 2020 17:27

Merge branch 'master' into runtime_fields_fetch_fields_take_two

7099198

Pass lookup when building

6313a12

nik9000 added 6 commits September 10, 2020 14:32

Merge branch 'master' into runtime_fields_fetch_fields_take_two

d7315fe

Revert "Close over lookup"

f92f8a5

This reverts commit 4e5bd65.

Fetch lookup

122663c

Rename k plz

77f130c

Merge branch 'master' into runtime_fields_fetch_fields_take_two

0fe6b3d

Compile plz

3790f5e

nik9000 requested a review from jtibshirani September 10, 2020 20:11

jtibshirani approved these changes Sep 14, 2020

View reviewed changes

javanna approved these changes Sep 15, 2020

View reviewed changes

javanna mentioned this pull request Sep 15, 2020

Introduce FetchContext #62357

Merged

nik9000 added 5 commits September 15, 2020 12:23

Merge branch 'master' into runtime_fields_fetch_fields_take_two

459808f

Compile

2055c32

Iter

d9e6209

oops

137ac90

Merge branch 'master' into runtime_fields_fetch_fields_take_two

d666f3f

nik9000 merged commit 9a127ad into elastic:master Sep 15, 2020

nik9000 mentioned this pull request Sep 15, 2020

Fix docvalue fetch for scaled floats #62425

Merged

javanna added >non-issue and removed >enhancement labels Oct 7, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fields fetch for runtime fields #61995

Implement fields fetch for runtime fields #61995

nik9000 commented Sep 4, 2020

nik9000 Sep 4, 2020

jtibshirani Sep 8, 2020

nik9000 Sep 8, 2020

nik9000 Sep 8, 2020

nik9000 Sep 9, 2020

javanna Sep 9, 2020

nik9000 Sep 4, 2020

nik9000 Sep 4, 2020

elasticmachine commented Sep 4, 2020

nik9000 commented Sep 4, 2020

nik9000 commented Sep 4, 2020

nik9000 commented Sep 4, 2020

jtibshirani left a comment

jtibshirani left a comment

jtibshirani Sep 4, 2020

nik9000 Sep 8, 2020

jtibshirani Sep 9, 2020

jtibshirani Sep 8, 2020

jtibshirani Sep 8, 2020

nik9000 Sep 8, 2020

nik9000 Sep 8, 2020

nik9000 Sep 9, 2020

nik9000 commented Sep 10, 2020

nik9000 commented Sep 14, 2020

jtibshirani left a comment

jtibshirani commented Sep 14, 2020

nik9000 commented Sep 14, 2020

javanna left a comment

javanna Sep 15, 2020

javanna Sep 15, 2020

nik9000 Sep 15, 2020

Implement fields fetch for runtime fields #61995

Implement fields fetch for runtime fields #61995

Conversation

nik9000 commented Sep 4, 2020

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

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

elasticmachine commented Sep 4, 2020

nik9000 commented Sep 4, 2020

nik9000 commented Sep 4, 2020

nik9000 commented Sep 4, 2020

jtibshirani left a comment

Choose a reason for hiding this comment

jtibshirani left a comment

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

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

Choose a reason for hiding this comment

nik9000 commented Sep 10, 2020

nik9000 commented Sep 14, 2020

jtibshirani left a comment

Choose a reason for hiding this comment

jtibshirani commented Sep 14, 2020

nik9000 commented Sep 14, 2020

javanna left a comment

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