Skip to content

Commit 5e7159e

Browse files
authored
Fix PerFieldFormatSupplier mistake (#134008)
Correctly use seqno_field_use_tsdb_doc_values_format feature flag to ensure _seq_no doesn't get excluded. Closes #133875 Closes #133993
1 parent ae7bfd6 commit 5e7159e

File tree

3 files changed

+55
-14
lines changed

3 files changed

+55
-14
lines changed

muted-tests.yml

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -513,12 +513,6 @@ tests:
513513
- class: org.elasticsearch.xpack.ml.integration.InferenceIT
514514
method: testInferClassificationModel
515515
issue: https://github.com/elastic/elasticsearch/issues/133448
516-
- class: org.elasticsearch.xpack.logsdb.LogsIndexingIT
517-
method: testRouteOnSortFields
518-
issue: https://github.com/elastic/elasticsearch/issues/133993
519-
- class: org.elasticsearch.xpack.logsdb.LogsIndexingIT
520-
method: testShrink
521-
issue: https://github.com/elastic/elasticsearch/issues/133875
522516
- class: org.elasticsearch.xpack.esql.action.CrossClusterQueryWithPartialResultsIT
523517
method: testPartialResults
524518
issue: https://github.com/elastic/elasticsearch/issues/131481

server/src/main/java/org/elasticsearch/index/codec/PerFieldFormatSupplier.java

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,37 @@
2727
import org.elasticsearch.index.mapper.Mapper;
2828
import org.elasticsearch.index.mapper.MapperService;
2929
import org.elasticsearch.index.mapper.SeqNoFieldMapper;
30+
import org.elasticsearch.index.mapper.TimeSeriesIdFieldMapper;
31+
import org.elasticsearch.index.mapper.TimeSeriesRoutingHashFieldMapper;
3032
import org.elasticsearch.index.mapper.vectors.DenseVectorFieldMapper;
3133

34+
import java.util.Collections;
35+
import java.util.HashSet;
36+
import java.util.Set;
37+
3238
/**
3339
* Class that encapsulates the logic of figuring out the most appropriate file format for a given field, across postings, doc values and
3440
* vectors.
3541
*/
3642
public class PerFieldFormatSupplier {
3743

38-
private static final FeatureFlag SEQNO_FIELD_USE_TSDB_DOC_VALUES_FORMAT = new FeatureFlag("seqno_field_use_tsdb_doc_values_format");
44+
static final FeatureFlag SEQNO_FIELD_USE_TSDB_DOC_VALUES_FORMAT = new FeatureFlag("seqno_field_use_tsdb_doc_values_format");
45+
private static final Set<String> INCLUDE_META_FIELDS;
46+
47+
static {
48+
// TODO: should we just allow all fields to use tsdb doc values codec?
49+
// Avoid using tsdb codec for fields like _seq_no, _primary_term.
50+
// But _tsid and _ts_routing_hash should always use the tsdb codec.
51+
Set<String> includeMetaField = new HashSet<>(3);
52+
includeMetaField.add(TimeSeriesIdFieldMapper.NAME);
53+
includeMetaField.add(TimeSeriesRoutingHashFieldMapper.NAME);
54+
if (SEQNO_FIELD_USE_TSDB_DOC_VALUES_FORMAT.isEnabled()) {
55+
includeMetaField.add(SeqNoFieldMapper.NAME);
56+
}
57+
// Don't the include _recovery_source_size and _recovery_source fields, since their values can be trimmed away in
58+
// RecoverySourcePruneMergePolicy, which leads to inconsistencies between merge stats and actual values.
59+
INCLUDE_META_FIELDS = Collections.unmodifiableSet(includeMetaField);
60+
}
3961

4062
private static final DocValuesFormat docValuesFormat = new Lucene90DocValuesFormat();
4163
private static final KnnVectorsFormat knnVectorsFormat = new Lucene99HnswVectorsFormat();
@@ -126,13 +148,7 @@ boolean useTSDBDocValuesFormat(final String field) {
126148
}
127149

128150
private boolean excludeFields(String fieldName) {
129-
// TODO: should we just allow all fields to use tsdb doc values codec?
130-
// Avoid using tsdb codec for fields like _seq_no, _primary_term.
131-
// But _tsid and _ts_routing_hash should always use the tsdb codec.
132-
return fieldName.startsWith("_")
133-
&& fieldName.equals("_tsid") == false
134-
&& fieldName.equals("_ts_routing_hash") == false
135-
&& (SEQNO_FIELD_USE_TSDB_DOC_VALUES_FORMAT.isEnabled() && fieldName.equals(SeqNoFieldMapper.NAME) == false);
151+
return fieldName.startsWith("_") && INCLUDE_META_FIELDS.contains(fieldName) == false;
136152
}
137153

138154
private boolean isTimeSeriesModeIndex() {

server/src/test/java/org/elasticsearch/index/codec/PerFieldMapperCodecTests.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@
2121
import org.elasticsearch.index.codec.bloomfilter.ES87BloomFilterPostingsFormat;
2222
import org.elasticsearch.index.codec.postings.ES812PostingsFormat;
2323
import org.elasticsearch.index.mapper.MapperService;
24+
import org.elasticsearch.index.mapper.SeqNoFieldMapper;
25+
import org.elasticsearch.index.mapper.SourceFieldMapper;
26+
import org.elasticsearch.index.mapper.TimeSeriesIdFieldMapper;
27+
import org.elasticsearch.index.mapper.TimeSeriesRoutingHashFieldMapper;
2428
import org.elasticsearch.test.ESTestCase;
2529

2630
import java.io.IOException;
@@ -201,6 +205,33 @@ public void testLogsIndexMode() throws IOException {
201205
assertThat((perFieldMapperCodec.useTSDBDocValuesFormat("response_size")), is(true));
202206
}
203207

208+
public void testMetaFields() throws IOException {
209+
PerFieldFormatSupplier perFieldMapperCodec = createFormatSupplier(true, IndexMode.LOGSDB, MAPPING_3);
210+
assertThat((perFieldMapperCodec.useTSDBDocValuesFormat(TimeSeriesIdFieldMapper.NAME)), is(true));
211+
assertThat((perFieldMapperCodec.useTSDBDocValuesFormat(TimeSeriesRoutingHashFieldMapper.NAME)), is(true));
212+
// See: PerFieldFormatSupplier why these fields shouldn't use tsdb codec
213+
assertThat((perFieldMapperCodec.useTSDBDocValuesFormat(SourceFieldMapper.RECOVERY_SOURCE_NAME)), is(false));
214+
assertThat((perFieldMapperCodec.useTSDBDocValuesFormat(SourceFieldMapper.RECOVERY_SOURCE_SIZE_NAME)), is(false));
215+
}
216+
217+
public void testSeqnoField() throws IOException {
218+
assumeTrue(
219+
"seqno_field_use_tsdb_doc_values_format should be enabled",
220+
PerFieldFormatSupplier.SEQNO_FIELD_USE_TSDB_DOC_VALUES_FORMAT.isEnabled()
221+
);
222+
PerFieldFormatSupplier perFieldMapperCodec = createFormatSupplier(true, IndexMode.LOGSDB, MAPPING_3);
223+
assertThat((perFieldMapperCodec.useTSDBDocValuesFormat(SeqNoFieldMapper.NAME)), is(true));
224+
}
225+
226+
public void testSeqnoFieldFeatureFlagDisabled() throws IOException {
227+
assumeTrue(
228+
"seqno_field_use_tsdb_doc_values_format should be disabled",
229+
PerFieldFormatSupplier.SEQNO_FIELD_USE_TSDB_DOC_VALUES_FORMAT.isEnabled() == false
230+
);
231+
PerFieldFormatSupplier perFieldMapperCodec = createFormatSupplier(true, IndexMode.LOGSDB, MAPPING_3);
232+
assertThat((perFieldMapperCodec.useTSDBDocValuesFormat(SeqNoFieldMapper.NAME)), is(false));
233+
}
234+
204235
private PerFieldFormatSupplier createFormatSupplier(boolean enableES87TSDBCodec, IndexMode mode, String mapping) throws IOException {
205236
Settings.Builder settings = Settings.builder();
206237
settings.put(IndexSettings.MODE.getKey(), mode);

0 commit comments

Comments
 (0)