Skip to content

Return empty data instead of throwing when Fields API tries to access non-available "fielddata" #99583

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ public class ScriptScoreBenchmark {
private final SearchLookup lookup = new SearchLookup(
fieldTypes::get,
(mft, lookup, fdo) -> mft.fielddataBuilder(FieldDataContext.noRuntimeFields("benchmark")).build(fieldDataCache, breakerService),
(mft, lookup, fdo) -> mft.isFielddataSupported(FieldDataContext.noRuntimeFields("benchmark")),
SourceProvider.fromStoredFields()
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@
import org.elasticsearch.core.IOUtils;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.IndexVersion;
import org.elasticsearch.index.fielddata.FieldDataContext;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.IndexFieldDataCache;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.MapperRegistry;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.ParsedDocument;
Expand Down Expand Up @@ -135,12 +138,17 @@ public void expand() {
protected SearchExecutionContext buildSearchExecutionContext() {
final SimilarityService similarityService = new SimilarityService(mapperService.getIndexSettings(), null, Map.of());
final long nowInMillis = 1;
return new SearchExecutionContext(
0,
0,
mapperService.getIndexSettings(),
null,
(ft, fdc) -> ft.fielddataBuilder(fdc).build(new IndexFieldDataCache.None(), new NoneCircuitBreakerService()),
return new SearchExecutionContext(0, 0, mapperService.getIndexSettings(), null, new SearchExecutionContext.IndexFieldDataLookup() {
@Override
public boolean isFielddataSupportedForField(MappedFieldType fieldType, FieldDataContext context) {
return fieldType.isFielddataSupported(context);
}

@Override
public IndexFieldData<?> getForField(MappedFieldType fieldType, FieldDataContext context) {
return fieldType.fielddataBuilder(context).build(new IndexFieldDataCache.None(), new NoneCircuitBreakerService());
}
},
mapperService,
mapperService.mappingLookup(),
similarityService,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public void setUp() throws Exception {
lookup = new SearchLookup(
field -> field.equals("field") ? fieldType : null,
(ignored, _lookup, fdt) -> fieldData,
(ignored, _lookup, fdt) -> true,
(ctx, doc) -> Source.empty(XContentType.JSON)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public void setUp() throws Exception {
lookup = new SearchLookup(
field -> field.equals("field") ? fieldType : null,
(ignored, _lookup, fdt) -> fieldData,
(ignored, _lookup, fdt) -> true,
(ctx, doc) -> Source.empty(XContentType.JSON)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public void setUp() throws Exception {
lookup = new SearchLookup(
field -> field.equals("field") ? fieldType : null,
(ignored, _lookup, fdt) -> fieldData,
(ignored, _lookup, fdt) -> true,
(ctx, doc) -> Source.empty(XContentType.JSON)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ setup:
rank: 1
boolean: true
boolean_no_doc_values: true
boolean_not_mapped: true
date: 2017-01-01T12:11:12
date_no_doc_values: 2017-01-01T12:11:12
nanos: 2015-01-01T12:10:30.123456789Z
Expand Down Expand Up @@ -412,6 +413,66 @@ boolean:
source: "field('boolean').size()"
- match: { hits.hits.0.fields.field.0: 0 }

---
boolean_not_existing:
- do:
catch: bad_request
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "doc['boolean_not_existing'].value"
- match: { error.failed_shards.0.reason.caused_by.type: "illegal_argument_exception" }

- do:
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "field('boolean_not_existing').empty"
- match: { hits.hits.0.fields.field.0: true }

- do:
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "field('boolean_not_existing').get(false)"
- match: { hits.hits.0.fields.field.0: false }

---
boolean_not_mapped:
- do:
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "doc['boolean_not_mapped'].value"
- match: { hits.hits.0.fields.field.0: true }

- do:
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "field('boolean_not_mapped').get(false)"
- match: { hits.hits.0.fields.field.0: true }

---
boolean_no_doc_values:
- do:
Expand Down Expand Up @@ -449,6 +510,28 @@ boolean_no_doc_values:
source: "field('boolean_no_doc_values').get(false)"
- match: { hits.hits.0.fields.field.0: true }

- do:
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "field('boolean_no_doc_values').empty"
- match: { hits.hits.0.fields.field.0: false }

- do:
search:
index: test
body:
query: { term: { _id: "2" } }
script_fields:
field:
script:
source: "field('boolean_no_doc_values').empty"
- match: { hits.hits.0.fields.field.0: true }

- do:
search:
index: test
Expand Down Expand Up @@ -2302,6 +2385,17 @@ keyword_synthetic:
source: "doc['keyword'].value"
- match: { hits.hits.0.fields.field.0: "not split at all" }

- do:
search:
index: test_synthetic
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "field('keyword').get('null')"
- match: { hits.hits.0.fields.field.0: "not split at all" }

- do:
search:
index: test_synthetic
Expand Down Expand Up @@ -6124,3 +6218,38 @@ unsupported date methods:
source: "/* avoid stash */ $('date', null).get(ChronoField.YEAR_OF_ERA)"
- match: { hits.hits.0.fields.field.0: 2017 }

---
_id_field:
- do:
catch: bad_request
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "doc['_id'].value"
- match: { error.failed_shards.0.reason.caused_by.type: "illegal_argument_exception" }

- do:
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "field('_id').empty"
- match: { hits.hits.0.fields.field.0: true }

- do:
search:
index: test
body:
query: { term: { _id: "1" } }
script_fields:
field:
script:
source: "field('_id').get(0)"
- match: { hits.hits.0.fields.field.0: 0 }
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ public Query phrasePrefixQuery(TokenStream stream, int slop, int maxExpansions,
return toQuery(query, queryShardContext);
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
return fieldDataContext.fielddataOperation() == FielddataOperation.SCRIPT;
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
if (fieldDataContext.fielddataOperation() != FielddataOperation.SCRIPT) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,14 @@ public Query rangeQuery(
return NumberFieldMapper.NumberType.LONG.rangeQuery(name(), lo, hi, true, true, hasDocValues(), context, isIndexed());
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
if (fieldDataContext.fielddataOperation() == FielddataOperation.SEARCH) {
return hasDocValues();
}
return true;
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
FielddataOperation operation = fieldDataContext.fielddataOperation();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ public boolean eagerGlobalOrdinals() {
return eagerGlobalOrdinals;
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
return hasDocValues();
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
failIfNoDocValues();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ public String typeName() {
return CONTENT_TYPE;
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
return true;
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
return new SortedSetOrdinalsIndexFieldData.Builder(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ protected String parseSourceValue(Object value) {
};
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
return hasDocValues();
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
failIfNoDocValues();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ public String typeName() {
return CONTENT_TYPE;
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
return hasDocValues();
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
failIfNoDocValues();
Expand Down
13 changes: 12 additions & 1 deletion server/src/main/java/org/elasticsearch/index/IndexService.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.elasticsearch.index.engine.Engine;
import org.elasticsearch.index.engine.EngineFactory;
import org.elasticsearch.index.fielddata.FieldDataContext;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.IndexFieldDataCache;
import org.elasticsearch.index.fielddata.IndexFieldDataService;
import org.elasticsearch.index.fielddata.ordinals.GlobalOrdinalsAccounting;
Expand Down Expand Up @@ -660,7 +661,17 @@ public SearchExecutionContext newSearchExecutionContext(
shardRequestIndex,
indexSettings,
indexCache.bitsetFilterCache(),
indexFieldData::getForField,
new SearchExecutionContext.IndexFieldDataLookup() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is where the single interface with both lookup and predicate methods is "worse": the anonymous class is not brilliant to read.
If we like the single interface, I would like to extract a static inner class (adapter) to make it nicer (more readable). Thoughts?

@Override
public boolean isFielddataSupportedForField(MappedFieldType fieldType, FieldDataContext fieldDataContext) {
return indexFieldData.isFielddataSupportedForField(fieldType, fieldDataContext);
}

@Override
public IndexFieldData<?> getForField(MappedFieldType fieldType, FieldDataContext fieldDataContext) {
return indexFieldData.getForField(fieldType, fieldDataContext);
}
},
mapperService(),
mapperService().mappingLookup(),
similarityService(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ public synchronized void clearField(final String fieldName) {
ExceptionsHelper.maybeThrowRuntimeAndSuppress(exceptions);
}

/**
* Returns if fielddata is available for the provided field type in the provided context.
*/
public boolean isFielddataSupportedForField(MappedFieldType fieldType, FieldDataContext fieldDataContext) {
return fieldType.isFielddataSupported(fieldDataContext);
}

/**
* Returns fielddata for the provided field type, given the provided fully qualified index name, while also making
* a {@link SearchLookup} supplier available that is required for runtime fields.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ public BytesReference valueForDisplay(Object value) {
return bytes;
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
return hasDocValues();
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
failIfNoDocValues();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,14 @@ public Boolean valueForDisplay(Object value) {
};
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
if (fieldDataContext.fielddataOperation() == FielddataOperation.SEARCH) {
return hasDocValues();
}
return true;
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
FielddataOperation operation = fieldDataContext.fielddataOperation();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,11 @@ public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
return DocValueFormat.BOOLEAN;
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
return true;
}

@Override
public BooleanScriptFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
return new BooleanScriptFieldData.Builder(name(), leafFactory(fieldDataContext.lookupSupplier().get()), BooleanDocValuesField::new);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,14 @@ public Function<byte[], Number> pointReaderIfPossible() {
return null;
}

@Override
public boolean isFielddataSupported(FieldDataContext fieldDataContext) {
if (fieldDataContext.fielddataOperation() == FielddataOperation.SEARCH) {
return hasDocValues();
}
return true;
}

@Override
public IndexFieldData.Builder fielddataBuilder(FieldDataContext fieldDataContext) {
FielddataOperation operation = fieldDataContext.fielddataOperation();
Expand Down
Loading