Add support for percentiles aggregation #306
Conversation
| * to be performed. | ||
| */ | ||
| final def percentilesAggregation[A: Numeric](name: String, field: Field[_, A]): PercentilesAggregation = | ||
| Percentiles(name = name, field = field.toString, Chunk.empty, missing = None) |
There was a problem hiding this comment.
All parameters should be named.
| ) extends PercentilesAggregation { self => | ||
| def missing(value: Double): PercentilesAggregation = |
There was a problem hiding this comment.
Put an empty line between.
| with WithAgg { | ||
|
|
||
| /** | ||
| * Sets the `percents` field for the [[zio.elasticsearch.aggregation.PercentilesAggregation]]. |
There was a problem hiding this comment.
| * Sets the `percents` field for the [[zio.elasticsearch.aggregation.PercentilesAggregation]]. | |
| * Sets the `percents` parameter for the [[zio.elasticsearch.aggregation.PercentilesAggregation]]. |
| * Sets the `percents` field for the [[zio.elasticsearch.aggregation.PercentilesAggregation]]. | ||
| * | ||
| * @param percents | ||
| * a array of percentiles for calculation for [[zio.elasticsearch.aggregation.PercentilesAggregation]] |
There was a problem hiding this comment.
| * a array of percentiles for calculation for [[zio.elasticsearch.aggregation.PercentilesAggregation]] | |
| * a array of percentiles to be calculated for [[zio.elasticsearch.aggregation.PercentilesAggregation]] |
*This is only suggestion. I am not sure, if meaning stays the same.
| def percents(percents: Double*): PercentilesAggregation = | ||
| self.copy(percents = Chunk.fromIterable(percents)) |
There was a problem hiding this comment.
Does it make sense to restrict percents parameter, so it must have at least one value? Something like this percents(percent: Double, percents: Double*)?
There was a problem hiding this comment.
And, maybe we could do something like this:
def percents(percent: Double, percents: Double*): PercentilesAggregation =
self.copy(percents = percents ++ (percent +: percents))
|
|
||
| private[elasticsearch] def toJson: Json = { | ||
| val percentsField = if (percents.nonEmpty) Some("percents" -> Arr(percents.map(_.toJson))) else None | ||
| val optionalFields = percentsField ++ missing.map("missing" -> _.toJson) |
There was a problem hiding this comment.
Maybe we could omit this optionalFields field, and write it directly in return?
| implicit val decoder: JsonDecoder[PercentilesAggregationResponse] = | ||
| DeriveJsonDecoder.gen[PercentilesAggregationResponse] | ||
| } | ||
|
|
There was a problem hiding this comment.
Look at lines below (140+). You must adjust that too. Please, test this aggregation as sub aggregation of some other aggregation (you can add one it-test also).
| @@ -221,6 +221,30 @@ object ElasticAggregationSpec extends ZIOSpecDefault { | |||
| ) | |||
| ) | |||
| }, | |||
| test("percentiles") { | |||
There was a problem hiding this comment.
Add aggregationTsWithAllParams test (in creating json too).
| assert(aggregation)( | ||
| equalTo(Percentiles(name = "aggregation", field = "testField", Chunk.empty, missing = None)) | ||
| ) && | ||
| assert(aggregationTs)( | ||
| equalTo(Percentiles(name = "aggregation", field = "intField", Chunk.empty, missing = None)) | ||
| ) && | ||
| assert(aggregationTsRaw)( | ||
| equalTo(Percentiles(name = "aggregation", field = "intField.raw", Chunk.empty, missing = None)) | ||
| ) && | ||
| assert(aggregationWithMissing)( | ||
| equalTo(Percentiles(name = "aggregation", field = "intField", Chunk.empty, missing = Some(20.0))) | ||
| ) && | ||
| assert(aggregationWithPercents)( | ||
| equalTo(Percentiles(name = "aggregation", field = "intField", Chunk(65, 90, 99), missing = None)) | ||
| ) |
| ``` | ||
|
|
||
| You can find more information about `Percentiles` aggregation [here](https://www.elastic.co/guide/en/elasticsearch/reference/7.17/search-aggregations-metrics-percentile-aggregation.html). | ||
|
|
| private[elasticsearch] final case class Percentiles( | ||
| name: String, | ||
| field: String, | ||
| percents: Chunk[Double], |
There was a problem hiding this comment.
Let's put missing before percents. I prefer alphabetically sorted fields and methods.
| def missing(value: Double): PercentilesAggregation = | ||
| self.copy(missing = Some(value)) | ||
|
|
||
| def withAgg(agg: SingleElasticAggregation): MultipleAggregations = |
There was a problem hiding this comment.
Please put withAgg below percents.
| def withAgg(agg: SingleElasticAggregation): MultipleAggregations = | ||
| multipleAggregations.aggregations(self, agg) | ||
|
|
||
| def percents(percent: Double, percents: Double*): PercentilesAggregation = |
There was a problem hiding this comment.
Do we have any specific reason why we named this percents if the aggregation is percentiles?
There was a problem hiding this comment.
In elastic api the parameter is named percents. But i can change the name to percentiles if it seems more intuitive.
There was a problem hiding this comment.
Agreed to leave this named percents
Part of #118