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

(dsl): Support Percentile ranks aggregation #366

Merged

Conversation

vanjaftn
Copy link
Contributor

@vanjaftn vanjaftn commented Nov 15, 2023

Part of #118

@vanjaftn vanjaftn changed the title (dsl): Support Percentile ranks aggregation (dsl): Support Percentile ranks aggregation Nov 15, 2023
Copy link
Collaborator

@dbulaja98 dbulaja98 left a comment

Choose a reason for hiding this comment

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

Nice job!
Please rebase this PR, and put this aggregation into the Filter aggregation too.

```

You can find more information about `Percentile ranks` aggregation [here](https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-metrics-percentile-rank-aggregation.html).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there one or two empty spaces at the end? If two, omit one.

* parameters.
*
* @param field
* the type-safe field for which stats aggregation will be executed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* the type-safe field for which stats aggregation will be executed
* the type-safe field for which percentile ranks aggregation will be executed

final def percentileRanksAggregation[A: Numeric](
field: Field[_, A],
name: String,
values: Int*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can there be zero values?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, you are missing values in scaladoc.

* parameters.
*
* @param field
* the field for which stats aggregation will be executed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* the field for which stats aggregation will be executed
* the field for which percentile ranks aggregation will be executed

* an instance of [[zio.elasticsearch.aggregation.PercentileRanksAggregation]] that represents percentile ranks
* aggregation to be performed.
*/
final def percentileRanksAggregation(field: String, name: String, values: Int*): PercentileRanksAggregation =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing values in scaladoc.

@vanjaftn vanjaftn force-pushed the task-support-percentile-rank-aggregation branch from 93dbe19 to 23ad012 Compare November 20, 2023 09:29
@vanjaftn vanjaftn force-pushed the task-support-percentile-rank-aggregation branch from aceb9b0 to 3bcdf89 Compare November 20, 2023 15:48
@dbulaja98 dbulaja98 merged commit 5b0dd30 into lambdaworks:main Nov 20, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants