Skip to content

Commit 3ed0da5

Browse files
jpountzjpountz
committed
GET operations should not extract fields from _source. #20158
This makes GET operations more consistent with `_search` operations which expect `(stored_)fields` to work on stored fields and source filtering to work on the `_source` field. This is now possible thanks to the fact that GET operations do not read from the translog anymore (#20102) and also allows to get rid of `FieldMapper#isGenerated`. The `_termvectors` API (and thus more_like_this too) was relying on the fact that GET operations would extract fields from either stored fields or the source so the logic to do this that used to exist in `ShardGetService` has been moved to `TermVectorsService`. It would be nice that term vectors do not rely on this, but this does not seem to be a low hanging fruit.
1 parent 6fe9ae2 commit 3ed0da5

File tree

17 files changed

+133
-134
lines changed

17 files changed

+133
-134
lines changed

‎core/src/main/java/org/elasticsearch/index/get/ShardGetService.java

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -41,28 +41,19 @@
4141
import org.elasticsearch.index.mapper.FieldMapper;
4242
import org.elasticsearch.index.mapper.MapperService;
4343
import org.elasticsearch.index.mapper.ParentFieldMapper;
44-
import org.elasticsearch.index.mapper.RoutingFieldMapper;
4544
import org.elasticsearch.index.mapper.SourceFieldMapper;
46-
import org.elasticsearch.index.mapper.TTLFieldMapper;
47-
import org.elasticsearch.index.mapper.TimestampFieldMapper;
4845
import org.elasticsearch.index.mapper.Uid;
4946
import org.elasticsearch.index.mapper.UidFieldMapper;
5047
import org.elasticsearch.index.shard.AbstractIndexShardComponent;
5148
import org.elasticsearch.index.shard.IndexShard;
52-
import org.elasticsearch.index.translog.Translog;
5349
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
5450
import org.elasticsearch.search.fetch.subphase.ParentFieldSubFetchPhase;
55-
import org.elasticsearch.search.lookup.LeafSearchLookup;
56-
import org.elasticsearch.search.lookup.SearchLookup;
5751

5852
import java.io.IOException;
59-
import java.util.Arrays;
6053
import java.util.Collections;
6154
import java.util.HashMap;
62-
import java.util.HashSet;
6355
import java.util.List;
6456
import java.util.Map;
65-
import java.util.Set;
6657
import java.util.concurrent.TimeUnit;
6758

6859
/**
@@ -218,41 +209,14 @@ private GetResult innerGetLoadFromStoredFields(String type, String id, String[]
218209
fields.put(ParentFieldMapper.NAME, new GetField(ParentFieldMapper.NAME, Collections.singletonList(parentId)));
219210
}
220211

221-
// now, go and do the script thingy if needed
222-
223212
if (gFields != null && gFields.length > 0) {
224-
SearchLookup searchLookup = null;
225213
for (String field : gFields) {
226-
Object value = null;
227214
FieldMapper fieldMapper = docMapper.mappers().smartNameFieldMapper(field);
228215
if (fieldMapper == null) {
229216
if (docMapper.objectMappers().get(field) != null) {
230217
// Only fail if we know it is a object field, missing paths / fields shouldn't fail.
231218
throw new IllegalArgumentException("field [" + field + "] isn't a leaf field");
232219
}
233-
} else if (!fieldMapper.fieldType().stored() && !fieldMapper.isGenerated()) {
234-
if (searchLookup == null) {
235-
searchLookup = new SearchLookup(mapperService, null, new String[]{type});
236-
LeafSearchLookup leafSearchLookup = searchLookup.getLeafSearchLookup(docIdAndVersion.context);
237-
searchLookup.source().setSource(source);
238-
leafSearchLookup.setDocument(docIdAndVersion.docId);
239-
}
240-
241-
List<Object> values = searchLookup.source().extractRawValues(field);
242-
if (values.isEmpty() == false) {
243-
value = values;
244-
}
245-
}
246-
247-
if (value != null) {
248-
if (fields == null) {
249-
fields = new HashMap<>(2);
250-
}
251-
if (value instanceof List) {
252-
fields.put(field, new GetField(field, (List) value));
253-
} else {
254-
fields.put(field, new GetField(field, Collections.singletonList(value)));
255-
}
256220
}
257221
}
258222
}

‎core/src/main/java/org/elasticsearch/index/mapper/AllFieldMapper.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,4 @@ protected void doMerge(Mapper mergeWith, boolean updateAllTypes) {
291291
super.doMerge(mergeWith, updateAllTypes);
292292
}
293293

294-
@Override
295-
public boolean isGenerated() {
296-
return true;
297-
}
298294
}

‎core/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -660,14 +660,4 @@ public List<String> copyToFields() {
660660
}
661661
}
662662

663-
/**
664-
* Fields might not be available before indexing, for example _all, token_count,...
665-
* When get is called and these fields are requested, this case needs special treatment.
666-
*
667-
* @return If the field is available before indexing or not.
668-
*/
669-
public boolean isGenerated() {
670-
return false;
671-
}
672-
673663
}

