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 Value count aggregation #352

Merged

Conversation

vanjaftn
Copy link
Contributor

@vanjaftn vanjaftn commented Nov 1, 2023

Part of #118

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.

Overall nice job!
Just two things:

  1. Add this aggregation to match case at lines 142. and 175. in AggregationResponse file.
  2. Please test this aggregation as a sub aggregation of terms aggregation (just to make sure it works properly)

val multipleAggregations: MultipleAggregations = valueCountAggregation(name = "valueCountAggregation1", field = Document.stringField).withAgg(valueCountAggregation(name = "valueCountAggregation2", field = Document.stringField))
```

You can find more information about `Value count` aggregation [here](https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-metrics-valuecount-aggregation.html#search-aggregations-metrics-valuecount-aggregation).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add empty line at the end of the file if you didn't.

* an instance of [[zio.elasticsearch.aggregation.ValueCountAggregation]] that represents value count aggregation to
* be performed.
*/
final def valueCountAggregation(name: String, field: Field[_, String]): ValueCountAggregation =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that Value count aggregation can be executed on non-textual field. Please check this and fix if necessary.

@@ -289,7 +289,8 @@ private[elasticsearch] final case class Terms(
order: Chunk[AggregationOrder],
subAggregations: Chunk[SingleElasticAggregation],
size: Option[Int]
) extends TermsAggregation { self =>
) extends TermsAggregation {
self =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put self in the line above.

sealed trait ValueCountAggregation extends SingleElasticAggregation with WithAgg

private[elasticsearch] final case class ValueCount(name: String, field: String) extends ValueCountAggregation {
self =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move self to the line above?


In order to use the `Value count` aggregation import the following:
```scala
import zio.elasticsearch.aggregation.valueCountAggregation
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
import zio.elasticsearch.aggregation.valueCountAggregation
import zio.elasticsearch.aggregation.ValueCountAggregation


If you want to add aggregation (on the same level), you can use `withAgg` method:
```scala
val multipleAggregations: MultipleAggregations = valueCountAggregation(name = "valueCountAggregation1", field = Document.stringField).withAgg(valueCountAggregation(name = "valueCountAggregation2", field = Document.stringField))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe change field in valueCountAggregation2.

)
.refreshTrue
)
aggregation = valueCountAggregation("aggregation", TestDocument.stringField.keyword)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name params.

@dbulaja98 dbulaja98 changed the title (dsl): Support value count aggregation (dsl): Support Value count aggregation Nov 3, 2023
@dbulaja98 dbulaja98 changed the title (dsl): Support Value count aggregation (dsl): Support Value count aggregation Nov 3, 2023
@dbulaja98 dbulaja98 merged commit e6ce645 into lambdaworks:main Nov 3, 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.

None yet

2 participants