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.

Sign up for GitHub

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

Jump to bottom

Implement fields fetch for runtime fields #61995

Merged
merged 27 commits into from
Sep 15, 2020
Merged

Implement fields fetch for runtime fields #61995

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
17f46a6
Implement fields fetch for runtime fields
nik9000 Sep 3, 2020
9be09d7
Doc
nik9000 Sep 4, 2020
186a911
Merge branch 'master' into runtime_fields_fetch_fields_take_two
nik9000 Sep 4, 2020
9b4b344
Use clean nested context
nik9000 Sep 4, 2020
9565d7d
Date nanos!
nik9000 Sep 4, 2020
ef3b2a4
Update test now that we force the right resolution in the formatter
nik9000 Sep 4, 2020
af9da06
Delay building all fetchers
nik9000 Sep 4, 2020
56405f5
Fixup
nik9000 Sep 4, 2020
e494412
Map
nik9000 Sep 4, 2020
7099198
Merge branch 'master' into runtime_fields_fetch_fields_take_two
nik9000 Sep 8, 2020
6313a12
Pass lookup when building
nik9000 Sep 8, 2020
6a6584b
Cleanup
nik9000 Sep 8, 2020
4e5bd65
Close over lookup
nik9000 Sep 8, 2020
c668838
Remove oops
nik9000 Sep 9, 2020
103df6a
Something is up with dates
nik9000 Sep 9, 2020
c63a886
Revert "Map"
nik9000 Sep 9, 2020
d7315fe
Merge branch 'master' into runtime_fields_fetch_fields_take_two
nik9000 Sep 10, 2020
f92f8a5
Revert "Close over lookup"
nik9000 Sep 10, 2020
122663c
Fetch lookup
nik9000 Sep 10, 2020
77f130c
Rename k plz
nik9000 Sep 10, 2020
0fe6b3d
Merge branch 'master' into runtime_fields_fetch_fields_take_two
nik9000 Sep 10, 2020
3790f5e
Compile plz
nik9000 Sep 10, 2020
459808f
Merge branch 'master' into runtime_fields_fetch_fields_take_two
nik9000 Sep 15, 2020
2055c32
Compile
nik9000 Sep 15, 2020
d9e6209
Iter
nik9000 Sep 15, 2020
137ac90
oops
nik9000 Sep 15, 2020
d666f3f
Merge branch 'master' into runtime_fields_fetch_fields_take_two
nik9000 Sep 15, 2020
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
2 changes: 1 addition & 1 deletion ...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ private Float objectToFloat(Object value) {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
2 changes: 1 addition & 1 deletion ...s/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
2 changes: 1 addition & 1 deletion ...es/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ private static double objectToDouble(Object value) {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
7 changes: 4 additions & 3 deletions ...apper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.similarity.SimilarityProvider;
import org.elasticsearch.index.similarity.SimilarityService;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -419,7 +420,7 @@ protected void parseCreateField(ParseContext context) {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
throw new UnsupportedOperationException();
}

Expand Down Expand Up @@ -465,7 +466,7 @@ protected void mergeOptions(FieldMapper other, List<String> conflicts) {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
throw new UnsupportedOperationException();
}

Expand Down Expand Up @@ -588,7 +589,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
throw new UnsupportedOperationException();
}

Expand Down
3 changes: 2 additions & 1 deletion ...les/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.lucene.document.FieldType;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.analysis.NamedAnalyzer;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.util.Iterator;
Expand Down Expand Up @@ -159,7 +160,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
2 changes: 1 addition & 1 deletion ...pper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
e.getCause().getMessage());
}

public void testFetchSourceValue() {
public void testFetchSourceValue() throws IOException {
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());
RankFeatureFieldMapper mapper = new RankFeatureFieldMapper.Builder("field").build(context);
Expand Down
2 changes: 1 addition & 1 deletion ...pper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public void testRejectIndexOptions() {
containsString("Failed to parse mapping: unknown parameter [index_options] on mapper [field] of type [scaled_float]"));
}

public void testFetchSourceValue() {
public void testFetchSourceValue() throws IOException {
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());

Expand Down
2 changes: 1 addition & 1 deletion modules/parent-join/src/main/java/org/elasticsearch/join/mapper/MetaJoinFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
throw new UnsupportedOperationException("Cannot fetch values for metadata field [" + typeName() + "].");
}

Expand Down
2 changes: 1 addition & 1 deletion modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentIdFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
throw new UnsupportedOperationException("Cannot fetch values for internal field [" + typeName() + "].");
}

