Add a simple 'fetch fields' phase. by jtibshirani · Pull Request #55639 · elastic/elasticsearch

jtibshirani · 2020-04-23T03:28:15Z

Note: the PR is opened against the field-retrieval feature branch.

Currently the phase just looks up each field name in the _source and returns its values in the 'fields' section of the response. This PR just lays out the initial class structure and tests -- much of the interesting functionality will come in subsequent PRs.

Known issues that will be addressed in future commits:

Fields like geopoints whose value can be an array are not handled correctly. Without consulting the mapping, it's not possible to know if it's a multi-valued field, or a single field with an array value.
We take a very simple approach to loading the values from _source. I've had discussions with the scripting team about a new SourceLookup method they have planned to efficiently load particular values: Partially parse source documents to speed up source access #52591. I plan to make use of this when it's available, but for now added a placeholder method extractValues.

Relates to #55363.

jtibshirani · 2020-04-23T03:44:36Z

rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml

We'll no longer use a flat list of fields once we introduce support for formatting the values.

You just have this temporarily because it is simpler to parse?

jtibshirani · 2020-04-23T03:55:50Z

server/src/test/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhaseTests.java

I tried hard to write proper unit tests (in the style of FetchSourcePhaseTests), but it's currently challenging to stub out the required pieces like source lookup. I think some broader refactors are necessary before it's possible to unit test these classes.

What if you made something like:

public void hitsExecute(Function<SearchHit, Map<String, Object>> sourceLookup, SearchHit[] hits)

and unit tested that? Then and the only thing missing a unit test would be the map from search context to "simple" classes?

I had been hesitant to do this, because I thought it would make FetchFieldsPhase harder to read... but I actually think it turned out pretty cleanly. I don't think I followed your suggestion exactly though, curious to hear what you think.

Currently the phase just looks up each field name in the _source and returns its values in the 'fields' section of the response. There are several aspects that need improvement -- this PR just lays out the initial class structure and tests.

elasticmachine · 2020-04-23T04:35:03Z

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

nik9000 · 2020-04-23T16:34:42Z

* We take a very simple approach to loading the values from _source. I've had discussions with the scripting team about a new `SourceLookup` method they have planned to efficiently load particular values: #52591. I plan to make use of this when it's available, but for now added a placeholder method `SourceLookup#extractValues`.

👍

nik9000

Looks like a great start! I left some "for later" stuff and some "maybe now" stuff.

nik9000 · 2020-04-23T16:38:49Z

rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml

You just have this temporarily because it is simpler to parse?

nik9000 · 2020-04-23T16:39:52Z

server/src/main/java/org/elasticsearch/common/document/DocumentField.java

-            // all.
+            // This call doesn't really need to support writing any kind of object, since the values
+            // here are always serializable to xContent. Each value could be a leaf types like a string,
+            // number, or boolean, a list of such values, or a map of such values with string keys.


Ah! Except it won't be for stuff like geo_points, right?

I do wonder if it'd be nicer to have the "type" of the field available so we don't need all the nasty reflection in value.

But that is a problem for another time, I think.

I agree, this is a really unfortunate part of the current PR. It's on the top of my mind in terms of what needs figuring out for a follow-up.

nik9000 · 2020-04-23T16:41:01Z

server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

        trackTotalHitsUpTo = in.readOptionalInt();
