Add a simple 'fetch fields' phase. #55639

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

+      search:
+        index: test
+        body:
+          fields: [keyword, integer_range]


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

+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasItems;
+
+public class FetchFieldsPhaseTests extends ESSingleNodeTestCase {


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

+      search:
+        index: test
+        body:
+          fields: [keyword, integer_range]


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

@@ -244,6 +247,12 @@ public SearchSourceBuilder(StreamInput in) throws IOException {
        sliceBuilder = in.readOptionalWriteable(SliceBuilder::new);
        collapse = in.readOptionalWriteable(CollapseBuilder::new);
        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

@@ -298,6 +307,13 @@ public void writeTo(StreamOutput out) throws IOException {
        out.writeOptionalWriteable(sliceBuilder);
        out.writeOptionalWriteable(collapse);
        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

@@ -1120,6 +1154,11 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th
                    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

@@ -201,6 +202,10 @@ public InnerHitsContext innerHits() {

    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

@@ -188,7 +188,7 @@ public void testFromXContentLenientParsing() throws IOException {
        XContentType xContentType = randomFrom(XContentType.values());
        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

+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasItems;
+
+public class FetchFieldsPhaseTests extends ESSingleNodeTestCase {


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a simple 'fetch fields' phase. #55639

Add a simple 'fetch fields' phase. #55639

jtibshirani commented Apr 23, 2020 •

edited

Loading

jtibshirani Apr 23, 2020

nik9000 Apr 23, 2020

jtibshirani Apr 23, 2020 •

edited

Loading

nik9000 Apr 23, 2020

jtibshirani Apr 23, 2020 •

edited

Loading

elasticmachine commented Apr 23, 2020

nik9000 commented Apr 23, 2020

nik9000 left a comment

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

jtibshirani Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

jtibshirani Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 Apr 23, 2020

nik9000 commented Apr 23, 2020

jtibshirani commented Apr 23, 2020

nik9000 left a comment

nik9000 Apr 24, 2020

		@@ -201,6 +202,10 @@ public InnerHitsContext innerHits() {

		public abstract SearchContext docValuesContext(FetchDocValuesContext docValuesContext);

		public abstract FetchFieldsContext fetchFieldsContext();

Add a simple 'fetch fields' phase. #55639

Add a simple 'fetch fields' phase. #55639

Conversation

jtibshirani commented Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Apr 23, 2020

nik9000 commented Apr 23, 2020

nik9000 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

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 Apr 23, 2020

jtibshirani commented Apr 23, 2020

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani commented Apr 23, 2020 •

edited

Loading

jtibshirani Apr 23, 2020 •

edited

Loading

jtibshirani Apr 23, 2020 •

edited

Loading