Skip to content

Add support for range agg #272

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Mitep
Copy link
Contributor

@Mitep Mitep commented Jun 27, 2023

No description provided.

@drmarjanovic
Copy link
Member

@Mitep, are you still working on this?

@drmarjanovic drmarjanovic marked this pull request as ready for review February 8, 2024 20:27
@drmarjanovic drmarjanovic marked this pull request as draft February 8, 2024 20:45
@Mitep
Copy link
Contributor Author

Mitep commented Jun 5, 2025

@Mitep, are you still working on this?

I didn't, I'll try to rebase it and see where we at

@Mitep Mitep closed this Jun 7, 2025
@Mitep Mitep deleted the task-add-support-for-range-agg branch June 7, 2025 12:52
@Mitep Mitep restored the task-add-support-for-range-agg branch June 7, 2025 12:52
@Mitep Mitep reopened this Jun 7, 2025
@Mitep Mitep force-pushed the task-add-support-for-range-agg branch 2 times, most recently from 58de7eb to 11f344d Compare June 14, 2025 16:12
@Mitep Mitep force-pushed the task-add-support-for-range-agg branch from 11f344d to c641f1a Compare June 14, 2025 16:17
@Mitep Mitep marked this pull request as ready for review June 15, 2025 07:54
@@ -55,6 +55,7 @@ module.exports = {
'overview/aggregations/elastic_aggregation_filter',
'overview/aggregations/elastic_aggregation_max',
'overview/aggregations/elastic_aggregation_min',
'overview/aggregations/elastic_aggregation_range',
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this alphabetically sorted.

@@ -1053,6 +1150,123 @@ object ElasticAggregationSpec extends ZIOSpecDefault {
assert(aggregationTs.toJson)(equalTo(expectedTs.toJson)) &&
assert(aggregationWithMissing.toJson)(equalTo(expectedWithMissing.toJson))
},
test("range") {
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this alphabetically sorted.

Copy link
Member

Choose a reason for hiding this comment

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

Also, can you provide "type-safe" tests? Using TestDocument.stringField instead of "testField".

@@ -216,6 +216,103 @@ object ElasticAggregationSpec extends ZIOSpecDefault {
equalTo(Min(name = "aggregation", field = "intField", missing = Some(20.0)))
)
},
test("range") {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep this alphabetically sorted, and provide type-safe examples.

Comment on lines +60 to +82
private[elasticsearch] sealed trait RangeAggregationResult extends AggregationResult

private[elasticsearch] final case class RegularRangeAggregationBucketResult(
key: String,
to: Option[Double],
from: Option[Double],
docCount: Int
)

private[elasticsearch] final case class KeyedRangeAggregationBucketResult(
to: Option[Double],
from: Option[Double],
docCount: Int
)

private[elasticsearch] final case class RegularRangeAggregationResult(
buckets: Chunk[RegularRangeAggregationBucketResult]
) extends RangeAggregationResult

private[elasticsearch] final case class KeyedRangeAggregationResult(
buckets: Map[String, KeyedRangeAggregationBucketResult]
) extends RangeAggregationResult

Copy link
Member

Choose a reason for hiding this comment

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

Keep this alphabetically sorted.

* a [[RIO]] effect that, when executed, will produce the aggregation as instance of
* [[result.RangeAggregationResult]].
*/
def asRangeAggregation(name: String): RIO[R, Option[RangeAggregationResult]] =
Copy link
Member

Choose a reason for hiding this comment

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

Keep this alphabetically sorted.

@@ -86,6 +86,8 @@ private[elasticsearch] final case class SearchWithAggregationsResponse(
MaxAggregationResponse.decoder.decodeJson(data.toString).map(field.split("#")(1) -> _)
case str if str.contains("min#") =>
MinAggregationResponse.decoder.decodeJson(data.toString).map(field.split("#")(1) -> _)
case str if str.contains("range#") =>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Comment on lines +240 to +241
range: SingleRange,
ranges: SingleRange*
Copy link
Member

Choose a reason for hiding this comment

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

Why is this named SingleRange? Why not Range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot have two case classes with the same name inside the same package. I don't like the SingleRange name either, but I can't think of a better approach right now.

Comment on lines 244 to 265
private[elasticsearch] final case class SingleRange(
from: Option[Double],
to: Option[Double],
key: Option[String]
) { self =>
def from(value: Double): SingleRange = self.copy(from = Some(value))
def to(value: Double): SingleRange = self.copy(to = Some(value))
def key(value: String): SingleRange = self.copy(key = Some(value))
}

object SingleRange {

def from(value: Double): SingleRange =
SingleRange(from = Some(value), to = None, key = None)

def to(value: Double): SingleRange =
SingleRange(from = None, to = Some(value), key = None)

def apply(from: Double, to: Double): SingleRange =
SingleRange(from = Some(from), to = Some(to), key = None)

}
Copy link
Member

Choose a reason for hiding this comment

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

I would say we have to create an API for this.

keyed: Option[Boolean]
) extends RangeAggregation { self =>

def keyed(value: Boolean): Range =
Copy link
Member

Choose a reason for hiding this comment

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

Dose it make sense to have keyed and notKeyed instead of having keyed(value: Boolean)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but we cannot have keyed as def while having keyed as field, so I put asKeyed as def name.

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