+
+        if (in.getVersion().onOrAfter(Version.V_8_0_0)) {
+            if (in.readBoolean()) {


readOptionalStringList?

nik9000 · 2020-04-23T16:41:12Z

server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

        out.writeOptionalInt(trackTotalHitsUpTo);
+
+        if (out.getVersion().onOrAfter(Version.V_8_0_0)) {
+            out.writeBoolean(fetchFields != null);


writeOptionalStringCollection?

nik9000 · 2020-04-23T16:46:31Z

server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

                    while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
                        docValueFields.add(FieldAndFormat.fromXContent(parser));
                    }
+                } else if (FETCH_FIELDS_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {


I wonder when it'll be time to convert this to ObjectParser.....

nik9000 · 2020-04-23T16:50:20Z

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

+                Object value = entry.getValue();
+                List<Object> values = value instanceof List
+                    ? (List<Object>) value
+                    : List.of(value);


I could see wanting to switch this to something a little less instanceof soon. But this'll get the job done for now!

nik9000 · 2020-04-23T16:50:53Z

server/src/main/java/org/elasticsearch/search/internal/SearchContext.java


    public abstract SearchContext docValuesContext(FetchDocValuesContext docValuesContext);

+    public abstract FetchFieldsContext fetchFieldsContext();


I think we should have javadoc for this.

nik9000 · 2020-04-23T16:54:05Z

server/src/main/java/org/elasticsearch/search/lookup/SourceLookup.java

+    /**
+     * For each of the provided paths, return its value in the source. Note that
+     */
+    public Map<String, Object> extractValues(Collection<String> paths) {


I wonder if this'd be better kept in the new fetch phase, especially if we're going to migrate away from it.

I wonder if a better interface for collecting the values is Map<String, Consumer<?>> or something shaped like that. I feel like it'd let you build flexible casting and formatting pretty naturally. But that can wait too.

+1, I'll move this up into FetchFieldsPhase. I'll wait to refine the interface until I have a better handle on the approach to value formatting.

nik9000 · 2020-04-23T16:54:58Z

server/src/test/java/org/elasticsearch/search/SearchHitTests.java

        SearchHit searchHit = createTestItem(xContentType, true, true);
        BytesReference originalBytes = toXContent(searchHit, xContentType, true);
-        Predicate<String> pathsToExclude = path -> (path.endsWith("highlight") || path.endsWith("fields") || path.contains("_source")
+        Predicate<String> pathsToExclude = path -> (path.endsWith("highlight") || path.contains("fields") || path.contains("_source")


👍

This on looks like it took a while to find!

nik9000 · 2020-04-23T16:57:55Z

server/src/test/java/org/elasticsearch/search/fetch/subphase/FetchFieldsPhaseTests.java

What if you made something like:

public void hitsExecute(Function<SearchHit, Map<String, Object>> sourceLookup, SearchHit[] hits)

and unit tested that? Then and the only thing missing a unit test would be the map from search context to "simple" classes?

nik9000 · 2020-04-23T16:59:45Z

Looks like a great start! I left some "for later" stuff and some "maybe now" stuff.

And none of it really blocks merging, especially to a feature branch. But I do think now would be a simpler time to talk about the unit test and where to put the source extraction logic.

jtibshirani · 2020-04-23T21:41:06Z

Thanks @nik9000 for the speedy review, it's ready for another look.

nik9000

LGTM.

nik9000 · 2020-04-24T14:05:29Z

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

+/**
+ * The context needed to retrieve fields.
+ */
+public class FetchFieldsContext {


Fair enough!

Currently the phase just looks up each field name in the _source and returns its values in the 'fields' section of the response. There are several aspects that need improvement -- this PR just lays out the initial class structure and tests.

jtibshirani force-pushed the fetch-fields-phase branch from 10d498d to 358ade3 Compare April 23, 2020 03:33

jtibshirani commented Apr 23, 2020

View reviewed changes

Add a simple 'fetch fields' phase.

88d42d8

Currently the phase just looks up each field name in the _source and returns its values in the 'fields' section of the response. There are several aspects that need improvement -- this PR just lays out the initial class structure and tests.

jtibshirani added the WIP label Apr 23, 2020

jtibshirani added 3 commits April 22, 2020 21:24

Test that DocumentField can handle maps and lists.

9583e1a

Ensure we don't return values for object fields.

e6c86a7

Resolve field names only one time for all hits.

facc1cc

jtibshirani force-pushed the fetch-fields-phase branch from 358ade3 to facc1cc Compare April 23, 2020 04:26

jtibshirani added :Search/Search Search-related issues that do not fall into other categories >feature and removed WIP labels Apr 23, 2020

jtibshirani requested a review from nik9000 April 23, 2020 16:19

nik9000 reviewed Apr 23, 2020

View reviewed changes

jtibshirani added 5 commits April 23, 2020 11:08

Use correct stream serialization methods.

6365394

Pull extractValues up into FetchFieldsPhase.

f1f179a

Add javadoc.

cb9c6e0

Bring FetchFieldsPhaseTests closer to a real unit test.

8f71d7b

Fix javadoc.

f64415e

nik9000 approved these changes Apr 24, 2020

View reviewed changes

jtibshirani merged commit 1dcfdcc into elastic:field-retrieval Apr 24, 2020

jtibshirani deleted the fetch-fields-phase branch April 24, 2020 16:27

jtibshirani mentioned this pull request Apr 24, 2020

Search 'fields' option design + implementation #55363

Closed

10 tasks

jtibshirani mentioned this pull request Apr 24, 2020

Make the fetch fields phase easier to test. #55756

Merged


		public abstract SearchContext docValuesContext(FetchDocValuesContext docValuesContext);

		public abstract FetchFieldsContext fetchFieldsContext();

Conversation

jtibshirani commented Apr 23, 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 23, 2020

Uh oh!

nik9000 commented Apr 23, 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!

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!

nik9000 commented Apr 23, 2020

Uh oh!

jtibshirani commented Apr 23, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jtibshirani commented Apr 23, 2020 •

edited

Loading

jtibshirani Apr 23, 2020 •

edited

Loading

jtibshirani Apr 23, 2020 •

edited

Loading