Skip to content

Commit c832243

Browse files
jordan-powersjordan-powers
authored
Combine ignored fields spec with source paths (#136771)
With the recent addition of StoredFieldsSpec.sourcePaths, it is redundant to have a separate property for ignored source spec. This PR removes IgnoredSourceSpec and uses StoredFieldsSpec.sourcePaths instead.
1 parent e7a4eec commit c832243

File tree

8 files changed

+67
-120
lines changed

8 files changed

+67
-120
lines changed

‎server/src/main/java/org/elasticsearch/index/fieldvisitor/IgnoredSourceFieldLoader.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,11 @@ class IgnoredSourceFieldLoader extends StoredFieldLoader {
3535
IgnoredSourceFieldLoader(StoredFieldsSpec spec, boolean forceSequentialReader) {
3636
assert IgnoredSourceFieldLoader.supports(spec);
3737

38-
fieldNames = new HashSet<>(spec.ignoredFieldsSpec().requiredIgnoredFields());
38+
fieldNames = new HashSet<>(spec.sourcePaths());
3939
this.forceSequentialReader = forceSequentialReader;
4040

4141
HashMap<String, Set<String>> potentialFieldsInIgnoreSource = new HashMap<>();
42-
for (String requiredIgnoredField : spec.ignoredFieldsSpec().requiredIgnoredFields()) {
42+
for (String requiredIgnoredField : spec.sourcePaths()) {
4343
for (String potentialIgnoredField : FallbackSyntheticSourceBlockLoader.splitIntoFieldPaths(requiredIgnoredField)) {
4444
potentialFieldsInIgnoreSource.computeIfAbsent(potentialIgnoredField, k -> new HashSet<>()).add(requiredIgnoredField);
4545
}
@@ -142,7 +142,7 @@ void reset() {
142142
}
143143

144144
static boolean supports(StoredFieldsSpec spec) {
145-
return spec.onlyRequiresIgnoredFields()
146-
&& spec.ignoredFieldsSpec().format() == IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE;
145+
return spec.onlyRequiresSourcePaths()
146+
&& spec.ignoredSourceFormat() == IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE;
147147
}
148148
}

‎server/src/main/java/org/elasticsearch/index/mapper/FallbackSyntheticSourceBlockLoader.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public RowStrideReader rowStrideReader(LeafReaderContext context) throws IOExcep
6565

6666
@Override
6767
public StoredFieldsSpec rowStrideStoredFieldSpec() {
68-
return new StoredFieldsSpec(false, false, Set.of(), new IgnoredFieldsSpec(Set.of(fieldName), ignoredSourceFormat), Set.of());
68+
return StoredFieldsSpec.withSourcePaths(ignoredSourceFormat, Set.of(fieldName));
6969
}
7070

7171
@Override

‎server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldsSpec.java‎

Lines changed: 0 additions & 58 deletions
This file was deleted.

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

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
package org.elasticsearch.search.fetch;
1111

12-
import org.elasticsearch.index.mapper.IgnoredFieldsSpec;
12+
import org.elasticsearch.index.mapper.IgnoredSourceFieldMapper;
1313
import org.elasticsearch.index.mapper.IgnoredSourceFieldMapper.IgnoredSourceFormat;
1414

1515
import java.util.Collection;
@@ -26,22 +26,19 @@ public record StoredFieldsSpec(
2626
boolean requiresSource,
2727
boolean requiresMetadata,
2828
Set<String> requiredStoredFields,
29-
IgnoredFieldsSpec ignoredFieldsSpec,
29+
IgnoredSourceFormat ignoredSourceFormat,
3030
Set<String> sourcePaths
3131
) {
3232
public StoredFieldsSpec(boolean requiresSource, boolean requiresMetadata, Set<String> requiredStoredFields) {
33-
this(requiresSource, requiresMetadata, requiredStoredFields, IgnoredFieldsSpec.NONE, Set.of());
33+
this(requiresSource, requiresMetadata, requiredStoredFields, IgnoredSourceFormat.NO_IGNORED_SOURCE, Set.of());
3434
}
3535

3636
public boolean noRequirements() {
37-
return requiresSource == false && requiresMetadata == false && requiredStoredFields.isEmpty() && ignoredFieldsSpec.noRequirements();
37+
return requiresSource == false && requiresMetadata == false && requiredStoredFields.isEmpty() && sourcePaths.isEmpty();
3838
}
3939

40-
public boolean onlyRequiresIgnoredFields() {
41-
return requiresSource == false
42-
&& requiresMetadata == false
43-
&& requiredStoredFields.isEmpty()
44-
&& ignoredFieldsSpec.noRequirements() == false;
40+
public boolean onlyRequiresSourcePaths() {
41+
return requiresSource && requiresMetadata == false && requiredStoredFields.isEmpty() && sourcePaths.isEmpty() == false;
4542
}
4643

4744
/**
@@ -59,11 +56,7 @@ public boolean onlyRequiresIgnoredFields() {
5956
* This is more efficient than using {@link #NEEDS_SOURCE}.
6057
*/
6158
public static StoredFieldsSpec withSourcePaths(IgnoredSourceFormat ignoredSourceFormat, Set<String> sourcePaths) {
62-
// The fields in source paths might also be in ignored source, so include source paths there as well.
63-
IgnoredFieldsSpec ignoredFieldsSpec = ignoredSourceFormat == IgnoredSourceFormat.NO_IGNORED_SOURCE
64-
? IgnoredFieldsSpec.NONE
65-
: new IgnoredFieldsSpec(sourcePaths, ignoredSourceFormat);
66-
return new StoredFieldsSpec(true, false, Set.of(), ignoredFieldsSpec, sourcePaths);
59+
return new StoredFieldsSpec(true, false, Set.of(), ignoredSourceFormat, sourcePaths);
6760
}
6861

6962
/**
@@ -95,24 +88,41 @@ public StoredFieldsSpec merge(StoredFieldsSpec other) {
9588
} else {
9689
mergedSourcePaths = Set.of();
9790
}
91+
IgnoredSourceFormat mergedFormat;
92+
if (this.ignoredSourceFormat == IgnoredSourceFormat.NO_IGNORED_SOURCE) {
93+
mergedFormat = other.ignoredSourceFormat;
94+
} else if (other.ignoredSourceFormat == IgnoredSourceFormat.NO_IGNORED_SOURCE) {
95+
mergedFormat = this.ignoredSourceFormat;
96+
} else if (this.ignoredSourceFormat != other.ignoredSourceFormat) {
97+
throw new IllegalStateException(
98+
"failed to merge IgnoredFieldsSpec with differing formats ["
99+
+ this.ignoredSourceFormat.name()
100+
+ ","
101+
+ other.ignoredSourceFormat.name()
102+
+ "]"
103+
);
104+
} else {
105+
mergedFormat = this.ignoredSourceFormat;
106+
}
107+
98108
return new StoredFieldsSpec(
99109
this.requiresSource || other.requiresSource,
100110
this.requiresMetadata || other.requiresMetadata,
101111
mergedFields,
102-
ignoredFieldsSpec.merge(other.ignoredFieldsSpec),
112+
mergedFormat,
103113
mergedSourcePaths
104114
);
105115
}
106116

107117
public Set<String> requiredStoredFields() {
108-
if (ignoredFieldsSpec.noRequirements()) {
118+
if (sourcePaths.isEmpty() || ignoredSourceFormat == IgnoredSourceFormat.NO_IGNORED_SOURCE) {
109119
return requiredStoredFields;
110120
}
111121
if (requiredStoredFields.isEmpty()) {
112-
return ignoredFieldsSpec.requiredStoredFields();
122+
return Set.of(IgnoredSourceFieldMapper.NAME);
113123
}
114124
Set<String> mergedFields = new HashSet<>(requiredStoredFields);
115-
mergedFields.addAll(ignoredFieldsSpec.requiredStoredFields());
125+
mergedFields.add(IgnoredSourceFieldMapper.NAME);
116126
return mergedFields;
117127
}
118128

‎server/src/test/java/org/elasticsearch/index/fieldvisitor/IgnoredSourceFieldLoaderTests.java‎

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
import org.apache.lucene.store.Directory;
1717
import org.apache.lucene.util.BytesRef;
1818
import org.elasticsearch.common.lucene.Lucene;
19-
import org.elasticsearch.index.mapper.IgnoredFieldsSpec;
2019
import org.elasticsearch.index.mapper.IgnoredSourceFieldMapper;
2120
import org.elasticsearch.search.fetch.StoredFieldsSpec;
2221
import org.elasticsearch.test.ESTestCase;
@@ -35,37 +34,22 @@ public class IgnoredSourceFieldLoaderTests extends ESTestCase {
3534
public void testSupports() {
3635
assertTrue(
3736
IgnoredSourceFieldLoader.supports(
38-
new StoredFieldsSpec(
39-
false,
40-
false,
41-
Set.of(),
42-
new IgnoredFieldsSpec(Set.of("foo"), IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE),
43-
Set.of()
37+
StoredFieldsSpec.withSourcePaths(
38+
IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE,
39+
Set.of("foo")
4440
)
4541
)
4642
);
4743

4844
assertFalse(
4945
IgnoredSourceFieldLoader.supports(
50-
new StoredFieldsSpec(
51-
false,
52-
false,
53-
Set.of(),
54-
new IgnoredFieldsSpec(Set.of(), IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE),
55-
Set.of()
56-
)
46+
StoredFieldsSpec.withSourcePaths(IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE, Set.of())
5747
)
5848
);
5949

6050
assertFalse(
6151
IgnoredSourceFieldLoader.supports(
62-
new StoredFieldsSpec(
63-
true,
64-
false,
65-
Set.of(),
66-
new IgnoredFieldsSpec(Set.of("foo"), IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE),
67-
Set.of()
68-
)
52+
StoredFieldsSpec.withSourcePaths(IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE, Set.of("foo"))
6953
)
7054
);
7155

@@ -128,12 +112,9 @@ private void testLoader(
128112
Consumer<List<List<IgnoredSourceFieldMapper.NameValue>>> ignoredSourceTest
129113
) throws IOException {
130114
try (Directory dir = newDirectory(); IndexWriter iw = new IndexWriter(dir, newIndexWriterConfig(Lucene.STANDARD_ANALYZER))) {
131-
StoredFieldsSpec spec = new StoredFieldsSpec(
132-
false,
133-
false,
134-
Set.of(),
135-
new IgnoredFieldsSpec(fieldsToLoad, IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE),
136-
Set.of()
115+
StoredFieldsSpec spec = StoredFieldsSpec.withSourcePaths(
116+
IgnoredSourceFieldMapper.IgnoredSourceFormat.COALESCED_SINGLE_IGNORED_SOURCE,
117+
fieldsToLoad
137118
);
138119
assertTrue(IgnoredSourceFieldLoader.supports(spec));
139120
iw.addDocument(doc);

‎server/src/test/java/org/elasticsearch/index/fieldvisitor/StoredFieldLoaderTests.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
import org.apache.lucene.store.Directory;
1717
import org.apache.lucene.util.BytesRef;
1818
import org.elasticsearch.common.lucene.Lucene;
19-
import org.elasticsearch.index.mapper.IgnoredFieldsSpec;
2019
import org.elasticsearch.index.mapper.IgnoredSourceFieldMapper;
2120
import org.elasticsearch.search.fetch.StoredFieldsSpec;
2221
import org.elasticsearch.test.ESTestCase;
@@ -48,7 +47,7 @@ private StoredFieldsSpec fieldsSpec(
4847
Set<String> ignoredFields,
4948
IgnoredSourceFieldMapper.IgnoredSourceFormat format
5049
) {
51-
return new StoredFieldsSpec(false, false, storedFields, new IgnoredFieldsSpec(ignoredFields, format), Set.of());
50+
return new StoredFieldsSpec(ignoredFields.isEmpty() == false, false, storedFields, format, ignoredFields);
5251
}
5352

5453
public void testEmpty() throws IOException {

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

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
package org.elasticsearch.search.fetch;
1111

12-
import org.elasticsearch.index.mapper.IgnoredFieldsSpec;
12+
import org.elasticsearch.index.mapper.IgnoredSourceFieldMapper;
1313
import org.elasticsearch.script.Script;
1414
import org.elasticsearch.search.builder.SearchSourceBuilder;
1515
import org.elasticsearch.search.fetch.subphase.FetchSourcePhase;
@@ -90,30 +90,44 @@ public void testNoCloneOnMerge() {
9090

9191
public void testMergeSourcePaths() {
9292
StoredFieldsSpec spec = StoredFieldsSpec.NO_REQUIREMENTS;
93-
spec = spec.merge(new StoredFieldsSpec(true, false, Set.of(), IgnoredFieldsSpec.NONE, Set.of("cat")));
94-
assertThat(spec.ignoredFieldsSpec(), equalTo(IgnoredFieldsSpec.NONE));
93+
spec = spec.merge(
94+
new StoredFieldsSpec(true, false, Set.of(), IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE, Set.of("cat"))
95+
);
96+
assertThat(spec.ignoredSourceFormat(), equalTo(IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE));
9597
assertThat(spec.requiresSource(), equalTo(true));
9698
assertThat(spec.requiresMetadata(), equalTo(false));
9799
assertThat(spec.requiredStoredFields(), empty());
98100
assertThat(spec.sourcePaths(), containsInAnyOrder("cat"));
99101

100-
spec = spec.merge(new StoredFieldsSpec(true, false, Set.of(), IgnoredFieldsSpec.NONE, Set.of("dog")));
101-
assertThat(spec.ignoredFieldsSpec(), equalTo(IgnoredFieldsSpec.NONE));
102+
spec = spec.merge(
103+
new StoredFieldsSpec(true, false, Set.of(), IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE, Set.of("dog"))
104+
);
105+
assertThat(spec.ignoredSourceFormat(), equalTo(IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE));
102106
assertThat(spec.requiresSource(), equalTo(true));
103107
assertThat(spec.requiresMetadata(), equalTo(false));
104108
assertThat(spec.requiredStoredFields(), empty());
105109
assertThat(spec.sourcePaths(), containsInAnyOrder("cat", "dog"));
106110

107-
spec = spec.merge(new StoredFieldsSpec(true, false, Set.of(), IgnoredFieldsSpec.NONE, Set.of("hamster")));
108-
assertThat(spec.ignoredFieldsSpec(), equalTo(IgnoredFieldsSpec.NONE));
111+
spec = spec.merge(
112+
new StoredFieldsSpec(true, false, Set.of(), IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE, Set.of("hamster"))
113+
);
114+
assertThat(spec.ignoredSourceFormat(), equalTo(IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE));
109115
assertThat(spec.requiresSource(), equalTo(true));
110116
assertThat(spec.requiresMetadata(), equalTo(false));
111117
assertThat(spec.requiredStoredFields(), empty());
112118
assertThat(spec.sourcePaths(), containsInAnyOrder("cat", "dog", "hamster"));
113119
var pref = spec.sourcePaths();
114120

115-
spec = spec.merge(new StoredFieldsSpec(true, false, Set.of("other_field"), IgnoredFieldsSpec.NONE, Set.of()));
116-
assertThat(spec.ignoredFieldsSpec(), equalTo(IgnoredFieldsSpec.NONE));
121+
spec = spec.merge(
122+
new StoredFieldsSpec(
123+
true,
124+
false,
125+
Set.of("other_field"),
126+
IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE,
127+
Set.of()
128+
)
129+
);
130+
assertThat(spec.ignoredSourceFormat(), equalTo(IgnoredSourceFieldMapper.IgnoredSourceFormat.NO_IGNORED_SOURCE));
117131
assertThat(spec.requiresSource(), equalTo(true));
118132
assertThat(spec.requiresMetadata(), equalTo(false));
119133
assertThat(spec.requiredStoredFields(), containsInAnyOrder("other_field"));

‎test/framework/src/main/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,8 @@ protected final List<Object> blockLoaderReadValuesFromRowStrideReader(
515515
BlockLoader.RowStrideReader blockReader = loader.rowStrideReader(ctx);
516516
BlockLoader.Builder builder = loader.builder(TestBlock.factory(), ctx.reader().numDocs());
517517

518-
assert loader.rowStrideStoredFieldSpec().requiresSource() == false;
518+
assert loader.rowStrideStoredFieldSpec().requiresSource() == false
519+
|| loader.rowStrideStoredFieldSpec().sourcePaths().isEmpty() == false;
519520
BlockLoaderStoredFieldsFromLeafLoader storedFields = new BlockLoaderStoredFieldsFromLeafLoader(
520521
StoredFieldLoader.fromSpec(loader.rowStrideStoredFieldSpec()).getLoader(ctx, null),
521522
null

0 commit comments

Comments
 (0)