Skip to content

Commit 332a503

Browse files
nik9000georgewallace
authored andcommitted
Speed up stored field spec management (elastic#122645)
When you load stored fields from lucene you have get all of your ducks in a row first or it'll be really slow. You need to get a list of all of the fields you want so you can ignore the ones that you don't. We do this with the `StoredFieldsSpec` which is immutable and has a `merge` method. It's quite common to merge a few dozen of these specs together to prepare for the fetch phase or for a new segment when loading fields from ESQL. When I was loading thousands of fields in ESQL I noticed that the merge was slowing things down marginally. This skips a big chunk of the merge in the common case that we don't have to load any named stored fields.
1 parent bd9f6c8 commit 332a503

File tree

2 files changed

+25
-2
lines changed

2 files changed

+25
-2
lines changed

server/src/main/java/org/elasticsearch/search/fetch/StoredFieldsSpec.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,17 @@ public StoredFieldsSpec merge(StoredFieldsSpec other) {
4242
if (this == other) {
4343
return this;
4444
}
45-
Set<String> mergedFields = new HashSet<>(this.requiredStoredFields);
46-
mergedFields.addAll(other.requiredStoredFields);
45+
Set<String> mergedFields;
46+
if (other.requiredStoredFields.isEmpty()) {
47+
/*
48+
* In the very very common case that we don't need new stored fields
49+
* let's not clone the existing array.
50+
*/
51+
mergedFields = this.requiredStoredFields;
52+
} else {
53+
mergedFields = new HashSet<>(this.requiredStoredFields);
54+
mergedFields.addAll(other.requiredStoredFields);
55+
}
4756
return new StoredFieldsSpec(
4857
this.requiresSource || other.requiresSource,
4958
this.requiresMetadata || other.requiresMetadata,

server/src/test/java/org/elasticsearch/search/fetch/StoredFieldsSpecTests.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@
1818
import org.elasticsearch.search.internal.SearchContext;
1919
import org.elasticsearch.test.ESTestCase;
2020

21+
import java.util.Set;
22+
2123
import static org.hamcrest.Matchers.hasSize;
24+
import static org.hamcrest.Matchers.sameInstance;
2225
import static org.mockito.Mockito.mock;
2326
import static org.mockito.Mockito.when;
2427

@@ -67,6 +70,17 @@ public void testScriptFieldsEnableMetadata() {
6770
assertTrue(spec.requiresMetadata());
6871
}
6972

73+
public void testNoCloneOnMerge() {
74+
StoredFieldsSpec spec = StoredFieldsSpec.NO_REQUIREMENTS;
75+
spec = spec.merge(StoredFieldsSpec.NEEDS_SOURCE);
76+
assertThat(spec.requiredStoredFields(), sameInstance(StoredFieldsSpec.NO_REQUIREMENTS.requiredStoredFields()));
77+
78+
StoredFieldsSpec needsCat = new StoredFieldsSpec(false, false, Set.of("cat"));
79+
StoredFieldsSpec withCat = spec.merge(needsCat);
80+
spec = withCat.merge(StoredFieldsSpec.NO_REQUIREMENTS);
81+
assertThat(spec.requiredStoredFields(), sameInstance(withCat.requiredStoredFields()));
82+
}
83+
7084
private static SearchContext searchContext(SearchSourceBuilder sourceBuilder) {
7185
SearchContext sc = mock(SearchContext.class);
7286
when(sc.fetchSourceContext()).thenReturn(sourceBuilder.fetchSource());

0 commit comments

Comments
 (0)