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

Add arithmetic post aggregations #29

Closed tonipenya closed 6 years ago

tonipenya commented 6 years ago

It also adds license to DruidQuerySpec' header since scalastyle's current configuration requires it.

tonipenya commented 6 years ago

My older comment got deleted (looks like gitlab does not play well with git push -f)

Reading druid's documentation with more attention, turns out that what I was calling Field is actually a post aggregator.

I've renamed both Field and FieldType to PostAggregator and PostAggregatorType respectively.

I am in doubt whether to move them to a separate file or keep them with PostAggregation since the post aggregators are specific to post aggregations. What would you do?

bjgbeelen commented 6 years ago

That sounds valid, I'm OK with the code. No necessity to place this within the PostAggregation companion object now. To me this feels grouped now, so no need to place that part in a separate file.

LGTM

vkorchik commented 6 years ago

Hey guys, @tonipenya @bjgbeelen

I have one question about PostAggregator and PostAggregation: what is the difference between them? Here I see two exactly the same traits, that are derived by the same in theory things - post-aggregations (arithmetic, constant, fieldAccessor, etc.).

Why does current impl divide Arithmetic from Constant and FieldAccessor? Why does Arithmetic accept only two post-aggregation, when it can accept any amount of them? e.g. when we want to sum several (more than 2) aggregations together. Why does Arithmetic accepts only PostAggregators and not PostAggregations? It means that Arithmetic cannot accept itself, while in reality it can.

Thanks in advance :)

tonipenya commented 6 years ago

Really good questions, @vkorchik

The answer to most of your questions is: Because I didn't understand Druid's documentation. In my head, a post-aggregation defined an operation over some data and a post-aggregator defined where that data came from. Clearly, it isn't that way.

I'll see if I can find some time today to put together a PR to:

vkorchik commented 6 years ago

@tonipenya , yes, they have not the best documentation.

Well, about PR: I have exactly these changes already (I have done them long ago, but was ut of time to finish and polish them, to write tests and so on). I guess I will create new PR and, please, have a look at it too. 🙏

tonipenya commented 6 years ago

Great!

@ mention me so I don't miss it, plz.