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 Filter aggregation #349

Merged
merged 12 commits into from
Nov 17, 2023

Conversation

vanjaftn
Copy link
Contributor

@vanjaftn vanjaftn commented Oct 31, 2023

Part of #118

@vanjaftn vanjaftn force-pushed the task-support-filter-aggregation branch 2 times, most recently from 7ebeec7 to cb25165 Compare November 1, 2023 14:31
subAggregations.get(aggName) match {
case Some(aggRes) =>
Try(aggRes.asInstanceOf[A]) match {
case Failure(_) => Left(DecodingException(s"Aggregation with name $aggName was not of type you provided."))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check will probably not work here because of type erasure: at runtime the type A is not known here, so asInstanceOf will allow anything and a cast error will most likely happen later in user code. An example to demonstrate the case: https://scastie.scala-lang.org/kyL2fVL2S8eQByJb4aS1Yg

@vanjaftn vanjaftn force-pushed the task-support-filter-aggregation branch from 5726f35 to 0627b78 Compare November 7, 2023 12:41
@vanjaftn vanjaftn changed the title [DRAFT]: (dsl): Support filter aggregation (dsl): Support filter aggregation Nov 7, 2023
@dbulaja98 dbulaja98 marked this pull request as ready for review November 7, 2023 13:11
@dbulaja98 dbulaja98 changed the title (dsl): Support filter aggregation (dsl): Support Filter aggregation Nov 12, 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.

You will have to update this PR with the newly added aggregations.

```

You can create a `Filter` aggregation using the `filterAggregation` method in the following manner:
```scala
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the import for term query.

@@ -143,6 +144,31 @@ private[elasticsearch] final case class Cardinality(name: String, field: String,
}
}

sealed trait FilterAggregation extends SingleElasticAggregation with WithSubAgg[FilterAggregation] with WithAgg
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reorder traits (WithSubAgg and WithAgg).

Comment on lines 127 to 131
value => {
ZIO.succeed(new AggregateResult(value.aggs.map { case (key, response) =>
(key, toResult(response))
}))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Omit { and }.

Comment on lines 88 to 170
private[elasticsearch] final case class StatsAggregationResponse(
count: Int,
min: Double,
max: Double,
avg: Double,
sum: Double
) extends AggregationResponse

private[elasticsearch] object StatsAggregationResponse {
implicit val decoder: JsonDecoder[StatsAggregationResponse] = DeriveJsonDecoder.gen[StatsAggregationResponse]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep it ordered.

@@ -31,6 +31,16 @@ object AggregationResponse {
AvgAggregationResult(value)
case CardinalityAggregationResponse(value) =>
CardinalityAggregationResult(value)
case FilterAggregationResponse(docCount, subAggregations) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Omit { and }.

Comment on lines 145 to 146
subAggregations = Chunk.empty,
query = query
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep params ordered.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In other examples too.

@@ -104,6 +104,67 @@ object HttpExecutorSpec extends IntegrationSpec {
Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
),
test("aggregate using filter aggregation") {
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
test("aggregate using filter aggregation") {
test("aggregate using filter aggregation with max aggregation as a sub aggregation") {

@@ -104,6 +104,67 @@ object HttpExecutorSpec extends IntegrationSpec {
Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
),
test("aggregate using filter aggregation") {
val expectedResponse = (
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
val expectedResponse = (
val expectedResult = (

```

If you want to add aggregation (on the same level), you can use `withAgg` method:
```scala
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

```

If you want to add another sub-aggregation, you can use `withSubAgg` method:
```scala
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here add also for maxAggregation too.

@vanjaftn vanjaftn force-pushed the task-support-filter-aggregation branch from a6efadf to 30edae8 Compare November 14, 2023 10:29
.refreshTrue
)
query = term(field = TestDocument.stringField, value = secondDocumentUpdated.stringField.toLowerCase)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Omit empty line.

filterAggregation(name = "aggregation", query = query).withSubAgg(
maxAggregation("subAggregation", TestDocument.intField)
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Omit empty line.

@@ -120,6 +224,7 @@ private[elasticsearch] final case class SumAggregationResponse(value: Double) ex

private[elasticsearch] object SumAggregationResponse {
implicit val decoder: JsonDecoder[SumAggregationResponse] = DeriveJsonDecoder.gen[SumAggregationResponse]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Omit empty line.

filterAggregation("aggregation", query).withSubAgg(minAggregation("subAggregation", TestDocument.intField))
val aggregationWithMultipleSubAggregations = filterAggregation("aggregation", query)
.withSubAgg(maxAggregation("maxSubAggregation", TestDocument.intField))
.withSubAgg(minAggregation("minSubAggregation", TestDocument.intField))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put other numeric field (not intField) for minAggregation.

filterAggregation("aggregation", query).withSubAgg(minAggregation("subAggregation", TestDocument.intField))
val aggregationWithMultipleSubAggregations = filterAggregation("aggregation", query)
.withSubAgg(maxAggregation("maxSubAggregation", TestDocument.intField))
.withSubAgg(minAggregation("minSubAggregation", TestDocument.intField))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@vanjaftn vanjaftn force-pushed the task-support-filter-aggregation branch 2 times, most recently from c058f2c to 5914223 Compare November 17, 2023 09:27
@vanjaftn vanjaftn force-pushed the task-support-filter-aggregation branch from 4a374b4 to 422b24b Compare November 17, 2023 09:30
@dbulaja98 dbulaja98 merged commit 8c55381 into lambdaworks:main Nov 17, 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

3 participants