[query] Adds histogram_quantile function #1373
Conversation
| "invalid number of args for histogram_quantile: %d", len(args)) | ||
| } | ||
|
|
||
| if opType != HistogramQuantileType { |
There was a problem hiding this comment.
Was copying the pattern from clamp.go
Wouldn't mind dealing with these correctly within the parser in a later pr, like the binary and aggregation functions are done at the moment.
|
|
||
| for index := 0; stepIter.Next(); index++ { | ||
| cloned := make([]float64, len(outValues)) | ||
| copy(cloned, outValues) |
There was a problem hiding this comment.
Why not just use outValues instead of cloning it?
There was a problem hiding this comment.
Nice catch, copied from the regular path, forgot to fix this one
| ) error { | ||
| sanitizeBuckets(bucketedSeries) | ||
|
|
||
| builder, err := setupBuilder(bucketedSeries, meta, stepIter, controller) |
There was a problem hiding this comment.
Can you reuse the BlockBuilder in m3db/m3/src/query/executor/transform/controller.go?
There was a problem hiding this comment.
Need to set a bunch of meta, seriesMetas, step counts unfortunately
| @@ -299,6 +299,10 @@ type TagOptionsConfiguration struct { | |||
| // If not provided, defaults to `__name__`. | |||
| MetricName string `yaml:"metricName"` | |||
|
|
|||
| // BucketName specifies the tag name that corresponds to the metric's bucket. | |||
| // If not provided, defaults to `le`. | |||
| {upperBound: 1, value: 1}, | ||
| {upperBound: math.Inf(1), value: 22}, | ||
| }) | ||
| assert.Equal(t, float64(1), actual) |
There was a problem hiding this comment.
Add test with neg infinity and > 2 buckets?
| ninf = math.Inf(-1) | ||
| ) | ||
|
|
||
| func TestQuantileFunctionFilteringWithoutA(t *testing.T) { |
There was a problem hiding this comment.
Oh I copied some setup stuff from an agg function that filtered by an A tag, will update this name to make more sense
|
|
||
| func testQuantileFunctionWithQ(t *testing.T, q float64) [][]float64 { | ||
| args := make([]interface{}, 0, 1) | ||
| args = append(args, q) |
There was a problem hiding this comment.
Just pass in the args instead of having to construct them here?
There was a problem hiding this comment.
Unfortunately it's pretty annoying to build a []interface{..}, and that's what we need to pass into the function; since we only ever need the q value here, it should be fine to build the interface slice here
|
|
||
| q := n.op.q | ||
| if q < 0 || q > 1 { | ||
| return processInvalidQuantile(q, bucketedSeries, meta, stepIter, n.controller) |
There was a problem hiding this comment.
Seems kind of weird to split this out like this. Maybe just have one function called processQuantile and put the inf logic in there.
There was a problem hiding this comment.
If we do that, will need to do the comparison for each step
| // options) that denotes the upper bound of that bucket; series without this | ||
| // tag are ignored. | ||
| HistogramQuantileType = "histogram_quantile" | ||
| initIndexBucketLength = 10 |
There was a problem hiding this comment.
Any specific reason to use 10?
There was a problem hiding this comment.
Seems to be the general convention for initing arrays we don't know the length of
Here it has (by chance) a bit more context in that the general rule of thumb for histograms is no more than 10 buckets
| values := step.Values() | ||
| bucketValues := make([]bucketValue, 0, initIndexBucketLength) | ||
|
|
||
| aggregatedValues := make([]float64, 0, len(bucketedSeries)) |
There was a problem hiding this comment.
You should be able to just pre-allocate this with a length and index in below when adding to it. Looks like you always add to it.
There was a problem hiding this comment.
Will do; was fine using append here since didn't want to add a tracking idx variable
| assert.True(t, found) | ||
| assert.Equal(t, value, actual) | ||
|
|
||
| value2 := []byte("abc") |
There was a problem hiding this comment.
Add test for empty val? e.g. []byte("")
There was a problem hiding this comment.
Sure; not sure if it's necessary a special case though
| if buckets, found := bucketsForID[string(id)]; !found { | ||
| // Add a single indexed bucket for this ID with the current index only. | ||
| newBuckets := make([]indexedBucket, 0, initIndexBucketLength) | ||
| newBucket := indexedBucket{ |
There was a problem hiding this comment.
Can pull this out before if buckets, found := bucketsForID[string(id)]; !found { since it gets created in both found and !found
There was a problem hiding this comment.
Good call; this was originally going to have a list of indices that are valid for that bucket but the logic got really awkward. Changed it to add a bucket/index combo every time instead, but didn't update here to reflect that
| tags: tagsWithoutKeys, | ||
| } | ||
| } else { | ||
| newBucket := indexedBucket{ |
| controller *transform.Controller, | ||
| ) (block.Builder, error) { | ||
| metas := make([]block.SeriesMeta, len(bucketedSeries)) | ||
| idx := 0 |
There was a problem hiding this comment.
Why have this var? Just include idx in the for loop below?
There was a problem hiding this comment.
It's a map not an array
| assert.Equal(t, [][]float64{{inf, inf, inf, inf, inf}}, actual) | ||
|
|
||
| actual = testQuantileFunctionWithQ(t, 0.8) | ||
| test.EqualsWithNansWithDelta(t, [][]float64{{15.6, 20, 11.6, 13.4, 0.8}}, actual, 0.00001) |
There was a problem hiding this comment.
Add a case with some NaNs
No description provided.