Skip to content

Commit 0236aad

Browse files
jtibshiranijtibshirani
committed
Correct how field retrieval handles multifields and copy_to. (elastic#61309)
Before when a value was copied to a field through a parent field or `copy_to`, we parsed it using the `FieldMapper` from the source field. Instead we should parse it using the target `FieldMapper`. This ensures that we apply the appropriate mapping type and options to the copied value. To implement the fix cleanly, this PR refactors the value parsing strategy. Now instead of looking up values directly, field mappers produce a helper object `ValueFetcher`. The value fetchers are responsible for almost all aspects of fetching, including looking up the right paths in the _source. The PR is fairly big but each commit can be reviewed individually. Fixes elastic#61033.
1 parent 00b56bf commit 0236aad

File tree

64 files changed

+717
-544
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

64 files changed

+717
-544
lines changed

‎modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,11 +181,16 @@ private Float objectToFloat(Object value) {
181181
}
182182

183183
@Override
184-
protected Float parseSourceValue(Object value, String format) {
184+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
185185
if (format != null) {
186186
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
187187
}
188-
return objectToFloat(value);
188+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue()) {
189+
@Override
190+
protected Float parseSourceValue(Object value) {
191+
return objectToFloat(value);
192+
}
193+
};
189194
}
190195

191196
@Override

‎modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapper.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,11 +160,16 @@ protected void parseCreateField(ParseContext context) throws IOException {
160160
}
161161

162162
@Override
163-
protected Object parseSourceValue(Object value, String format) {
163+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
164164
if (format != null) {
165165
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
166166
}
167-
return value;
167+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue()) {
168+
@Override
169+
protected Object parseSourceValue(Object value) {
170+
return value;
171+
}
172+
};
168173
}
169174

170175
@Override

‎modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -311,11 +311,6 @@ protected ScaledFloatFieldMapper clone() {
311311
return (ScaledFloatFieldMapper) super.clone();
312312
}
313313

