Skip to content

Commit 5457b34

Browse files
jtibshiranijtibshirani
authored
Correct how field retrieval handles multifields and copy_to. (#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 #61033.
1 parent 572bb40 commit 5457b34

File tree

63 files changed

+712
-539
lines changed

Some content is hidden

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

63 files changed

+712
-539
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
@@ -356,11 +356,6 @@ protected ScaledFloatFieldMapper clone() {
356356
return (ScaledFloatFieldMapper) super.clone();
357357
}
358358

359-
@Override
360-
protected Double nullValue() {
361-
return nullValue;
362-
}
363-
364359
@Override
365360
protected void parseCreateField(ParseContext context) throws IOException {
366361

@@ -480,25 +475,30 @@ private static double objectToDouble(Object value) {
480475
}
481476

482477
@Override
483-
protected Double parseSourceValue(Object value, String format) {
478+
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
484479
if (format != null) {
485480
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
486481
}
482+
return new SourceValueFetcher(name(), mapperService, parsesArrayValue()) {
483+
@Override
484+
protected Double parseSourceValue(Object value) {
485+
double doubleValue;
486+
if (value.equals("")) {
487+
if (nullValue == null) {
488+
return null;
489+
}
490+
doubleValue = nullValue;
491+
} else {
492+
doubleValue = objectToDouble(value);
493+
}
487494

488-
double doubleValue;
489-
if (value.equals("")) {
490-
if (nullValue == null) {
491-
return null;
495+
double scalingFactor = fieldType().getScalingFactor();
496+
return Math.round(doubleValue * scalingFactor) / scalingFactor;
492497
}
493-
doubleValue = nullValue;
494-
} else {
495-
doubleValue = objectToDouble(value);
496-
}
497-
498-
double scalingFactor = fieldType().getScalingFactor();
499-
return Math.round(doubleValue * scalingFactor) / scalingFactor;
498+
};
500499
}
501500

501+
502502
private static class ScaledFloatIndexFieldData extends IndexNumericFieldData {
503503

504504
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
@@ -39,6 +39,7 @@
3939
import java.io.IOException;
4040
import java.util.Arrays;
4141
import java.util.Collection;
42+
import java.util.List;
4243
import java.util.Set;
4344

4445
public class RankFeatureFieldMapperTests extends FieldMapperTestCase<RankFeatureFieldMapper.Builder> {
@@ -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: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,17 +30,16 @@
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
import org.junit.Before;
3534

3635
import java.io.IOException;
3736
import java.util.Arrays;
3837
import java.util.Collection;
39-
import java.util.Collections;
4038
import java.util.List;
4139
import java.util.Set;
4240

4341
import static java.util.Collections.singletonList;
42+
import static org.elasticsearch.index.mapper.FieldMapperTestCase.fetchSourceValue;
4443
import static org.hamcrest.Matchers.containsString;
4544

4645
public class ScaledFloatFieldMapperTests extends FieldMapperTestCase2<ScaledFloatFieldMapper.Builder> {
@@ -69,7 +68,7 @@ protected ScaledFloatFieldMapper.Builder newBuilder() {
6968

7069
@Override
7170
protected void minimalMapping(XContentBuilder b) throws IOException {
72-
b.field("type", "scaled_float").field("scaling_factor", 10.0);
71+
b.field("type", "scaled_float").field("scaling_factor", 10.0);
7372
}
7473

7574
public void testDefaults() throws Exception {
@@ -278,25 +277,21 @@ public void testRejectIndexOptions() throws IOException {
278277
assertThat(e.getMessage(), containsString("index_options not allowed in field [field] of type [scaled_float]"));
279278
}
280279

281-
public void testParseSourceValue() {
280+
public void testFetchSourceValue() {
282281
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
283282
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
284283

285284
ScaledFloatFieldMapper mapper = new ScaledFloatFieldMapper.Builder("field")
286285
.scalingFactor(100)
287286
.build(context);
288-
assertEquals(3.14, mapper.parseSourceValue(3.1415926, null), 0.00001);
289-
assertEquals(3.14, mapper.parseSourceValue("3.1415", null), 0.00001);
290-
assertNull(mapper.parseSourceValue("", null));
287+
assertEquals(List.of(3.14), fetchSourceValue(mapper, 3.1415926));
288+
assertEquals(List.of(3.14), fetchSourceValue(mapper, "3.1415"));
289+
assertEquals(List.of(), fetchSourceValue(mapper, ""));
291290

292291
ScaledFloatFieldMapper nullValueMapper = new ScaledFloatFieldMapper.Builder("field")
293292
.scalingFactor(100)
294293
.nullValue(2.71)
295294
.build(context);
296-
assertEquals(2.71, nullValueMapper.parseSourceValue("", null), 0.00001);
297-
298-
SourceLookup sourceLookup = new SourceLookup();
299-
sourceLookup.setSource(Collections.singletonMap("field", null));
300-
assertEquals(List.of(2.71), nullValueMapper.lookupValues(sourceLookup, null));
295+
assertEquals(List.of(2.71), fetchSourceValue(nullValueMapper, ""));
301296
}
302297
}

‎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
@@ -62,11 +62,14 @@
6262
import org.elasticsearch.index.mapper.MappedFieldType;
6363
import org.elasticsearch.index.mapper.Mapper;
6464
import org.elasticsearch.index.mapper.MapperParsingException;
65+
import org.elasticsearch.index.mapper.MapperService;
6566
import org.elasticsearch.index.mapper.NumberFieldMapper;
6667
import org.elasticsearch.index.mapper.ParseContext;
6768
import org.elasticsearch.index.mapper.RangeFieldMapper;
6869
import org.elasticsearch.index.mapper.RangeType;
70+
import org.elasticsearch.index.mapper.SourceValueFetcher;
6971
import org.elasticsearch.index.mapper.TextSearchInfo;
72+
import org.elasticsearch.index.mapper.ValueFetcher;
7073
import org.elasticsearch.index.query.BoolQueryBuilder;
7174
import org.elasticsearch.index.query.BoostingQueryBuilder;
7275
import org.elasticsearch.index.query.ConstantScoreQueryBuilder;
@@ -366,11 +369,16 @@ public void parse(ParseContext context) throws IOException {
366369
}
367370

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

376384
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)