Introduce BlockLoaderTestCase #119415
Introduce BlockLoaderTestCase #119415
Conversation
lkts
added
>test
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
Team:Analytics
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| return new DataSourceResponse.DynamicMappingGenerator(isObject -> false); | ||
| } | ||
| })) | ||
| .build(); |
There was a problem hiding this comment.
I wonder how much it's worth changing the interface of this one so it feels less "logsdb" and more "everything". OTOH, that's a thing you do when you are further along then this. So keep this for now.
There was a problem hiding this comment.
Is there something specific that caught your eye? Reading this requires some knowledge of data generation indeed but i don't really see it screaming "logsdb" (except the namespace which i will change).
There was a problem hiding this comment.
DataSourceHandlers isn't really a vocabulary word I know. But it's fine. DynamicMappingGenerator sounds quite sensible.
| } | ||
|
|
||
| return list; | ||
| } |
There was a problem hiding this comment.
This'll almost certainly float to the superclass when you make more of these.
| var mapping = mappingGenerator.generate(template); | ||
| var mappingXContent = XContentBuilder.builder(XContentType.JSON.xContent()).map(mapping.raw()); | ||
|
|
||
| var syntheticSource = randomBoolean(); |
There was a problem hiding this comment.
Seeing this makes me wonder how much we should parameterize vs randomize the mappings. It's more work to make a parameterized array, but these feel like something a couple of nested for loops could spit out the valid cases and we could see them. It there aren't thousands of them we could run them on every execution.
No need to change anything in this PR. I'd be fine flipping later if you think it's a good idea. Or never. It's probably fine.
There was a problem hiding this comment.
Yes, this is a good question. My statement here is definitely more "i need mappings and values and there is already this thing that knows how to do that" than "randomize is a way to go". I'll keep it but i can definitely see some evolution.
|
|
||
| var fieldValue = generator.generateValue(); | ||
|
|
||
| Object blockLoaderResult = setupAndInvokeBlockLoader(mapperService, fieldValue); |
There was a problem hiding this comment.
Can we randomly inject extra fields and/or documents to mimic the issue in #117792, but that's for follow-ups.
There was a problem hiding this comment.
We could but i wonder if we should cover this in more integration level tests.
This PR proposes extracting block loader tests into a separate hierarchy and breaking the connection between synthetic source tests and block loader tests. See #115257 for motivation.
This approach leverages existing data generation infrastructure used for LogsDB testing. As a result there is no need to implement generation of mapping/values again and block loader tests have to check all possible mapping parameter combinations (provided they are supported in data generation).
KeywordFieldBlockLoaderTestsis implemented as an example.Next steps:
org.elasticsearch.logsdb.datagenerationtoorg.elasticsearch.datagenerationsince it is not specific to LogsDB now.KeywordFieldMapperTests.ignore_aboveforkeywordand we'll needignore_malformedfairly soon.