[Test] Use appropriate DocValueFormats in Aggregations tests #24155
Conversation
Some aggregations (like Min, Max etc) use a wrong DocValueFormat in tests (like IP or GeoHash). We should not test aggregations that expect a numeric value with a DocValueFormat like IP. Such wrong DocValueFormat can also prevent the aggregation to be rendered as ToXContent, and this will be an issue for the High Level Rest Client tests which expect to be able to parse back aggregations.
tlrx
added
:Analytics/Aggregations
| protected static DocValueFormat randomNumericDocValueFormat() { | ||
| final List<Supplier<DocValueFormat>> formats = new ArrayList<>(3); | ||
| formats.add(() -> DocValueFormat.RAW); | ||
| formats.add(() -> DocValueFormat.BOOLEAN); |
There was a problem hiding this comment.
Is this a numeric doc value format?
There was a problem hiding this comment.
I kept this one because it was used in many aggregations tests before and seems to be supported (true/false are translated to 1/0) by aggregations that work on numeric. I'm not sure it makes much sense though. Would you like to see it removed?
There was a problem hiding this comment.
hmm not sure, it does seem a bit weird to be there so I'm leaning towards "remove it". It doesn't feel like a great idea to be running numeric aggregations on a boolean field and I don't know if its something that works by design or just happens to work right now and might break in the future. /cc @jpountz who might have thoughts on that
There was a problem hiding this comment.
I think it just happens to work today and might break in the future indeed.
There was a problem hiding this comment.
In which case I would say remove the BOOLEAN format here since its not actually a numeric format
|
Thanks @colings86 |
…#24155) Some aggregations (like Min, Max etc) use a wrong DocValueFormat in tests (like IP or GeoHash). We should not test aggregations that expect a numeric value with a DocValueFormat like IP. Such wrong DocValueFormat can also prevent the aggregation to be rendered as ToXContent, and this will be an issue for the High Level Rest Client tests which expect to be able to parse back aggregations.
3 participants
Some aggregations (like
Min,Maxetc) use a wrongDocValueFormatintests (like
IPorGeoHash). We should not test aggregations that expecta numeric value with a DocValueFormat like IP. Such wrong DocValueFormat
can also prevent the aggregation to be rendered as ToXContent, and this
will be an issue for the High Level Rest Client tests which expect to be
able to parse back aggregations.