‎core/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -289,8 +289,4 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
289289
return builder;
290290
}
291291

292-
@Override
293-
public boolean isGenerated() {
294-
return true;
295-
}
296292
}

‎core/src/main/java/org/elasticsearch/index/mapper/LegacyTokenCountFieldMapper.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,4 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
187187
builder.field("analyzer", analyzer());
188188
}
189189

190-
@Override
191-
public boolean isGenerated() {
192-
return true;
193-
}
194-
195190
}

‎core/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,4 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
182182
builder.field("analyzer", analyzer());
183183
}
184184

185-
@Override
186-
public boolean isGenerated() {
187-
return true;
188-
}
189-
190185
}

‎core/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
import org.elasticsearch.common.Strings;
3636
import org.elasticsearch.common.bytes.BytesReference;
3737
import org.elasticsearch.common.lucene.uid.Versions;
38+
import org.elasticsearch.common.xcontent.XContentHelper;
39+
import org.elasticsearch.common.xcontent.support.XContentMapValues;
3840
import org.elasticsearch.index.engine.Engine;
3941
import org.elasticsearch.index.get.GetField;
4042
import org.elasticsearch.index.get.GetResult;
@@ -44,6 +46,7 @@
4446
import org.elasticsearch.index.mapper.MapperService;
4547
import org.elasticsearch.index.mapper.ParseContext;
4648
import org.elasticsearch.index.mapper.ParsedDocument;
49+
import org.elasticsearch.index.mapper.SourceFieldMapper;
4750
import org.elasticsearch.index.mapper.StringFieldMapper;
4851
import org.elasticsearch.index.mapper.TextFieldMapper;
4952
import org.elasticsearch.index.mapper.Uid;
@@ -55,8 +58,10 @@
5558
import java.util.Arrays;
5659
import java.util.Collection;
5760
import java.util.Collections;
61+
import java.util.HashMap;
5862
import java.util.HashSet;
5963
import java.util.Iterator;
64+
import java.util.List;
6065
import java.util.Map;
6166
import java.util.Set;
6267
import java.util.TreeMap;
@@ -191,9 +196,11 @@ private static Fields addGeneratedTermVectors(IndexShard indexShard, Engine.GetR
191196
}
192197

193198
/* generate term vectors from fetched document fields */
199+
String[] getFields = validFields.toArray(new String[validFields.size() + 1]);
200+
getFields[getFields.length - 1] = SourceFieldMapper.NAME;
194201
GetResult getResult = indexShard.getService().get(
195-
get, request.id(), request.type(), validFields.toArray(Strings.EMPTY_ARRAY), null);
196-
Fields generatedTermVectors = generateTermVectors(indexShard, getResult.getFields().values(), request.offsets(), request.perFieldAnalyzer(), validFields);
202+
get, request.id(), request.type(), getFields, null);
203+
Fields generatedTermVectors = generateTermVectors(indexShard, getResult.sourceAsMap(), getResult.getFields().values(), request.offsets(), request.perFieldAnalyzer(), validFields);
197204

