Skip to content

Commit 3d75e27

Browse files
martijnvgmartijnvg
committed
ensure doc values enabled and added more tests
1 parent acf0aed commit 3d75e27

File tree

5 files changed

+144
-34
lines changed

5 files changed

+144
-34
lines changed

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -613,8 +613,12 @@ private static void throwOnCreateDynamicNestedViaCopyTo(Mapper dynamicObjectMapp
613613
}
614614

615615
private static void parseArray(DocumentParserContext context, String lastFieldName) throws IOException {
616+
// Record previous immediate parent, so that it can be reset after array has been parsed:
617+
// This is for recording array offset with synthetic source. Only if the immediate parent is an array,
618+
// then we offsets can be accounted accurately.
616619
var prev = context.getImmediateXContentParent();
617620
context.setImmediateXContentParent(context.parser().currentToken());
621+
618622
Mapper mapper = getLeafMapper(context, lastFieldName);
619623
if (mapper != null) {
620624
// There is a concrete mapper for this field already. Need to check if the mapper
@@ -628,6 +632,7 @@ private static void parseArray(DocumentParserContext context, String lastFieldNa
628632
} else {
629633
parseArrayDynamic(context, lastFieldName);
630634
}
635+
// Reset previous immediate parent
631636
context.setImmediateXContentParent(prev);
632637
}
633638

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -500,18 +500,18 @@ public FieldArrayContext getOffSetContext() {
500500
return fieldArrayContext;
501501
}
502502

503-
private XContentParser.Token token;
503+
private XContentParser.Token lastSetToken;
504504

505505
public void setImmediateXContentParent(XContentParser.Token token) {
506-
this.token = token;
506+
this.lastSetToken = token;
507507
}
508508

509509
public XContentParser.Token getImmediateXContentParent() {
510-
return token;
510+
return lastSetToken;
511511
}
512512

513513
public boolean isImmediateParentAnArray() {
514-
return token == XContentParser.Token.START_ARRAY;
514+
return lastSetToken == XContentParser.Token.START_ARRAY;
515515
}
516516

517517
/**

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ private static class Offsets {
7474

7575
int currentOffset;
7676
// Need to use TreeMap here, so that we maintain the order in which each value (with offset) gets inserted,
77-
// (which is in the same order was document gets parsed) so we store offsets in right order.
77+
// (which is in the same order the document gets parsed) so we store offsets in right order.
7878
final Map<String, List<Integer>> valueToOffsets = new TreeMap<>();
7979
final List<Integer> nullValueOffsets = new ArrayList<>(2);
8080

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,7 @@ public KeywordFieldMapper build(MapperBuilderContext context) {
443443
String offsetsFieldName;
444444
if (context.isSourceSynthetic()
445445
&& sourceKeepMode == SourceKeepMode.ARRAYS
446+
&& hasDocValues()
446447
&& fieldtype.stored() == false
447448
&& copyTo.copyToFields().isEmpty()
448449
&& multiFieldsBuilder.hasMultiFields() == false) {
@@ -1106,11 +1107,7 @@ public KeywordFieldType fieldType() {
11061107
}
11071108

11081109
public boolean supportStoringArrayOffsets(DocumentParserContext context) {
1109-
if (offsetsFieldName != null) {
1110-
return true;
1111-
} else {
1112-
return false;
1113-
}
1110+
return offsetsFieldName != null;
11141111
}
11151112

11161113
@Override

‎server/src/test/java/org/elasticsearch/index/mapper/OffsetDocValuesLoaderTests.java‎

Lines changed: 132 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.io.IOException;
2121

2222
import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
23+
import static org.hamcrest.Matchers.nullValue;
2324

2425
public class OffsetDocValuesLoaderTests extends MapperServiceTestCase {
2526

@@ -31,6 +32,107 @@ protected Settings getIndexSettings() {
3132
.build();
3233
}
3334

35+
public void testOffsetArrayNoDocValues() throws Exception {
36+
String mapping = """
37+
{
38+
"_doc": {
39+
"properties": {
40+
"field": {
41+
"type": "keyword",
42+
"doc_values": false
43+
}
44+
}
45+
}
46+
}
47+
""";
48+
try (var mapperService = createMapperService(mapping)) {
49+
var fieldMapper = mapperService.mappingLookup().getMapper("field");
50+
assertThat(fieldMapper.getOffsetFieldName(), nullValue());
51+
}
52+
}
53+
54+
public void testOffsetArrayStored() throws Exception {
55+
String mapping = """
56+
{
57+
"_doc": {
58+
"properties": {
59+
"field": {
60+
"type": "keyword",
61+
"store": true
62+
}
63+
}
64+
}
65+
}
66+
""";
67+
try (var mapperService = createMapperService(mapping)) {
68+
var fieldMapper = mapperService.mappingLookup().getMapper("field");
69+
assertThat(fieldMapper.getOffsetFieldName(), nullValue());
70+
}
71+
}
72+
73+
public void testOffsetMultiFields() throws Exception {
74+
String mapping = """
75+
{
76+
"_doc": {
77+
"properties": {
78+
"field": {
79+
"type": "keyword",
80+
"fields": {
81+
"sub": {
82+
"type": "text"
83+
}
84+
}
85+
}
86+
}
87+
}
88+
}
89+
""";
90+
try (var mapperService = createMapperService(mapping)) {
91+
var fieldMapper = mapperService.mappingLookup().getMapper("field");
92+
assertThat(fieldMapper.getOffsetFieldName(), nullValue());
93+
}
94+
}
95+
96+
public void testOffsetArrayNoSyntheticSource() throws Exception {
97+
String mapping = """
98+
{
99+
"_doc": {
100+
"properties": {
101+
"field": {
102+
"type": "keyword"
103+
}
104+
}
105+
}
106+
}
107+
""";
108+
try (var mapperService = createMapperService(Settings.EMPTY, mapping)) {
109+
var fieldMapper = mapperService.mappingLookup().getMapper("field");
110+
assertThat(fieldMapper.getOffsetFieldName(), nullValue());
111+
}
112+
}
113+
114+
public void testOffsetArrayNoSourceArrayKeep() throws Exception {
115+
String mapping = """
116+
{
117+
"_doc": {
118+
"properties": {
119+
"field": {
120+
"type": "keyword"
121+
}
122+
}
123+
}
124+
}
125+
""";
126+
var settingsBuilder = Settings.builder().put("index.mapping.source.mode", "synthetic");
127+
if (randomBoolean()) {
128+
settingsBuilder.put("index.mapping.synthetic_source_keep", randomBoolean() ? "none" : "all");
129+
}
130+
try (var mapperService = createMapperService(settingsBuilder.build(), mapping)) {
131+
var fieldMapper = mapperService.mappingLookup().getMapper("field");
132+
assertThat(fieldMapper.getOffsetFieldName(), nullValue());
133+
}
134+
}
135+
34136
public void testOffsetArray() throws Exception {
35137
verifyOffsets("{\"field\":[\"z\",\"x\",\"y\",\"c\",\"b\",\"a\"]}");
36138
verifyOffsets("{\"field\":[\"z\",null,\"y\",\"c\",null,\"a\"]}");
@@ -73,7 +175,7 @@ private void verifyOffsets(String source) throws IOException {
73175
}
74176

75177
private void verifyOffsets(String source, String expectedSource) throws IOException {
76-
var mapperService = createMapperService("""
178+
String mapping = """
77179
{
78180
"_doc": {
79181
"properties": {
@@ -83,29 +185,35 @@ private void verifyOffsets(String source, String expectedSource) throws IOExcept
83185
}
84186
}
85187
}
86-
""");
87-
var mapper = mapperService.documentMapper();
88-
89-
try (var directory = newDirectory()) {
90-
var iw = indexWriterForSyntheticSource(directory);
91-
var doc = mapper.parse(new SourceToParse("_id", new BytesArray(source), XContentType.JSON));
92-
doc.updateSeqID(0, 0);
93-
doc.version().setLongValue(0);
94-
iw.addDocuments(doc.docs());
95-
iw.close();
96-
try (var indexReader = wrapInMockESDirectoryReader(DirectoryReader.open(directory))) {
97-
var layer = new SortedSetWithOffsetsDocValuesSyntheticFieldLoaderLayer("field", "field.offsets");
98-
var leafReader = indexReader.leaves().get(0).reader();
99-
var loader = (ImmediateDocValuesLoader) layer.docValuesLoader(leafReader, new int[] { 0 });
100-
assertTrue(loader.advanceToDoc(0));
101-
assertTrue(loader.count() > 0);
102-
XContentBuilder builder = jsonBuilder().startObject();
103-
builder.startArray("field");
104-
loader.write(builder);
105-
builder.endArray().endObject();
106-
107-
var actual = Strings.toString(builder);
108-
assertEquals(expectedSource, actual);
188+
""";
189+
verifyOffsets(mapping, source, expectedSource);
190+
}
191+
192+
private void verifyOffsets(String mapping, String source, String expectedSource) throws IOException {
193+
try (var mapperService = createMapperService(mapping)) {
194+
var mapper = mapperService.documentMapper();
195+
196+
try (var directory = newDirectory()) {
197+
var iw = indexWriterForSyntheticSource(directory);
198+
var doc = mapper.parse(new SourceToParse("_id", new BytesArray(source), XContentType.JSON));
199+
doc.updateSeqID(0, 0);
200+
doc.version().setLongValue(0);
201+
iw.addDocuments(doc.docs());
202+
iw.close();
203+
try (var indexReader = wrapInMockESDirectoryReader(DirectoryReader.open(directory))) {
204+
var layer = new SortedSetWithOffsetsDocValuesSyntheticFieldLoaderLayer("field", "field.offsets");
205+
var leafReader = indexReader.leaves().getFirst().reader();
206+
var loader = (ImmediateDocValuesLoader) layer.docValuesLoader(leafReader, new int[] { 0 });
207+
assertTrue(loader.advanceToDoc(0));
208+
assertTrue(loader.count() > 0);
209+
XContentBuilder builder = jsonBuilder().startObject();
210+
builder.startArray("field");
211+
loader.write(builder);
212+
builder.endArray().endObject();
213+
214+
var actual = Strings.toString(builder);
215+
assertEquals(expectedSource, actual);
216+
}
109217
}
110218
}
111219
}

0 commit comments

Comments
 (0)