Skip to content
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

Make FieldNamesFieldMapper responsible for adding its own doc fields #71929

Merged
merged 5 commits into from
Apr 27, 2021
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -365,7 +365,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
context.doc().addAll(fields);

if (hasDocValues == false && (indexed || stored)) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
context.doc().add(new Field(prefixField.fieldType().name(), value, prefixField.getLuceneFieldType()));
}
if (fieldType().fieldType.omitNorms()) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ void processQuery(Query query, ParseContext context) {
doc.add(new Field(extractionResultField.name(), EXTRACTION_PARTIAL, INDEXED_KEYWORD));
}

createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
doc.add(new NumericDocValuesField(minimumShouldMatchFieldMapper.name(), result.minimumShouldMatch));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ public void testExtractTermsAndRanges_partial() throws Exception {
ParseContext.Document document = parseContext.doc();

PercolatorFieldMapper.PercolatorFieldType fieldType = (PercolatorFieldMapper.PercolatorFieldType) fieldMapper.fieldType();
assertThat(document.getFields().size(), equalTo(4));
assertThat(document.getFields().size(), equalTo(3));
assertThat(document.getFields().get(0).binaryValue().utf8ToString(), equalTo("field\u0000term"));
assertThat(document.getField(fieldType.extractionResultField.name()).stringValue(), equalTo(EXTRACTION_PARTIAL));
}
Expand Down Expand Up @@ -609,7 +609,7 @@ public void testNestedPercolatorField() throws Exception {
.field("query_field", queryBuilder)
.endObject().endObject()),
XContentType.JSON));
assertThat(doc.rootDoc().getFields().size(), equalTo(12)); // also includes all other meta fields
assertThat(doc.rootDoc().getFields().size(), equalTo(11)); // also includes all other meta fields
IndexableField queryBuilderField = doc.rootDoc().getField("object_field.query_field.query_builder_field");
assertTrue(queryBuilderField.fieldType().omitNorms());
IndexableField extractionResultField = doc.rootDoc().getField("object_field.query_field.extraction_result");
Expand All @@ -624,7 +624,7 @@ public void testNestedPercolatorField() throws Exception {
.endArray()
.endObject()),
XContentType.JSON));
assertThat(doc.rootDoc().getFields().size(), equalTo(12)); // also includes all other meta fields
assertThat(doc.rootDoc().getFields().size(), equalTo(11)); // also includes all other meta fields
queryBuilderAsBytes = doc.rootDoc().getField("object_field.query_field.query_builder_field").binaryValue();
assertQueryBuilder(queryBuilderAsBytes, queryBuilder);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
if (hasDocValues) {
context.doc().add(new SortedSetDocValuesField(fieldType().name(), binaryValue));
} else if (fieldType.indexOptions() != IndexOptions.NONE || fieldType.stored()) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
Field field = new Field(mappedFieldType.name(), value, fieldType);
context.doc().add(field);
if (fieldType.omitNorms()) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public void indexValue(ParseContext context, byte[] value) {
// Only add an entry to the field names field if the field is stored
// but has no doc values so exists query will work on a field with
// no doc values
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ private void indexValue(ParseContext context, Boolean value) {
if (hasDocValues) {
context.doc().add(new SortedNumericDocValuesField(fieldType().name(), value ? 1 : 0));
} else {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ public void parse(ParseContext context) throws IOException {
}
}

createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
for (CompletionInputMetadata metadata: inputMap.values()) {
ParseContext externalValueContext = context.createExternalValueContext(metadata);
multiFields.parse(this, externalValueContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ private void indexValue(ParseContext context, long timestamp) {
if (hasDocValues) {
context.doc().add(new SortedNumericDocValuesField(fieldType().name(), timestamp));
} else if (store || indexed) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
if (store) {
context.doc().add(new StoredField(fieldType().name(), timestamp));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

package org.elasticsearch.index.mapper;

import org.apache.lucene.document.Field;
import org.apache.lucene.index.LeafReaderContext;
import org.elasticsearch.Version;
import org.elasticsearch.common.Explicit;
Expand All @@ -24,7 +23,6 @@
import org.elasticsearch.common.xcontent.support.AbstractXContentParser;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.analysis.NamedAnalyzer;
import org.elasticsearch.index.mapper.FieldNamesFieldMapper.FieldNamesFieldType;
import org.elasticsearch.index.mapper.Mapper.TypeParser.ParserContext;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptType;
Expand Down Expand Up @@ -266,19 +264,6 @@ protected void indexScriptValues(SearchLookup searchLookup, LeafReaderContext re
throw new UnsupportedOperationException("FieldMapper " + name() + " does not support [script]");
}

protected final void createFieldNamesField(ParseContext context) {
assert fieldType().hasDocValues() == false : "_field_names should only be used when doc_values are turned off";
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to keep this assertion? I vaguely remember adding it while cleaning up some inconsistencies around using field_names where it would not make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an assertion in FieldNamesFieldMapper.postParse()

FieldNamesFieldMapper fieldNamesFieldMapper = (FieldNamesFieldMapper) context.getMetadataMapper(FieldNamesFieldMapper.NAME);
if (fieldNamesFieldMapper != null) {
FieldNamesFieldType fieldNamesFieldType = fieldNamesFieldMapper.fieldType();
if (fieldNamesFieldType != null && fieldNamesFieldType.isEnabled()) {
for (String fieldName : FieldNamesFieldMapper.extractFieldNames(fieldType().name())) {
context.doc().add(new Field(FieldNamesFieldMapper.NAME, fieldName, FieldNamesFieldMapper.Defaults.FIELD_TYPE));
}
}
}
}

@Override
public Iterator<Mapper> iterator() {
Iterator<FieldMapper> multiFieldsIterator = multiFields.iterator();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

package org.elasticsearch.index.mapper;

import org.apache.lucene.document.Field;
import org.apache.lucene.document.FieldType;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.search.Query;
Expand All @@ -17,8 +18,8 @@
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.index.query.SearchExecutionContext;

import java.io.IOException;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;

/**
Expand Down Expand Up @@ -155,40 +156,14 @@ public FieldNamesFieldType fieldType() {
return (FieldNamesFieldType) super.fieldType();
}

static Iterable<String> extractFieldNames(final String fullPath) {
return new Iterable<String>() {
@Override
public Iterator<String> iterator() {
return new Iterator<>() {
int endIndex = nextEndIndex(0);

private int nextEndIndex(int index) {
while (index < fullPath.length() && fullPath.charAt(index) != '.') {
index += 1;
}
return index;
}

@Override
public boolean hasNext() {
return endIndex <= fullPath.length();
}

@Override
public String next() {
final String result = fullPath.substring(0, endIndex);
endIndex = nextEndIndex(endIndex + 1);
return result;
}

@Override
public void remove() {
throw new UnsupportedOperationException();
}

};
}
};
Copy link
Member

Choose a reason for hiding this comment

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

on simplifying this and not indexing all the inner paths, that makes sense, but should we make that change in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, I'll pull this change into a separate issue.

@Override
public void postParse(ParseContext context) throws IOException {
if (enabled.value() == false) {
return;
}
for (String field : context.getFieldExistsFields()) {
context.doc().add(new Field(NAME, field, Defaults.FIELD_TYPE));
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ protected void index(ParseContext context, GeoPoint geometry) throws IOException
if (fieldType().hasDocValues()) {
context.doc().add(new LatLonDocValuesField(fieldType().name(), geometry.lat(), geometry.lon()));
} else if (fieldType().isStored() || fieldType().isSearchable()) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
if (fieldType().isStored()) {
context.doc().add(new StoredField(fieldType().name(), geometry.toString()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ protected void checkIncomingMergeType(FieldMapper mergeWith) {
@Override
protected void index(ParseContext context, Geometry geometry) throws IOException {
context.doc().addAll(indexer.indexShape(geometry));
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ private void indexValue(ParseContext context, InetAddress address) {
if (hasDocValues) {
context.doc().add(new SortedSetDocValuesField(fieldType().name(), new BytesRef(InetAddressPoint.encode(address))));
} else if (stored || indexed) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
if (stored) {
context.doc().add(new StoredField(fieldType().name(), new BytesRef(InetAddressPoint.encode(address))));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ private void indexValue(ParseContext context, String value) {
context.doc().add(field);

if (fieldType().hasDocValues() == false && fieldType.omitNorms()) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ protected void index(ParseContext context, ShapeBuilder<?, ?, ?> shapeBuilder) t
}
}
context.doc().addAll(Arrays.asList(fieldType().defaultPrefixTreeStrategy().createIndexableFields(shape)));
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,7 @@ private void indexValue(ParseContext context, Number numericValue) {
indexed, hasDocValues, stored));

if (hasDocValues == false && (stored || indexed)) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,16 @@ public void addIgnoredField(String field) {
public Collection<String> getIgnoredFields() {
return in.getIgnoredFields();
}

@Override
public void addFieldExistsField(String field) {
in.addFieldExistsField(field);
}

@Override
public Collection<String> getFieldExistsFields() {
return in.getFieldExistsFields();
}
}

public static class InternalParseContext extends ParseContext {
Expand All @@ -307,6 +317,7 @@ public static class InternalParseContext extends ParseContext {
private final Map<String, ObjectMapper> dynamicObjectMappers = new HashMap<>();
private final List<RuntimeField> dynamicRuntimeFields = new ArrayList<>();
private final Set<String> ignoredFields = new HashSet<>();
private final Set<String> fieldNameFields = new HashSet<>();
private Field version;
private SeqNoFieldMapper.SequenceIDFields seqID;
private long numNestedDocs;
Expand Down Expand Up @@ -490,6 +501,16 @@ public void addIgnoredField(String field) {
public Collection<String> getIgnoredFields() {
return Collections.unmodifiableCollection(ignoredFields);
}

@Override
public void addFieldExistsField(String field) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I slightly prefer the previous name, shall we call it addFieldToFieldNames ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up with addtoFieldNames

fieldNameFields.add(field);
}

@Override
public Collection<String> getFieldExistsFields() {
Copy link
Member

Choose a reason for hiding this comment

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

getFieldNames?

return Collections.unmodifiableCollection(fieldNameFields);
}
}

/**
Expand All @@ -509,6 +530,19 @@ public Collection<String> getIgnoredFields() {
*/
public abstract Collection<String> getIgnoredFields();

/**
* Add the given {@code field} to the _fieldnames field
*
* Use this if an exists query run against the field cannot use docvalues
* or norms.
*/
public abstract void addFieldExistsField(String field);

/**
* Return the collection of fields to be added to the _fieldnames field
*/
public abstract Collection<String> getFieldExistsFields();

public abstract Mapper.TypeParser.ParserContext parserContext(DateFormatter dateFormatter);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
context.doc().addAll(fieldType().rangeType.createFields(context, name(), range, index, hasDocValues, store));

if (hasDocValues == false && (index || store)) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public void preParse(ParseContext context) {
String routing = context.sourceToParse().routing();
if (routing != null) {
context.doc().add(new Field(fieldType().name(), routing, Defaults.FIELD_TYPE));
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -826,7 +826,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
Field field = new Field(fieldType().name(), value, fieldType);
context.doc().add(field);
if (fieldType.omitNorms()) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
if (prefixFieldInfo != null) {
context.doc().add(new Field(prefixFieldInfo.field, value, prefixFieldInfo.fieldType));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
context.doc().addAll(fieldParser.parse(xContentParser));

if (mappedFieldType.hasDocValues() == false) {
createFieldNamesField(context);
context.addFieldExistsField(fieldType().name());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,6 @@

public class FieldNamesFieldMapperTests extends MapperServiceTestCase {

private static SortedSet<String> extract(String path) {
SortedSet<String> set = new TreeSet<>();
for (String fieldName : FieldNamesFieldMapper.extractFieldNames(path)) {
set.add(fieldName);
}
return set;
}

private static SortedSet<String> set(String... values) {
return new TreeSet<>(Arrays.asList(values));
}
Expand All @@ -41,16 +33,6 @@ void assertFieldNames(Set<String> expected, ParsedDocument doc) {
assertEquals(expected, set(got));
}

public void testExtractFieldNames() {
assertEquals(set("abc"), extract("abc"));
assertEquals(set("a", "a.b"), extract("a.b"));
assertEquals(set("a", "a.b", "a.b.c"), extract("a.b.c"));
// and now corner cases
assertEquals(set("", ".a"), extract(".a"));
assertEquals(set("a", "a."), extract("a."));
assertEquals(set("", ".", ".."), extract(".."));
}

public void testFieldType() throws Exception {
DocumentMapper docMapper = createDocumentMapper(mapping(b -> {}));
FieldNamesFieldMapper fieldNamesMapper = docMapper.metadataMapper(FieldNamesFieldMapper.class);
Expand Down
Loading