Skip to content
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
5 changes: 5 additions & 0 deletions docs/changelog/134629.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 134629
summary: Improve block loader for source only runtime fields of type double
area: Mapping
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import java.time.ZoneId;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
Expand Down Expand Up @@ -109,7 +110,35 @@ public DocValueFormat docValueFormat(String format, ZoneId timeZone) {

@Override
public BlockLoader blockLoader(BlockLoaderContext blContext) {
return new DoubleScriptBlockDocValuesReader.DoubleScriptBlockLoader(leafFactory(blContext.lookup()));
var indexSettings = blContext.indexSettings();
if (isParsedFromSource && indexSettings.getIndexMappingSourceMode() == SourceFieldMapper.Mode.SYNTHETIC
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation

Copy link
Member Author

Choose a reason for hiding this comment

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

This is how spotless formatted it :) I fixed the indentation, but since I copied from long script field type, I generalized the logic, it should be reusable for other runtime field types as well.

// A runtime and normal field can share the same name.
// In that case there is no ignored source entry, and so we need to fail back to LongScriptBlockLoader.
// We could optimize this, but at this stage feels like a rare scenario.
&& blContext.lookup().onlyMappedAsRuntimeField(name())) {
var reader = new NumberType.NumberFallbackSyntheticSourceReader(NumberType.DOUBLE, null, true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test for this code path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is tested in the testBlockLoaderSourceOnlyRuntimeFieldWithSyntheticSource() test, which is also included in this change.

@Override
public void writeToBlock(List<Number> values, BlockLoader.Builder blockBuilder) {
var builder = (BlockLoader.DoubleBuilder) blockBuilder;
for (var value : values) {
builder.appendDouble(value.doubleValue());
}
}
};

return new FallbackSyntheticSourceBlockLoader(
reader,
name(),
IgnoredSourceFieldMapper.ignoredSourceFormat(indexSettings.getIndexVersionCreated())
) {
@Override
public Builder builder(BlockFactory factory, int expectedCount) {
return factory.doubles(expectedCount);
}
};
} else {
return new DoubleScriptBlockDocValuesReader.DoubleScriptBlockLoader(leafFactory(blContext.lookup()));
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.lucene.tests.index.RandomIndexWriter;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.lucene.search.function.ScriptScoreQuery;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexVersion;
import org.elasticsearch.index.fielddata.DoubleScriptFieldData;
import org.elasticsearch.index.fielddata.ScriptDocValues;
Expand All @@ -40,15 +41,19 @@
import org.elasticsearch.script.ScriptFactory;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.MultiValueMode;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static java.util.Collections.emptyMap;
import static org.elasticsearch.index.mapper.LongScriptFieldTypeTests.createDocumentWithIgnoredSource;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.nullValue;

public class DoubleScriptFieldTypeTests extends AbstractNonTextScriptFieldTypeTestCase {

Expand Down Expand Up @@ -269,6 +274,69 @@ public void testBlockLoader() throws IOException {
}
}

public void testBlockLoaderSourceOnlyRuntimeField() throws IOException {
try (
Directory directory = newDirectory();
RandomIndexWriter iw = new RandomIndexWriter(random(), directory, newIndexWriterConfig().setMergePolicy(NoMergePolicy.INSTANCE))
) {
iw.addDocuments(
List.of(
List.of(new StoredField("_source", new BytesRef("{\"test\": [1.1]}"))),
List.of(new StoredField("_source", new BytesRef("{\"test\": [2.1]}")))
)
);
try (DirectoryReader reader = iw.getReader()) {
DoubleScriptFieldType fieldType = simpleSourceOnlyMappedFieldType();

// Assert implementations:
BlockLoader loader = fieldType.blockLoader(blContext(Settings.EMPTY, true));
assertThat(loader, instanceOf(DoubleScriptBlockDocValuesReader.DoubleScriptBlockLoader.class));
// ignored source doesn't support column at a time loading:
var columnAtATimeLoader = loader.columnAtATimeReader(reader.leaves().getFirst());
assertThat(columnAtATimeLoader, instanceOf(DoubleScriptBlockDocValuesReader.class));
var rowStrideReader = loader.rowStrideReader(reader.leaves().getFirst());
assertThat(rowStrideReader, instanceOf(DoubleScriptBlockDocValuesReader.class));

// Assert values:
assertThat(blockLoaderReadValuesFromColumnAtATimeReader(reader, fieldType, 0), equalTo(List.of(1.1, 2.1)));
assertThat(blockLoaderReadValuesFromColumnAtATimeReader(reader, fieldType, 1), equalTo(List.of(2.1)));
assertThat(blockLoaderReadValuesFromRowStrideReader(reader, fieldType), equalTo(List.of(1.1, 2.1)));
}
}
}

public void testBlockLoaderSourceOnlyRuntimeFieldWithSyntheticSource() throws IOException {
var settings = Settings.builder().put("index.mapping.source.mode", "synthetic").build();
try (
Directory directory = newDirectory();
RandomIndexWriter iw = new RandomIndexWriter(random(), directory, newIndexWriterConfig().setMergePolicy(NoMergePolicy.INSTANCE))
) {

var document1 = createDocumentWithIgnoredSource("[1.1]");
var document2 = createDocumentWithIgnoredSource("[2.1]");

iw.addDocuments(List.of(document1, document2));
try (DirectoryReader reader = iw.getReader()) {
DoubleScriptFieldType fieldType = simpleSourceOnlyMappedFieldType();

// Assert implementations:
BlockLoader loader = fieldType.blockLoader(blContext(settings, true));
assertThat(loader, instanceOf(FallbackSyntheticSourceBlockLoader.class));
// ignored source doesn't support column at a time loading:
var columnAtATimeLoader = loader.columnAtATimeReader(reader.leaves().getFirst());
assertThat(columnAtATimeLoader, nullValue());
var rowStrideReader = loader.rowStrideReader(reader.leaves().getFirst());
assertThat(
rowStrideReader.getClass().getName(),
equalTo("org.elasticsearch.index.mapper.FallbackSyntheticSourceBlockLoader$IgnoredSourceRowStrideReader")
);

// Assert values:
assertThat(blockLoaderReadValuesFromRowStrideReader(settings, reader, fieldType, true), equalTo(List.of(1.1, 2.1)));
}
}
}

@Override
protected Query randomTermsQuery(MappedFieldType ft, SearchExecutionContext ctx) {
return ft.termsQuery(List.of(randomLong()), ctx);
Expand All @@ -279,6 +347,10 @@ protected DoubleScriptFieldType simpleMappedFieldType() {
return build("read_foo", Map.of(), OnScriptError.FAIL);
}

private DoubleScriptFieldType simpleSourceOnlyMappedFieldType() {
return build("read_test", Map.of(), OnScriptError.FAIL);
}

@Override
protected MappedFieldType loopFieldType() {
return build("loop", Map.of(), OnScriptError.FAIL);
Expand All @@ -296,6 +368,31 @@ protected DoubleScriptFieldType build(String code, Map<String, Object> params, O

private static DoubleFieldScript.Factory factory(Script script) {
return switch (script.getIdOrCode()) {
case "read_test" -> new DoubleFieldScript.Factory() {
@Override
public DoubleFieldScript.LeafFactory newFactory(
String fieldName,
Map<String, Object> params,
SearchLookup lookup,
OnScriptError onScriptError
) {
return (ctx) -> new DoubleFieldScript(fieldName, params, lookup, onScriptError, ctx) {
@Override
@SuppressWarnings("unchecked")
public void execute() {
Map<String, Object> source = (Map<String, Object>) this.getParams().get("_source");
for (Object foo : (List<?>) source.get("test")) {
emit(((Number) foo).doubleValue());
}
};
};
}

@Override
public boolean isParsedFromSource() {
return true;
}
};
case "read_foo" -> (fieldName, params, lookup, onScriptError) -> (ctx) -> new DoubleFieldScript(
fieldName,
params,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public void testBlockLoaderSourceOnlyRuntimeFieldWithSyntheticSource() throws IO
}
}

private static LuceneDocument createDocumentWithIgnoredSource(String bytes) throws IOException {
static LuceneDocument createDocumentWithIgnoredSource(String bytes) throws IOException {
var doc = new LuceneDocument();
var parser = XContentHelper.createParser(
XContentParserConfiguration.EMPTY,
Expand Down