-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Move auto date histogram to aggregations module #90746
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
Changes from 5 commits
df77c3c
624c6a6
91415ab
34213e5
819dd7e
10e5ffa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,19 +6,24 @@ | |
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.search.aggregations.pipeline; | ||
| package org.elasticsearch.aggregations.pipeline; | ||
|
|
||
| import org.elasticsearch.action.index.IndexRequestBuilder; | ||
| import org.elasticsearch.action.search.SearchResponse; | ||
| import org.elasticsearch.aggregations.AggregationsPlugin; | ||
| import org.elasticsearch.common.collect.EvictingQueue; | ||
| import org.elasticsearch.common.util.Maps; | ||
| import org.elasticsearch.plugins.Plugin; | ||
| import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; | ||
| import org.elasticsearch.search.aggregations.bucket.histogram.Histogram.Bucket; | ||
| import org.elasticsearch.search.aggregations.pipeline.BucketHelpers; | ||
| import org.elasticsearch.search.aggregations.pipeline.SimpleValue; | ||
| import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; | ||
| import org.elasticsearch.test.ESIntegTestCase; | ||
| import org.hamcrest.Matchers; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
@@ -66,6 +71,12 @@ public String toString() { | |
| } | ||
| } | ||
|
|
||
| // TODO: maybe add base class that overwrites nodePlugins(...) for all tests that will be added to this module. | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add base test classes in a followup change, so that we will not have to add |
||
| @Override | ||
| protected Collection<Class<? extends Plugin>> nodePlugins() { | ||
| return List.of(AggregationsPlugin.class); | ||
| } | ||
|
|
||
| private ValuesSourceAggregationBuilder<? extends ValuesSourceAggregationBuilder<?>> randomMetric(String name, String field) { | ||
| int rand = randomIntBetween(0, 3); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.aggregations.bucket; | ||
| package org.elasticsearch.aggregations.bucket.adjacency; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At one point we were actually trying to reduce the number of packages. I'm not 100% sure why - they don't make a big difference to me either way. I guess I'd prefer they all be in a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is what I prefer too, but then I realise it can become a bit messy too. When we move aggregations like Note that some packages already share multiple aggregators. Like the histogram or terms package.
That is true, but the way I remember this was, avoiding packages to have with just a small number of classes. For example a package with 3 classes or less, is something we should avoid. Most of these aggregator packages have more classes. Enough imo that having its own package is okay.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. Let's keep going then. |
||
|
|
||
| import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
This java integration test is also using:
PipelineAggregationHelperTests. So I already moved this integration test to the aggregations module, for the same reason why the other pipeline unit tests were moved.