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

Added support for sorting buckets based on sub aggregation down the current hierarchy #5340

Merged
merged 1 commit into from Mar 5, 2014
Merged

Added support for sorting buckets based on sub aggregation down the current hierarchy #5340

merged 1 commit into from Mar 5, 2014

Conversation

uboness
Copy link
Contributor

@uboness uboness commented Mar 5, 2014

This is supported as long as the aggregation in the specified order path are of a single-bucket type, where the last aggregation in the path points to either a single-bucket aggregation or a metrics one. If it's a single-bucket aggregation, the sort will be applied on the document count in the bucket (i.e. doc_count), and if it is a metrics type, the sort will be applied on the pointed out metric (in case of a single-metric aggregations, such as avg, the sort will be applied on the single metric value)

Closes #5253

}
}
}
--------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this example actually make sense? Since stats are computed on the same field that is used for the histogram, this order is equivalent to _key: desc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed :D... will change it to popularity?

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

@jpountz
Copy link
Contributor

jpountz commented Mar 5, 2014

This change looks good, I particularly appreciate that the aggregator that is used for comparisons is computed up-front.
Before getting this in, I think we should add validation of aggregation names so that they don't collide with special characters used in sort expressions (either as part of this PR or in a different one)?

@uboness
Copy link
Contributor Author

uboness commented Mar 5, 2014

Before getting this in, I think we should add validation of aggregation names so that they don't collide with special characters used in sort expressions (either as part of this PR or in a different one)?

yeah, agreed... in particular, verify that aggregation names don't include .,[,]. The only thing that we should be aware of is that putting this constraint does break bwc in a way (as we never had these constraints in 1.0)

 Supports sorting on sub-aggs down the current hierarchy. This is supported as long as the aggregation in the specified order path are of a single-bucket type, where the last aggregation in the path points to either a single-bucket aggregation or a metrics one. If it's a single-bucket aggregation, the sort will be applied on the document count in the bucket (i.e. doc_count), and if it is a metrics type, the sort will be applied on the pointed out metric (in case of a single-metric aggregations, such as avg, the sort will be applied on the single metric value)

 NOTE: this commit adds a constraint on what should be considered a valid aggregation name. Aggregations names must be alpha-numeric and may contain '-' and '_'.

 Closes #5253
@jpountz
Copy link
Contributor

jpountz commented Mar 5, 2014

Looks good to me!

@uboness uboness merged commit 9d0fc76 into elastic:master Mar 5, 2014
@uboness uboness deleted the feature/sort_by_sub_aggs branch March 5, 2014 23:31
@clintongormley clintongormley changed the title Added support for sorting buckets based on sub aggregation down the curr... Added support for sorting buckets based on sub aggregation down the current hierarchy Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sorting on SingleBucketAggregation's should be possible
3 participants