Skip to content
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

[data.search.aggs]: Expression functions for bucket agg types #64772

Merged
merged 21 commits into from
May 5, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Apr 29, 2020

Closes #63762

Summary

In this PR new expression functions were created for the following bucket agg types:

  • date_histogram
  • histogram
  • `range
  • date_range
  • ip_range
  • filter
  • filters
  • significant_terms
  • geo_hash
  • geo_tile

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@alexwizp alexwizp added Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) release_note:skip Skip the PR/issue when compiling release notes labels Apr 29, 2020
@alexwizp alexwizp marked this pull request as ready for review April 29, 2020 16:04
@alexwizp alexwizp requested a review from a team as a code owner April 29, 2020 16:04
@alexwizp alexwizp changed the title [data.search.aggs]: Expression functions for bucket agg types - ranges agg types + significant terms [Step 1][data.search.aggs]: Expression functions for bucket agg types Apr 30, 2020
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great -- thanks for powering through all of these! I have a few comments, mostly just nits around the help text, and a couple of questions.

The only broader thing I'm noticing is that, looking at agg_type.ts, it seems that customLabel param is also magically appended to every agg type (much like json), unless it is explicitly false.

I think this means that we will need to add a customLabel arg to every expression function as well 🙁

I'm wondering if it makes sense to use a base interface for each of the params to extend from, which can contain some of these global things, e.g.

interface AggParamsFilter extends BaseAggParams {}

But I'm also fine with duplicating them in each interface like we've done for json, it's just a matter of remembering them.

WDYT?

src/plugins/data/public/search/aggs/buckets/date_range.ts Outdated Show resolved Hide resolved
src/plugins/data/public/search/aggs/buckets/filter_fn.ts Outdated Show resolved Hide resolved
src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts Outdated Show resolved Hide resolved
src/plugins/data/public/search/aggs/buckets/geo_tile_fn.ts Outdated Show resolved Hide resolved
src/plugins/data/public/search/aggs/buckets/ip_range.ts Outdated Show resolved Hide resolved
src/plugins/data/public/search/aggs/buckets/ip_range_fn.ts Outdated Show resolved Hide resolved
alexwizp and others added 4 commits May 1, 2020 11:17
# Conflicts:
#	docs/development/plugins/data/public/kibana-plugin-plugins-data-public.daterangekey.from.md
#	docs/development/plugins/data/public/kibana-plugin-plugins-data-public.daterangekey.md
#	docs/development/plugins/data/public/kibana-plugin-plugins-data-public.daterangekey.to.md
#	docs/development/plugins/data/public/kibana-plugin-plugins-data-public.iprangekey.md
#	src/plugins/data/public/public.api.md
#	src/plugins/data/public/search/aggs/types.ts
@alexwizp alexwizp changed the title [Step 1][data.search.aggs]: Expression functions for bucket agg types [data.search.aggs]: Expression functions for bucket agg types May 4, 2020
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall revisions LGTM, just one more note

src/plugins/data/public/search/aggs/types.ts Show resolved Hide resolved
@alexwizp alexwizp requested a review from lukeelmers May 4, 2020 15:58
Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM! One small nit on customLabels in filters, otherwise no remaining concerns on my end.

Comment on lines +51 to +56
export interface AggParamsFilters extends BaseAggParams {
filters?: Array<{
input: Query;
label: string;
}>;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this interface accurate, don't we need to override the customLabels from BaseAggParams here, since AggParamsFilters explicitly sets it to false and therefore it isn't a valid param at all?

@alexwizp
Copy link
Contributor Author

alexwizp commented May 5, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alexwizp alexwizp merged commit 6349575 into elastic:master May 5, 2020
alexwizp added a commit to alexwizp/kibana that referenced this pull request May 5, 2020
…c#64772)

* [data.search.aggs]: Expression functions for bucket agg types - ranges agg types + significant terms

* new portion of changes

* add geo_tile_fn

* add geo_hash_fn

* Update src/plugins/data/public/search/aggs/buckets/filter_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_tile_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/ip_range_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* create BaseAggParams

* add filters_fn

* add histogram / date_histogram expression functions

* cleanup

* terms - order should be optional

* add custom label params

Co-authored-by: Luke Elmers <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
alexwizp added a commit that referenced this pull request May 5, 2020
#65227)

* [data.search.aggs]: Expression functions for bucket agg types - ranges agg types + significant terms

* new portion of changes

* add geo_tile_fn

* add geo_hash_fn

* Update src/plugins/data/public/search/aggs/buckets/filter_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_tile_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/ip_range_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* Update src/plugins/data/public/search/aggs/buckets/geo_hash_fn.ts

Co-authored-by: Luke Elmers <[email protected]>

* create BaseAggParams

* add filters_fn

* add histogram / date_histogram expression functions

* cleanup

* terms - order should be optional

* add custom label params

Co-authored-by: Luke Elmers <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Luke Elmers <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data.search.aggs]: Expression functions for bucket agg types
5 participants