198205
/* merge with existing Fields */
199206
if (termVectorsByField == null) {
@@ -227,17 +234,31 @@ private static Set<String> getFieldsToGenerate(Map<String, String> perAnalyzerFi
227234
return selectedFields;
228235
}
229236

230-
private static Fields generateTermVectors(IndexShard indexShard, Collection<GetField> getFields, boolean withOffsets, @Nullable Map<String, String> perFieldAnalyzer, Set<String> fields) throws IOException {
231-
/* store document in memory index */
232-
MemoryIndex index = new MemoryIndex(withOffsets);
237+
private static Fields generateTermVectors(IndexShard indexShard, Map<String, Object> source, Collection<GetField> getFields, boolean withOffsets, @Nullable Map<String, String> perFieldAnalyzer, Set<String> fields) throws IOException {
238+
Map<String, Collection<Object>> values = new HashMap<>();
233239
for (GetField getField : getFields) {
234240
String field = getField.getName();
235-
if (fields.contains(field) == false) {
236-
// some fields are returned even when not asked for, eg. _timestamp
237-
continue;
241+
if (fields.contains(field)) { // some fields are returned even when not asked for, eg. _timestamp
242+
values.put(field, getField.getValues());
238243
}
244+
}
245+
if (source != null) {
246+
for (String field : fields) {
247+
if (values.containsKey(field) == false) {
248+
List<Object> v = XContentMapValues.extractRawValues(field, source);
249+
if (v.isEmpty() == false) {
250+
values.put(field, v);
251+
}
252+
}
253+
}
254+
}
255+
256+
/* store document in memory index */
257+
MemoryIndex index = new MemoryIndex(withOffsets);
258+
for (Map.Entry<String, Collection<Object>> entry : values.entrySet()) {
259+
String field = entry.getKey();
239260
Analyzer analyzer = getAnalyzerAtField(indexShard, field, perFieldAnalyzer);
240-
for (Object text : getField.getValues()) {
261+
for (Object text : entry.getValue()) {
241262
index.addField(field, text.toString(), analyzer);
242263
}
243264
}
@@ -270,7 +291,7 @@ private static Fields generateTermVectorsFromDoc(IndexShard indexShard, TermVect
270291
String[] values = doc.getValues(field.name());
271292
getFields.add(new GetField(field.name(), Arrays.asList((Object[]) values)));
272293
}
273-
return generateTermVectors(indexShard, getFields, request.offsets(), request.perFieldAnalyzer(), seenFields);
294+
return generateTermVectors(indexShard, XContentHelper.convertToMap(parsedDocument.source(), true).v2(), getFields, request.offsets(), request.perFieldAnalyzer(), seenFields);
274295
}
275296

276297
private static ParsedDocument parseDocument(IndexShard indexShard, String index, String type, BytesReference doc) {

‎core/src/test/java/org/elasticsearch/action/bulk/BulkWithUpdatesIT.java

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -149,21 +149,21 @@ public void testBulkUpdateSimple() throws Exception {
149149
assertThat(bulkResponse.getItems()[2].getResponse().getId(), equalTo("3"));
150150
assertThat(bulkResponse.getItems()[2].getResponse().getVersion(), equalTo(2L));
151151

152-
GetResponse getResponse = client().prepareGet().setIndex("test").setType("type1").setId("1").setFields("field").execute()
152+
GetResponse getResponse = client().prepareGet().setIndex("test").setType("type1").setId("1").execute()
153153
.actionGet();
154154
assertThat(getResponse.isExists(), equalTo(true));
155155
assertThat(getResponse.getVersion(), equalTo(2L));
156-
assertThat(((Number) getResponse.getField("field").getValue()).longValue(), equalTo(2L));
156+
assertThat(((Number) getResponse.getSource().get("field")).longValue(), equalTo(2L));
157157

158-
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("2").setFields("field").execute().actionGet();
158+
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("2").execute().actionGet();
159159
assertThat(getResponse.isExists(), equalTo(true));
160160
assertThat(getResponse.getVersion(), equalTo(2L));
161-
assertThat(((Number) getResponse.getField("field").getValue()).longValue(), equalTo(3L));
161+
assertThat(((Number) getResponse.getSource().get("field")).longValue(), equalTo(3L));
162162

163-
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("3").setFields("field1").execute().actionGet();
163+
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("3").execute().actionGet();
164164
assertThat(getResponse.isExists(), equalTo(true));
165165
assertThat(getResponse.getVersion(), equalTo(2L));
166-
assertThat(getResponse.getField("field1").getValue().toString(), equalTo("test"));
166+
assertThat(getResponse.getSource().get("field1").toString(), equalTo("test"));
167167

168168
bulkResponse = client()
169169
.prepareBulk()
@@ -185,18 +185,18 @@ public void testBulkUpdateSimple() throws Exception {
185185
assertThat(bulkResponse.getItems()[2].getResponse().getIndex(), equalTo("test"));
186186
assertThat(bulkResponse.getItems()[2].getResponse().getVersion(), equalTo(3L));
187187

188-
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("6").setFields("field").execute().actionGet();
188+
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("6").execute().actionGet();
189189
assertThat(getResponse.isExists(), equalTo(true));
190190
assertThat(getResponse.getVersion(), equalTo(1L));
191-
assertThat(((Number) getResponse.getField("field").getValue()).longValue(), equalTo(0L));
191+
assertThat(((Number) getResponse.getSource().get("field")).longValue(), equalTo(0L));
192192

193-
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("7").setFields("field").execute().actionGet();
193+
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("7").execute().actionGet();
194194
assertThat(getResponse.isExists(), equalTo(false));
195195

196-
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("2").setFields("field").execute().actionGet();
196+
getResponse = client().prepareGet().setIndex("test").setType("type1").setId("2").execute().actionGet();
197197
assertThat(getResponse.isExists(), equalTo(true));
198198
assertThat(getResponse.getVersion(), equalTo(3L));
199-
assertThat(((Number) getResponse.getField("field").getValue()).longValue(), equalTo(4L));
199+
assertThat(((Number) getResponse.getSource().get("field")).longValue(), equalTo(4L));
200200
}
201201

202202
public void testBulkVersioning() throws Exception {
@@ -325,11 +325,11 @@ public void testBulkUpdateLargerVolume() throws Exception {
325325
assertThat(((UpdateResponse) response.getItems()[i].getResponse()).getGetResult().field("counter").getValue(), equalTo(1));
326326

327327
for (int j = 0; j < 5; j++) {
328-
GetResponse getResponse = client().prepareGet("test", "type1", Integer.toString(i)).setFields("counter").execute()
328+
GetResponse getResponse = client().prepareGet("test", "type1", Integer.toString(i)).execute()
329329
.actionGet();
330330
assertThat(getResponse.isExists(), equalTo(true));
331331
assertThat(getResponse.getVersion(), equalTo(1L));
332-
assertThat(((Number) getResponse.getField("counter").getValue()).longValue(), equalTo(1L));
332+
assertThat(((Number) getResponse.getSource().get("counter")).longValue(), equalTo(1L));
333333
}
334334
}
335335

‎core/src/test/java/org/elasticsearch/document/AliasedIndexDocumentActionsIT.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,9 @@ protected void createIndex() {
3737
// ignore
3838
}
3939
logger.info("--> creating index test");
40-
client().admin().indices().create(createIndexRequest("test1").alias(new Alias("test"))).actionGet();
40+
client().admin().indices().create(createIndexRequest("test1")
41+
.mapping("type1", "name", "type=keyword,store=true")
42+
.alias(new Alias("test"))).actionGet();
4143
}
4244

4345
@Override

‎core/src/test/java/org/elasticsearch/document/DocumentActionsIT.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.elasticsearch.common.xcontent.XContentBuilder;
3434
import org.elasticsearch.common.xcontent.XContentFactory;
3535
import org.elasticsearch.test.ESIntegTestCase;
36+
import org.elasticsearch.test.hamcrest.ElasticsearchAssertions;
3637

3738
import java.io.IOException;
3839

@@ -50,7 +51,7 @@
5051
*/
5152
public class DocumentActionsIT extends ESIntegTestCase {
5253
protected void createIndex() {
53-
createIndex(getConcreteIndexName());
54+
ElasticsearchAssertions.assertAcked(prepareCreate(getConcreteIndexName()).addMapping("type1", "name", "type=keyword,store=true"));
5455
}
5556

5657
protected String getConcreteIndexName() {

0 commit comments

Comments
 (0)