-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add a simple 'fetch fields' phase. #55639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
88d42d8
9583e1a
e6c86a7
facc1cc
6365394
f1f179a
cb9c6e0
8f71d7b
f64415e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
setup: | ||
- skip: | ||
version: " - 7.99.99" | ||
reason: "fields retrieval is currently only implemented on master" | ||
- do: | ||
indices.create: | ||
index: test | ||
body: | ||
mappings: | ||
properties: | ||
keyword: | ||
type: keyword | ||
integer_range: | ||
type: integer_range | ||
|
||
- do: | ||
index: | ||
index: test | ||
id: 1 | ||
body: | ||
keyword: [ "first", "second" ] | ||
integer_range: | ||
gte: 0 | ||
lte: 42 | ||
|
||
- do: | ||
indices.refresh: | ||
index: [ test ] | ||
|
||
--- | ||
"Test basic field retrieval": | ||
- do: | ||
search: | ||
index: test | ||
body: | ||
fields: [keyword, integer_range] | ||
|
||
- is_true: hits.hits.0._id | ||
- is_true: hits.hits.0._source | ||
|
||
- match: { hits.hits.0.fields.keyword.0: first } | ||
- match: { hits.hits.0.fields.keyword.1: second } | ||
|
||
- match: { hits.hits.0.fields.integer_range.0.gte: 0 } | ||
- match: { hits.hits.0.fields.integer_range.0.lte: 42 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,10 +113,9 @@ public void writeTo(StreamOutput out) throws IOException { | |
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
builder.startArray(name); | ||
for (Object value : values) { | ||
// this call doesn't really need to support writing any kind of object. | ||
// Stored fields values are converted using MappedFieldType#valueForDisplay. | ||
// As a result they can either be Strings, Numbers, or Booleans, that's | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! Except it won't be for stuff like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But that is a problem for another time, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
builder.value(value); | ||
} | ||
builder.endArray(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
|
||
import org.apache.logging.log4j.LogManager; | ||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.common.Booleans; | ||
import org.elasticsearch.common.Nullable; | ||
import org.elasticsearch.common.ParseField; | ||
|
@@ -94,6 +95,7 @@ public final class SearchSourceBuilder implements Writeable, ToXContentObject, R | |
public static final ParseField _SOURCE_FIELD = new ParseField("_source"); | ||
public static final ParseField STORED_FIELDS_FIELD = new ParseField("stored_fields"); | ||
public static final ParseField DOCVALUE_FIELDS_FIELD = new ParseField("docvalue_fields"); | ||
public static final ParseField FETCH_FIELDS_FIELD = new ParseField("fields"); | ||
public static final ParseField SCRIPT_FIELDS_FIELD = new ParseField("script_fields"); | ||
public static final ParseField SCRIPT_FIELD = new ParseField("script"); | ||
public static final ParseField IGNORE_FAILURE_FIELD = new ParseField("ignore_failure"); | ||
|
@@ -170,6 +172,7 @@ public static HighlightBuilder highlight() { | |
private List<FieldAndFormat> docValueFields; | ||
private List<ScriptField> scriptFields; | ||
private FetchSourceContext fetchSourceContext; | ||
private List<String> fetchFields; | ||
|
||
private AggregatorFactories.Builder aggregations; | ||
|
||
|
@@ -244,6 +247,10 @@ 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)) { | ||
fetchFields = in.readOptionalStringList(); | ||
} | ||
} | ||
|
||
@Override | ||
|
@@ -298,6 +305,10 @@ 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.writeOptionalStringCollection(fetchFields); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -825,6 +836,24 @@ public SearchSourceBuilder docValueField(String name) { | |
return docValueField(name, null); | ||
} | ||
|
||
/** | ||
* Gets the fields to load and return as part of the search request. | ||
*/ | ||
public List<String> fetchFields() { | ||
return fetchFields; | ||
} | ||
|
||
/** | ||
* Adds a field to load and return as part of the search request. | ||
*/ | ||
public SearchSourceBuilder fetchField(String fieldName) { | ||
if (fetchFields == null) { | ||
fetchFields = new ArrayList<>(); | ||
} | ||
fetchFields.add(fieldName); | ||
return this; | ||
} | ||
|
||
/** | ||
* Adds a script field under the given name with the provided script. | ||
* | ||
|
@@ -1120,6 +1149,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())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder when it'll be time to convert this to |
||
fetchFields = new ArrayList<>(); | ||
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { | ||
fetchFields.add(parser.text()); | ||
} | ||
} else if (INDICES_BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { | ||
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { | ||
indexBoosts.add(new IndexBoost(parser)); | ||
|
@@ -1227,6 +1261,10 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t | |
builder.endArray(); | ||
} | ||
|
||
if (fetchFields != null) { | ||
builder.array(FETCH_FIELDS_FIELD.getPreferredName(), fetchFields); | ||
} | ||
|
||
if (scriptFields != null) { | ||
builder.startObject(SCRIPT_FIELDS_FIELD.getPreferredName()); | ||
for (ScriptField scriptField : scriptFields) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.elasticsearch.search.fetch.subphase; | ||
|
||
import java.util.List; | ||
|
||
/** | ||
* The context needed to retrieve fields. | ||
*/ | ||
public class FetchFieldsContext { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you see this ever being more than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if it makes sense to have this class at all if it'll never end up holding more than a single list. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have immediate plans to add more information to the context. But I think it's worth having a dedicated context class for consistency with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough! |
||
|
||
private final List<String> fields; | ||
|
||
public FetchFieldsContext(List<String> fields) { | ||
this.fields = fields; | ||
} | ||
|
||
public List<String> fields() { | ||
return this.fields; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.search.fetch.subphase; | ||
|
||
import org.apache.lucene.index.LeafReaderContext; | ||
import org.apache.lucene.index.ReaderUtil; | ||
import org.elasticsearch.common.document.DocumentField; | ||
import org.elasticsearch.common.xcontent.support.XContentMapValues; | ||
import org.elasticsearch.index.mapper.DocumentMapper; | ||
import org.elasticsearch.search.SearchHit; | ||
import org.elasticsearch.search.fetch.FetchSubPhase; | ||
import org.elasticsearch.search.internal.SearchContext; | ||
import org.elasticsearch.search.lookup.SourceLookup; | ||
|
||
import java.util.Collection; | ||
import java.util.HashMap; | ||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.function.Function; | ||
|
||
/** | ||
* A fetch sub-phase for high-level field retrieval. Given a list of fields, it | ||
* retrieves the field values from _source and returns them as document fields. | ||
*/ | ||
public final class FetchFieldsPhase implements FetchSubPhase { | ||
|
||
@Override | ||
public void hitsExecute(SearchContext context, SearchHit[] hits) { | ||
hitsExecute(context, hit -> getSourceLookup(context, hit), hits); | ||
} | ||
|
||
// Visible for testing. | ||
@SuppressWarnings("unchecked") | ||
void hitsExecute(SearchContext context, | ||
Function<SearchHit, SourceLookup> sourceProvider, | ||
SearchHit[] hits) { | ||
FetchFieldsContext fetchFieldsContext = context.fetchFieldsContext(); | ||
if (fetchFieldsContext == null || fetchFieldsContext.fields().isEmpty()) { | ||
return; | ||
} | ||
|
||
DocumentMapper documentMapper = context.mapperService().documentMapper(); | ||
if (documentMapper.sourceMapper().enabled() == false) { | ||
throw new IllegalArgumentException("Unable to retrieve the requested [fields] since _source is " + | ||
"disabled in the mappings for index [" + context.indexShard().shardId().getIndexName() + "]"); | ||
} | ||
|
||
Set<String> fields = new HashSet<>(); | ||
for (String fieldPattern : context.fetchFieldsContext().fields()) { | ||
if (documentMapper.objectMappers().containsKey(fieldPattern)) { | ||
continue; | ||
} | ||
Collection<String> concreteFields = context.mapperService().simpleMatchToFullName(fieldPattern); | ||
fields.addAll(concreteFields); | ||
} | ||
|
||
for (SearchHit hit : hits) { | ||
SourceLookup sourceLookup = sourceProvider.apply(hit); | ||
Map<String, Object> valuesByField = extractValues(sourceLookup, fields); | ||
|
||
for (Map.Entry<String, Object> entry : valuesByField.entrySet()) { | ||
String field = entry.getKey(); | ||
Object value = entry.getValue(); | ||
List<Object> values = value instanceof List | ||
? (List<Object>) value | ||
: List.of(value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could see wanting to switch this to something a little less |
||
|
||
DocumentField documentField = new DocumentField(field, values); | ||
hit.setField(field, documentField); | ||
} | ||
} | ||
} | ||
|
||
private SourceLookup getSourceLookup(SearchContext context, SearchHit hit) { | ||
SourceLookup sourceLookup = context.lookup().source(); | ||
int readerIndex = ReaderUtil.subIndex(hit.docId(), context.searcher().getIndexReader().leaves()); | ||
LeafReaderContext readerContext = context.searcher().getIndexReader().leaves().get(readerIndex); | ||
sourceLookup.setSegmentAndDocument(readerContext, hit.docId()); | ||
return sourceLookup; | ||
} | ||
|
||
/** | ||
* For each of the provided paths, return its value in the source. Note that in contrast with | ||
* {@link SourceLookup#extractRawValues}, array and object values can be returned. | ||
*/ | ||
private Map<String, Object> extractValues(SourceLookup sourceLookup, Collection<String> paths) { | ||
Map<String, Object> result = new HashMap<>(paths.size()); | ||
for (String path : paths) { | ||
Object value = XContentMapValues.extractValue(path, sourceLookup); | ||
if (value != null) { | ||
result.put(path, value); | ||
} | ||
} | ||
return result; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll no longer use a flat list of fields once we introduce support for formatting the values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just have this temporarily because it is simpler to parse?