314-
@Override
315-
protected Double nullValue() {
316-
return nullValue;
317-
}
318-
319314
@Override
320315
protected void parseCreateField(ParseContext context) throws IOException {
321316

@@ -401,25 +396,30 @@ private static double objectToDouble(Object value) {
401396
}
402397

403398
@Override
404-
protected Double parseSourceValue(Object value, String format) {
399+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
405400
if (format != null) {
406401
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
407402
}
403+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue()) {
404+
@Override
405+
protected Double parseSourceValue(Object value) {
406+
double doubleValue;
407+
if (value.equals("")) {
408+
if (nullValue == null) {
409+
return null;
410+
}
411+
doubleValue = nullValue;
412+
} else {
413+
doubleValue = objectToDouble(value);
414+
}
408415

409-
double doubleValue;
410-
if (value.equals("")) {
411-
if (nullValue == null) {
412-
return null;
416+
double scalingFactor = fieldType().getScalingFactor();
417+
return Math.round(doubleValue * scalingFactor) / scalingFactor;
413418
}
414-
doubleValue = nullValue;
415-
} else {
416-
doubleValue = objectToDouble(value);
417-
}
418-
419-
double scalingFactor = fieldType().getScalingFactor();
420-
return Math.round(doubleValue * scalingFactor) / scalingFactor;
419+
};
421420
}
422421

422+
423423
private static class ScaledFloatIndexFieldData extends IndexNumericFieldData {
424424

425425
private final IndexNumericFieldData scaledFieldData;

‎modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ protected void parseCreateField(ParseContext context) {
419419
}
420420

421421
@Override
422-
protected Object parseSourceValue(Object value, String format) {
422+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
423423
throw new UnsupportedOperationException();
424424
}
425425

@@ -465,7 +465,7 @@ protected void mergeOptions(FieldMapper other, List<String> conflicts) {
465465
}
466466

467467
@Override
468-
protected Object parseSourceValue(Object value, String format) {
468+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
469469
throw new UnsupportedOperationException();
470470
}
471471

@@ -588,11 +588,8 @@ protected void parseCreateField(ParseContext context) throws IOException {
588588
}
589589

590590
@Override
591-
protected String parseSourceValue(Object value, String format) {
592-
if (format != null) {
593-
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
594-
}
595-
return value.toString();
591+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
592+
throw new UnsupportedOperationException();
596593
}
597594

598595
@Override

‎modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,12 +159,17 @@ protected void parseCreateField(ParseContext context) throws IOException {
159159
}
160160

161161
@Override
162-
protected String parseSourceValue(Object value, String format) {
162+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
163163
if (format != null) {
164164
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
165165
}
166166

167-
return value.toString();
167+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue(), nullValue) {
168+
@Override
169+
protected String parseSourceValue(Object value) {
170+
return value.toString();
171+
}
172+
};
168173
}
169174

170175
/**

‎modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.elasticsearch.cluster.metadata.IndexMetadata;
2828
import org.elasticsearch.common.Strings;
2929
import org.elasticsearch.common.bytes.BytesReference;
30+
import org.elasticsearch.common.collect.List;
3031
import org.elasticsearch.common.compress.CompressedXContent;
3132
import org.elasticsearch.common.settings.Settings;
3233
import org.elasticsearch.common.xcontent.XContentFactory;
@@ -189,12 +190,12 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
189190
e.getCause().getMessage());
190191
}
191192

192-
public void testParseSourceValue() {
193+
public void testFetchSourceValue() {
193194
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
194195
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
195196
RankFeatureFieldMapper mapper = new RankFeatureFieldMapper.Builder("field").build(context);
196197

197-
assertEquals(3.14f, mapper.parseSourceValue(3.14, null), 0.0001);
198-
assertEquals(42.9f, mapper.parseSourceValue("42.9", null), 0.0001);
198+
assertEquals(List.of(3.14f), fetchSourceValue(mapper, 3.14));
199+
assertEquals(List.of(42.9f), fetchSourceValue(mapper, "42.9"));
199200
}
200201
}

‎modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapperTests.java

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,14 @@
3030
import org.elasticsearch.common.xcontent.XContentFactory;
3131
import org.elasticsearch.common.xcontent.XContentType;
3232
import org.elasticsearch.plugins.Plugin;
33-
import org.elasticsearch.search.lookup.SourceLookup;
3433

3534
import java.io.IOException;
3635
import java.util.Arrays;
3736
import java.util.Collection;
38-
import java.util.Collections;
3937
import java.util.List;
4038

4139
import static java.util.Collections.singletonList;
40+
import static org.elasticsearch.index.mapper.FieldMapperTestCase.fetchSourceValue;
4241
import static org.hamcrest.Matchers.containsString;
4342

4443
public class ScaledFloatFieldMapperTests extends MapperTestCase {
@@ -261,25 +260,21 @@ public void testRejectIndexOptions() {
261260
assertWarnings("Parameter [index_options] has no effect on type [scaled_float] and will be removed in future");
262261
}
263262

264-
public void testParseSourceValue() {
263+
public void testFetchSourceValue() {
265264
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
266265
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
267266

268267
ScaledFloatFieldMapper mapper = new ScaledFloatFieldMapper.Builder("field", false, false)
269268
.scalingFactor(100)
270269
.build(context);
271-
assertEquals(3.14, mapper.parseSourceValue(3.1415926, null), 0.00001);
272-
assertEquals(3.14, mapper.parseSourceValue("3.1415", null), 0.00001);
273-
assertNull(mapper.parseSourceValue("", null));
270+
assertEquals(org.elasticsearch.common.collect.List.of(3.14), fetchSourceValue(mapper, 3.1415926));
271+
assertEquals(org.elasticsearch.common.collect.List.of(3.14), fetchSourceValue(mapper, "3.1415"));
272+
assertEquals(org.elasticsearch.common.collect.List.of(), fetchSourceValue(mapper, ""));
274273

275274
ScaledFloatFieldMapper nullValueMapper = new ScaledFloatFieldMapper.Builder("field", false, false)
276275
.scalingFactor(100)
277276
.nullValue(2.71)
278277
.build(context);
279-
assertEquals(2.71, nullValueMapper.parseSourceValue("", null), 0.00001);
280-
281-
SourceLookup sourceLookup = new SourceLookup();
282-
sourceLookup.setSource(Collections.singletonMap("field", null));
283-
assertEquals(org.elasticsearch.common.collect.List.of(2.71), nullValueMapper.lookupValues(sourceLookup, null));
278+
assertEquals(org.elasticsearch.common.collect.List.of(2.71), fetchSourceValue(nullValueMapper, ""));
284279
}
285280
}

‎modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,11 @@
2626
import org.elasticsearch.index.fielddata.IndexFieldData;
2727
import org.elasticsearch.index.fielddata.plain.SortedSetOrdinalsIndexFieldData;
2828
import org.elasticsearch.index.mapper.FieldMapper;
29+
import org.elasticsearch.index.mapper.MapperService;
2930
import org.elasticsearch.index.mapper.ParseContext;
3031
import org.elasticsearch.index.mapper.StringFieldType;
3132
import org.elasticsearch.index.mapper.TextSearchInfo;
33+
import org.elasticsearch.index.mapper.ValueFetcher;
3234
import org.elasticsearch.index.query.QueryShardContext;
3335
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
3436

@@ -136,8 +138,8 @@ protected void parseCreateField(ParseContext context) throws IOException {
136138
}
137139

138140
@Override
139-
protected Object parseSourceValue(Object value, String format) {
140-
throw new UnsupportedOperationException("The " + typeName() + " field is not stored in _source.");
141+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
142+
throw new UnsupportedOperationException("Cannot fetch values for metadata field [" + typeName() + "].");
141143
}
142144

143145
@Override

‎modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentIdFieldMapper.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,11 @@
3636
import org.elasticsearch.index.fielddata.plain.SortedSetOrdinalsIndexFieldData;
3737
import org.elasticsearch.index.mapper.FieldMapper;
3838
import org.elasticsearch.index.mapper.MappedFieldType;
39+
import org.elasticsearch.index.mapper.MapperService;
3940
import org.elasticsearch.index.mapper.ParseContext;
4041
import org.elasticsearch.index.mapper.StringFieldType;
4142
import org.elasticsearch.index.mapper.TextSearchInfo;
43+
import org.elasticsearch.index.mapper.ValueFetcher;
4244
import org.elasticsearch.index.query.QueryShardContext;
4345
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
4446

@@ -186,8 +188,8 @@ protected void parseCreateField(ParseContext context) throws IOException {
186188
}
187189

188190
@Override
189-
protected Object parseSourceValue(Object value, String format) {
190-
throw new UnsupportedOperationException("The " + typeName() + " field is not stored in _source.");
191+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
192+
throw new UnsupportedOperationException("Cannot fetch values for internal field [" + typeName() + "].");
191193
}
192194

193195
@Override

‎modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,10 @@
4242
import org.elasticsearch.index.mapper.MapperParsingException;
4343
import org.elasticsearch.index.mapper.MapperService;
4444
import org.elasticsearch.index.mapper.ParseContext;
45+
import org.elasticsearch.index.mapper.SourceValueFetcher;
4546
import org.elasticsearch.index.mapper.StringFieldType;
4647
import org.elasticsearch.index.mapper.TextSearchInfo;
48+
import org.elasticsearch.index.mapper.ValueFetcher;
4749
import org.elasticsearch.index.query.QueryShardContext;
4850
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
4951

@@ -348,11 +350,16 @@ protected void parseCreateField(ParseContext context) throws IOException {
348350
}
349351

350352
@Override
351-
protected Object parseSourceValue(Object value, String format) {
353+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
352354
if (format != null) {
353355
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
354356
}
355-
return value;
357+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue()) {
358+
@Override
359+
protected Object parseSourceValue(Object value) {
360+
return value;
361+
}
362+
};
356363
}
357364

358365
@Override

‎modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,14 @@
6565
import org.elasticsearch.index.mapper.MappedFieldType;
6666
import org.elasticsearch.index.mapper.Mapper;
6767
import org.elasticsearch.index.mapper.MapperParsingException;
68+
import org.elasticsearch.index.mapper.MapperService;
6869
import org.elasticsearch.index.mapper.NumberFieldMapper;
6970
import org.elasticsearch.index.mapper.ParseContext;
7071
import org.elasticsearch.index.mapper.RangeFieldMapper;
7172
import org.elasticsearch.index.mapper.RangeType;
73+
import org.elasticsearch.index.mapper.SourceValueFetcher;
7274
import org.elasticsearch.index.mapper.TextSearchInfo;
75+
import org.elasticsearch.index.mapper.ValueFetcher;
7376
import org.elasticsearch.index.query.BoolQueryBuilder;
7477
import org.elasticsearch.index.query.BoostingQueryBuilder;
7578
import org.elasticsearch.index.query.ConstantScoreQueryBuilder;
@@ -367,11 +370,16 @@ public void parse(ParseContext context) throws IOException {
367370
}
368371

369372
@Override
370-
protected Object parseSourceValue(Object value, String format) {
373+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
371374
if (format != null) {
372375
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
373376
}
374-
return value;
377+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue()) {
378+
@Override
379+
protected Object parseSourceValue(Object value) {
380+
return value;
381+
}
382+
};
375383
}
376384

377385
static void createQueryBuilderField(Version indexVersion, BinaryFieldMapper qbField,

‎plugins/analysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -577,11 +577,6 @@ protected String contentType() {
577577
return CONTENT_TYPE;
578578
}
579579

580-
@Override
581-
protected String nullValue() {
582-
return nullValue;
583-
}
584-
585580
@Override
586581
protected void mergeOptions(FieldMapper other, List<String> conflicts) {
587582
ICUCollationKeywordFieldMapper icuMergeWith = (ICUCollationKeywordFieldMapper) other;
@@ -738,15 +733,21 @@ protected void parseCreateField(ParseContext context) throws IOException {
738733
}
739734

740735
@Override
741-
protected String parseSourceValue(Object value, String format) {
736+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
742737
if (format != null) {
743738
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
744739
}
745740

746-
String keywordValue = value.toString();
747-
if (keywordValue.length() > ignoreAbove) {
748-
return null;
749-
}
750-
return keywordValue;
741+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue(), nullValue) {
742+
@Override
743+
protected String parseSourceValue(Object value) {
744+
String keywordValue = value.toString();
745+
if (keywordValue.length() > ignoreAbove) {
746+
return null;
747+
}
748+
return keywordValue;
749+
}
750+
};
751751
}
752+
752753
}

0 commit comments

Comments
 (0)