search

ing-bank / scruid

Scala + Druid: Scruid. A library that allows you to compose queries in Scala, and parse the result back into typesafe classes.
Apache License 2.0
115 stars 29 forks source link

Post-aggregations #31

Closed vkorchik closed 5 years ago

vkorchik commented 6 years ago

1) Added all post-aggregations from Druid documetation except just one: HyperUnique Cardinality. Simply because we dont have aggregations of the same type, and not to confuse users. Anyway, it can be added later without any troubles. 2) Arithmetic functions for ArithmeticPostAggregation are coded as a separate family of case classes. It gives users only the defined set of functions to choose from, and not put "blah-blah" into function field. 3) Added implicit conversions for arithmetic functions, as it have been already done for Filters and Havings. Precedence of operators of course is preserved, but remember that quotient has the lowest precedence, when using implicit conversions.

vkorchik commented 6 years ago

@tonipenya, here we go

tonipenya commented 6 years ago

The implementation looks great!

I'll integrate this with our code tomorrow just to see how it looks.

tonipenya commented 6 years ago

The implementation on this MR is not backwards compatible with what was done before post-aggregation wise. The changes needed are minimal though.

IMHO the benefits outweight the drawbacks and, at least for us, the change is worth it. 👍

Fokko commented 6 years ago

@vkorchik Can you clean up the unused imports?

[warn] /home/ubuntu/scruid/src/main/scala/ing/wbaa/druid/DruidResponse.scala:25:27: Unused import
[warn] import cats.syntax.either._
[warn]                           ^
[warn] /home/ubuntu/scruid/src/main/scala/ing/wbaa/druid/Enum.scala:22:27: Unused import
[warn] import cats.syntax.either._
[warn]

@tonipenya Let me know if you like the change

vkorchik commented 6 years ago

Guys, continue without me, i wont be available for the next two weeks. If PR is ok - merge it, if not - well, then not :)

Fokko commented 6 years ago

@bjgbeelen @krisgeus Any problems with the backward incompatibility?

krisgeus commented 6 years ago

@Fokko It is only backward incompatibility in the post aggregator stuff. And @tonipenya already mentioned the changes are minor and outweigh the benefits. So no objection. To be good release managers we can bump the major version for this.....

Fokko commented 6 years ago

@krisgeus Awesome, I've retriggered the build.