-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add supported type tests to min aggregation #54021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
polyfractal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
andyb-elastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Not sure if we want to do this or not, but in files like this one where there are some existing tests for unsupported types, I've been removing them, e.g.
Lines 281 to 295 in 39785eb
| public void testUnsupportedType() { | |
| MinAggregationBuilder aggregationBuilder = new MinAggregationBuilder("min").field("not_a_number"); | |
| MappedFieldType fieldType = new KeywordFieldMapper.KeywordFieldType(); | |
| fieldType.setName("not_a_number"); | |
| fieldType.setHasDocValues(true); | |
| IllegalArgumentException e = expectThrows(IllegalArgumentException.class, | |
| () -> testCase(aggregationBuilder, new MatchAllDocsQuery(), iw -> { | |
| iw.addDocument(singleton(new SortedSetDocValuesField("string", new BytesRef("foo")))); | |
| }, (Consumer<InternalMin>) min -> { | |
| fail("Should have thrown exception"); | |
| }, fieldType)); | |
| assertEquals(e.getMessage(), "Expected numeric type on field [not_a_number], but got [keyword]"); | |
| } |
It is covered by new supported-type tests.
PR that adds tests for supported ValuesSourceTypes for the
MinAggregator