Wire up Top aggregate function #3930
Conversation
| } | ||
|
|
||
| // Secondly, determine if specific calls have at least one and only one argument | ||
| // Secondly, determine if specific calls have the correct number of arguments |
There was a problem hiding this comment.
We were validating aggregate functions in functions.go as well, and this was all work we could do ahead of time, plus it was fragmenting who was responsible for validating. This moves it all up before we even start mapping calls and falls earlier.
corylanou
force-pushed
the
top
branch
2 times, most recently
from
21eafb0 to
ae1ef89
Compare
| @@ -0,0 +1,37 @@ | |||
| package slices | |||
There was a problem hiding this comment.
I'm not a fan of utility packages, so if anyone has a better suggestion on where to put this/name it fire away.
There was a problem hiding this comment.
I've seen some projects use pkg as the root for utility packages that are usable outside of the project. See https://github.com/docker/docker/tree/master/pkg, https://github.com/coreos/etcd/tree/master/pkg and https://github.com/kubernetes/kubernetes/tree/master/pkg. They sometimes put a README.md in there as well. I kind of like pkg as opposed to helpers.
corylanou
force-pushed
the
top
branch
2 times, most recently
from
e5146ee to
f199dc5
Compare
| } | ||
| callArgs := c.Fields() | ||
| tags := itr.Tags() | ||
| // TODO send fields in |
There was a problem hiding this comment.
No, we are doing it in a future PR, I left it in as a future TODO. Is there a better way to note that?
There was a problem hiding this comment.
I don't know what the TODO actually means. Maybe elaborate a bit more on what is missing and needs to be done?
|
A few small things but LGTM. 👍. |
| @@ -11,6 +11,8 @@ With this release InfluxDB is moving to Go 1.5. | |||
| - [#3876](https://github.com/influxdb/influxdb/pull/3876): Allow the following syntax in CQs: INTO "1hPolicy".:MEASUREMENT | |||
| - [#3975](https://github.com/influxdb/influxdb/pull/3975): Add shard copy service | |||
| - [#3986](https://github.com/influxdb/influxdb/pull/3986): Support sorting by time desc | |||
| - [#3930](https://github.com/influxdb/influxdb/pull/3930): Wire up TOP aggregate function | |||
There was a problem hiding this comment.
Duplicating this in the CHANGELOG looks wrong to me. I think it's better to keep the first line, and then add "fixes #1821" to the line.
There was a problem hiding this comment.
agreed, I'll make the change.
|
OK, took a look, my understand of the work required isn't super-deep, but we have decent testing around this so it should be OK. +1, some minor feedback. |
|
@otoolep addressed feedback. |
|
Great, the feedback was minor so merge away on green. |
Wire up Top aggregate function
|
Is this feature also nestable with aggregates? I.e. |
|
@corylanou Going from the comments on a few issues, should we be able to run Testing this on 0.9.4.2 which the changelog mentions it should be possible (I think) however it does accept a nested aggregate. |
|
You can not do a nested aggregate in top currently, |
Issue
This addresses #1821
Also of interest (but not solved for in this PR) is Bottom: #1820
Aggregate Validations
We were doing aggregate validations in two separate spots. This PR consolidates all of it to fail faster in the parser validation, not in the function mapping validation.