-
Notifications
You must be signed in to change notification settings - Fork 20
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
base: main
Are you sure you want to change the base?
Conversation
@Mitep, are you still working on this? |
I didn't, I'll try to rebase it and see where we at |
58de7eb
to
11f344d
Compare
11f344d
to
c641f1a
Compare
website/sidebars.js
Outdated
@@ -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', |
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
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.
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 | ||
|
There was a problem hiding this comment.
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]] = |
There was a problem hiding this comment.
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#") => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
range: SingleRange, | ||
ranges: SingleRange* |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
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) | ||
|
||
} |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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.
No description provided.