Expand Down
2 changes: 1 addition & 1 deletion modules/parent-join/src/main/java/org/elasticsearch/join/mapper/ParentJoinFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
3 changes: 2 additions & 1 deletion modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
import org.elasticsearch.index.query.QueryShardException;
import org.elasticsearch.index.query.Rewriteable;
import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand Down Expand Up @@ -366,7 +367,7 @@ public void parse(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
6 changes: 4 additions & 2 deletions ...rcolator/src/main/java/org/elasticsearch/percolator/PercolatorHighlightSubFetchPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
import org.elasticsearch.search.fetch.subphase.highlight.Highlighter;
import org.elasticsearch.search.fetch.subphase.highlight.SearchHighlightContext;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.search.lookup.SearchLookup;
import org.elasticsearch.search.lookup.SourceLookup;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -54,7 +56,7 @@ final class PercolatorHighlightSubFetchPhase implements FetchSubPhase {
}

@Override
public FetchSubPhaseProcessor getProcessor(SearchContext searchContext) throws IOException {
public FetchSubPhaseProcessor getProcessor(SearchContext searchContext, SearchLookup lookup) throws IOException {
if (searchContext.highlight() == null) {
return null;
}
Expand Down Expand Up @@ -95,7 +97,7 @@ public void process(HitContext hit) throws IOException {
BytesReference document = percolateQuery.getDocuments().get(slot);
HitContext subContext = new HitContext(
new SearchHit(slot, "unknown", Collections.emptyMap(), Collections.emptyMap()),
percolatorLeafReaderContext, slot, percolatorIndexSearcher, new HashMap<>()
percolatorLeafReaderContext, slot, percolatorIndexSearcher, new SourceLookup(), new HashMap<>()
);
subContext.sourceLookup().setSource(document);
// force source because MemoryIndex does not store fields
Expand Down
3 changes: 2 additions & 1 deletion ...olator/src/main/java/org/elasticsearch/percolator/PercolatorMatchedSlotSubFetchPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.elasticsearch.search.fetch.FetchSubPhase;
import org.elasticsearch.search.fetch.FetchSubPhaseProcessor;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -56,7 +57,7 @@ final class PercolatorMatchedSlotSubFetchPhase implements FetchSubPhase {
static final String FIELD_NAME_PREFIX = "_percolator_document_slot";

@Override
public FetchSubPhaseProcessor getProcessor(SearchContext searchContext) throws IOException {
public FetchSubPhaseProcessor getProcessor(SearchContext searchContext, SearchLookup lookup) throws IOException {

List<PercolateContext> percolateContexts = new ArrayList<>();
List<PercolateQuery> percolateQueries = locatePercolatorQuery(searchContext.query());
Expand Down
4 changes: 2 additions & 2 deletions ...tor/src/test/java/org/elasticsearch/percolator/PercolatorHighlightSubFetchPhaseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ public void testHitsExecutionNeeded() throws IOException {
Mockito.when(searchContext.highlight()).thenReturn(new SearchHighlightContext(Collections.emptyList()));
Mockito.when(searchContext.query()).thenReturn(new MatchAllDocsQuery());

assertNull(subFetchPhase.getProcessor(searchContext));
assertNull(subFetchPhase.getProcessor(searchContext, null));
Mockito.when(searchContext.query()).thenReturn(percolateQuery);
assertNotNull(subFetchPhase.getProcessor(searchContext));
assertNotNull(subFetchPhase.getProcessor(searchContext, null));
}

public void testLocatePercolatorQuery() {
Expand Down
13 changes: 7 additions & 6 deletions ...r/src/test/java/org/elasticsearch/percolator/PercolatorMatchedSlotSubFetchPhaseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.elasticsearch.search.fetch.FetchSubPhase.HitContext;
import org.elasticsearch.search.fetch.FetchSubPhaseProcessor;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.search.lookup.SourceLookup;
import org.elasticsearch.test.ESTestCase;

import java.util.Collections;
Expand All @@ -66,7 +67,7 @@ public void testHitsExecute() throws Exception {
LeafReaderContext context = reader.leaves().get(0);
// A match:
{
HitContext hit = new HitContext(new SearchHit(0), context, 0, null, new HashMap<>());
HitContext hit = new HitContext(new SearchHit(0), context, 0, null, new SourceLookup(), new HashMap<>());
PercolateQuery.QueryStore queryStore = ctx -> docId -> new TermQuery(new Term("field", "value"));
MemoryIndex memoryIndex = new MemoryIndex();
memoryIndex.addField("field", "value", new WhitespaceAnalyzer());
Expand All @@ -77,7 +78,7 @@ public void testHitsExecute() throws Exception {
SearchContext sc = mock(SearchContext.class);
when(sc.query()).thenReturn(percolateQuery);

FetchSubPhaseProcessor processor = phase.getProcessor(sc);
FetchSubPhaseProcessor processor = phase.getProcessor(sc, null);
assertNotNull(processor);
processor.process(hit);

Expand All @@ -87,7 +88,7 @@ public void testHitsExecute() throws Exception {

// No match:
{
HitContext hit = new HitContext(new SearchHit(0), context, 0, null, new HashMap<>());
HitContext hit = new HitContext(new SearchHit(0), context, 0, null, new SourceLookup(), new HashMap<>());
PercolateQuery.QueryStore queryStore = ctx -> docId -> new TermQuery(new Term("field", "value"));
MemoryIndex memoryIndex = new MemoryIndex();
memoryIndex.addField("field", "value1", new WhitespaceAnalyzer());
Expand All @@ -98,7 +99,7 @@ public void testHitsExecute() throws Exception {
SearchContext sc = mock(SearchContext.class);
when(sc.query()).thenReturn(percolateQuery);

FetchSubPhaseProcessor processor = phase.getProcessor(sc);
FetchSubPhaseProcessor processor = phase.getProcessor(sc, null);
assertNotNull(processor);
processor.process(hit);

Expand All @@ -107,7 +108,7 @@ public void testHitsExecute() throws Exception {

// No query:
{
HitContext hit = new HitContext(new SearchHit(0), context, 0, null, new HashMap<>());
HitContext hit = new HitContext(new SearchHit(0), context, 0, null, new SourceLookup(), new HashMap<>());
PercolateQuery.QueryStore queryStore = ctx -> docId -> null;
MemoryIndex memoryIndex = new MemoryIndex();
memoryIndex.addField("field", "value", new WhitespaceAnalyzer());
Expand All @@ -118,7 +119,7 @@ public void testHitsExecute() throws Exception {
SearchContext sc = mock(SearchContext.class);
when(sc.query()).thenReturn(percolateQuery);

FetchSubPhaseProcessor processor = phase.getProcessor(sc);
FetchSubPhaseProcessor processor = phase.getProcessor(sc, null);
assertNotNull(processor);
processor.process(hit);

Expand Down
2 changes: 1 addition & 1 deletion ...ysis-icu/src/main/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
2 changes: 1 addition & 1 deletion ...icu/src/test/java/org/elasticsearch/index/mapper/ICUCollationKeywordFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ public void testUpdateIgnoreAbove() throws IOException {
assertEquals(0, fields.length);
}

public void testFetchSourceValue() {
public void testFetchSourceValue() throws IOException {
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());

Expand Down
2 changes: 1 addition & 1 deletion ...Test/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ public void testEmptyName() throws IOException {
assertThat(e.getMessage(), containsString("name cannot be empty string"));
}

public void testFetchSourceValue() {
public void testFetchSourceValue() throws IOException {
Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT.id).build();
Mapper.BuilderContext context = new Mapper.BuilderContext(settings, new ContentPath());

Expand Down
3 changes: 2 additions & 1 deletion .../src/main/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.elasticsearch.index.mapper.ValueFetcher;
import org.elasticsearch.index.mapper.annotatedtext.AnnotatedTextFieldMapper.AnnotatedText.AnnotationToken;
import org.elasticsearch.index.similarity.SimilarityProvider;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.io.Reader;
Expand Down Expand Up @@ -589,7 +590,7 @@ protected void parseCreateField(ParseContext context) throws IOException {
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
2 changes: 1 addition & 1 deletion ...pper-murmur3/src/main/java/org/elasticsearch/index/mapper/murmur3/Murmur3FieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ protected void parseCreateField(ParseContext context)
}

@Override
public ValueFetcher valueFetcher(MapperService mapperService, String format) {
public ValueFetcher valueFetcher(MapperService mapperService, SearchLookup searchLookup, String format) {
if (format != null) {
throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] doesn't support formats.");
}
Expand Down
11 changes: 9 additions & 2 deletions rest-api-spec/src/main/resources/rest-api-spec/test/search/240_date_nanos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,15 @@ setup:
rest_total_hits_as_int: true
index: date*
body:
docvalue_fields: [ { "field": "date", "format" : "strict_date_optional_time" }, { "field": "date", "format": "epoch_millis" }, { "field" : "date", "format": "uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSSX" } ]
sort: [ { "date": "desc" } ]
docvalue_fields:
- field: date
format: strict_date_optional_time
- field: date
format: epoch_millis
- field: date
format: uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSSX
sort:
- date: desc

- match: { hits.total: 2 }
- length: { hits.hits: 2 }
Expand Down
3 changes: 2 additions & 1 deletion ...er/src/internalClusterTest/java/org/elasticsearch/search/fetch/FetchSubPhasePluginIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.elasticsearch.search.SearchExtBuilder;
import org.elasticsearch.search.builder.SearchSourceBuilder;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.search.lookup.SearchLookup;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
import org.elasticsearch.test.ESIntegTestCase.Scope;
Expand Down Expand Up @@ -114,7 +115,7 @@ private static final class TermVectorsFetchSubPhase implements FetchSubPhase {
private static final String NAME = "term_vectors_fetch";

@Override
public FetchSubPhaseProcessor getProcessor(SearchContext searchContext) {
public FetchSubPhaseProcessor getProcessor(SearchContext searchContext, SearchLookup lookup) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if in all places where we carry around the search lookup, we should not rather make it a supplier like we did in fielddataBuilder.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we did the Supplier we could automatically forkAndTrackReferences when you call get on it which is kind of neat. I like it!

Copy link
Member

Choose a reason for hiding this comment

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

@jtibshirani what do you think? I was under the impression that the lookup is not used in many places, and maybe we could / should create it lazily. We do that already in fielddataBuilder, though it is some indirection that may make the code harder to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm - I don't think we can know from the signature how to fork....

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait wait wait, we don't need to forkAndTrackReferences because getting doc values does that for us. Ignore that. I think we're actually fine without the Supplier then. Because the only thing it'd let us do is lazily build the SearchLookup but that class isn't that heavy so I don't think it is worth it. And we need the SourceLookup anyway because the fetch phase is pretty centralized around it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly dumb question - is it worth putting it into the SearchContext, rather than having it as an extra parameter here?

Copy link
Member

Choose a reason for hiding this comment

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

It is all but a dumb question to me. In fact when I saw the new argument the first thing I did was checking that it was not already available in search context :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to add it to SearchContext because it'd have a huge blast radius. I think it'd be reasonable to make a FetchContext which has both things. But I don't think it is worth it because there are only 2 arguments..

Copy link
Contributor

Choose a reason for hiding this comment

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

FetchContext might be an interesting followup, but sure let's leave it for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if we did FetchContext I'd want it to be smaller than the SearchContext. That thing is just too big....

return new FetchSubPhaseProcessor() {
@Override
public void setNextReader(LeafReaderContext readerContext) {
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
import org.elasticsearch.index.fielddata.fieldcomparator.LongValuesComparatorSource;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.MultiValueMode;
import org.elasticsearch.search.sort.BucketedSort;
import org.elasticsearch.search.sort.SortOrder;
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;
import org.elasticsearch.search.sort.BucketedSort;
import org.elasticsearch.search.sort.SortOrder;

import java.io.IOException;
import java.util.function.LongUnaryOperator;
Expand Down
26 changes: 26 additions & 0 deletions server/src/main/java/org/elasticsearch/index/fielddata/LeafFieldData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@

import org.apache.lucene.util.Accountable;
import org.elasticsearch.common.lease.Releasable;
import org.elasticsearch.index.mapper.DocValueFetcher;
import org.elasticsearch.search.DocValueFormat;

import java.io.IOException;

/**
* The thread safe {@link org.apache.lucene.index.LeafReader} level cache of the data.
Expand All @@ -37,4 +41,26 @@ public interface LeafFieldData extends Accountable, Releasable {
*/
SortedBinaryDocValues getBytesValues();

/**
* Return a value fetcher for this leaf implementation.
*/
default DocValueFetcher.Leaf getLeafValueFetcher(DocValueFormat format) {
Copy link
Member

Choose a reason for hiding this comment

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

this looks much better, thank you!

SortedBinaryDocValues values = getBytesValues();
return new DocValueFetcher.Leaf() {
javanna marked this conversation as resolved.
Show resolved Hide resolved
@Override
public boolean advanceExact(int docId) throws IOException {
return values.advanceExact(docId);
}

@Override
public int docValueCount() throws IOException {
return values.docValueCount();
}

@Override
public Object nextValue() throws IOException {
return format.format(values.nextValue());
}
};
}
}
Loading