Test PerFieldMapperCodec with and without synthetic ID randomly#143027
Conversation
- Add syntheticId to 5-arg createFormatSupplier chain for time series mode - Use syntheticId(timeSeries) in tests that use the 4-arg supplier - Assert TSDBSyntheticIdPostingsFormat or ES87/ES812 where applicable - Run spotlessApply for formatting Co-authored-by: Cursor <cursoragent@cursor.com>
burqen
added
>test
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| @@ -165,17 +183,21 @@ public void testUseEs812PostingsFormatForIdField() throws IOException { | |||
| Function<String, PostingsFormat> postingsFormats = es87BloomFilterPostingsFormat.getPostingsFormats(); | |||
| result = postingsFormats.apply("_id"); | |||
| } | |||
| assertThat(result, instanceOf(ES812PostingsFormat.class)); | |||
| assertThat(result, anyOf(instanceOf(ES812PostingsFormat.class), instanceOf(TSDBSyntheticIdPostingsFormat.class))); | |||
There was a problem hiding this comment.
can't we use useSyntheticId here to make a stronger assertion?
tlrx
left a comment
tlrx
left a comment
There was a problem hiding this comment.
LGTM but I left some minor comments I'd like to see addressed
| assertThat(perFieldMapperCodec.useBloomFilter("another_field"), is(false)); | ||
|
|
||
| Class<? extends PostingsFormat> expectedPostingsFormat = timeSeries ? ES812PostingsFormat.class : Lucene103PostingsFormat.class; | ||
| assertThat(perFieldMapperCodec.getPostingsFormatForField("another_field"), instanceOf(expectedPostingsFormat)); | ||
| } | ||
|
|
||
| public void testUseBloomFilterWithTimestampFieldEnabled() throws IOException { | ||
| PerFieldFormatSupplier perFieldMapperCodec = createFormatSupplier(true, true, false); | ||
| boolean timeSeries = true; |
There was a problem hiding this comment.
I wonder why timeSeries is stored in a local variable and not passed directly?
There was a problem hiding this comment.
I wanted to make the dependency between timeSeries and syntheticId clear. That can be quite difficult to decipher without knowledge of the code otherwise.
There was a problem hiding this comment.
Maybe a comment could have helped?
There was a problem hiding this comment.
I inlined the timeSeries instead as it cause more confusion than clarity :)
| @@ -94,33 +96,49 @@ public class PerFieldMapperCodecTests extends ESTestCase { | |||
|
|
|||
| public void testUseBloomFilter() throws IOException { | |||
| boolean timeSeries = randomBoolean(); | |||
| PerFieldFormatSupplier perFieldMapperCodec = createFormatSupplier(false, timeSeries, false); | |||
| boolean syntheticId = syntheticId(timeSeries); | |||
There was a problem hiding this comment.
nit: could we make this final and rename syntheticId to randomSyntheticId or something like this?
| } | ||
| } | ||
|
|
||
| public void testUseES87TSDBEncodingForTimestampField() throws IOException { | ||
| PerFieldFormatSupplier perFieldMapperCodec = createFormatSupplier(true, true, true); | ||
| boolean timeSeries = true; |
There was a problem hiding this comment.
yeah, I don't get those local variables :)
server/src/test/java/org/elasticsearch/index/codec/PerFieldMapperCodecTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/codec/PerFieldMapperCodecTests.java
Outdated
Show resolved
Hide resolved
4 participants
Using synthetic_id based on feature_flag, index mode (time_series) and random boolean.
Update assertions of expected format instances to fit.