[query] Fix quantile() argument not being passed through #2780

vpranckaitis · 2020-10-21T12:53:27Z

What this PR does / why we need it:

Fix quantile() argument not being passed through when processing promql expression. This improves m3query compatibility

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

CLAassistant · 2020-10-21T12:53:33Z

All committers have signed the CLA.

codecov · 2020-10-21T13:19:26Z

Codecov Report

Merging #2780 into master will decrease coverage by 0.9%.
The diff coverage is 0.0%.

@@           Coverage Diff            @@
##           master   #2780     +/-   ##
========================================
- Coverage    61.5%   60.6%   -1.0%     
========================================
  Files        1015    1015             
  Lines       92824   92920     +96     
========================================
- Hits        57106   56315    -791     
- Misses      31220   32184    +964     
+ Partials     4498    4421     -77

Flag	Coverage Δ
#aggregator	`62.7% <ø> (-1.6%)`	⬇️
#cluster	`77.6% <ø> (ø)`
#collector	`64.3% <ø> (ø)`
#dbnode	`63.7% <ø> (+<0.1%)`	⬆️
#m3em	`48.8% <ø> (-18.8%)`	⬇️
#m3ninx	`60.9% <ø> (-8.6%)`	⬇️
#m3nsch	`51.1% <ø> (ø)`
#msg	`70.6% <ø> (+0.1%)`	⬆️
#query	`48.7% <0.0%> (-0.1%)`	⬇️
#x	`63.0% <ø> (-1.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cca152...c5b23bb. Read the comment docs.

src/query/parser/promql/matchers.go

linasm · 2020-10-21T13:40:20Z

src/query/parser/promql/matchers.go

@@ -90,27 +90,31 @@ func NewAggregationOperator(expr *promql.AggregateExpr) (parser.Params, error) {
 		Without:      expr.Without,
 	}

-	op := getAggOpType(opType)
-	if op == common.UnknownOpType {
+	switch op := getAggOpType(opType); op {


Nit: this switch has multi-line case blocks, would be a bit easier on the eyes if they were separated with empty lines.

0897d9f#diff-fc8829d2c3cb3722ece0dc21d2fb4c8b2cf03230f5b641113b7413faf1660fa7R96-R118

linasm · 2020-10-21T13:44:58Z

scripts/comparator/basic_queries/queries.json

+      "quantile(0, quack)",
+      "quantile(0.8, avg_over_time(quack[30s]))",
+      "quantile(1, avg(avg_over_time(quack[30s])) by (name))"


Better use multi_10 metrics, it will give you 10 series.

linasm · 2020-10-21T14:10:35Z

scripts/comparator/basic_queries/queries.json

+    "queryGroup":"quantile",
+    "queries":[


Mini nit:

Suggested change

"queryGroup":"quantile",

"queries":[

"queryGroup": "quantile",

"queries": [

0897d9f#diff-16b78a7d0d764b2c97fa4117d319f3c792af57eeb763fef2c11576f330d11cb6L108-R109

linasm · 2020-10-21T14:10:49Z

scripts/comparator/basic_queries/queries.json

+      "quantile(0.8, avg_over_time(quack[30s]))",
+      "quantile(1, avg(avg_over_time(quack[30s])) by (name))"
+    ],
+    "steps" : [


Mini nit:

Suggested change

"steps" : [

"steps": [

952ab17#diff-16b78a7d0d764b2c97fa4117d319f3c792af57eeb763fef2c11576f330d11cb6L114-R114

arnikola · 2020-10-22T14:47:29Z

src/query/parser/promql/matchers.go

@@ -90,27 +90,35 @@ func NewAggregationOperator(expr *promql.AggregateExpr) (parser.Params, error) {
 		Without:      expr.Without,
 	}

-	op := getAggOpType(opType)
-	if op == common.UnknownOpType {
+	switch op := getAggOpType(opType); op {


nit: switch getAggOpType(opType) { is a simpler way of doing this

Suggested change

switch op := getAggOpType(opType); op {

switch getAggOpType(opType) {

127200b#diff-fc8829d2c3cb3722ece0dc21d2fb4c8b2cf03230f5b641113b7413faf1660fa7L93-R94 , though op is still needed

arnikola · 2020-10-22T14:50:38Z

src/query/parser/promql/matchers.go

+		val, err := resolveScalarArgument(expr.Param)
+		if err != nil {
+			return nil, err
+		}
+
+		nodeInformation.Parameter = val
+		return aggregation.NewAggregationOp(op, nodeInformation)
+
+	default:
+		return aggregation.NewAggregationOp(op, nodeInformation)
+	}


nit; Don't think you need the default case or the special return here if you move the aggregation.NewAggregationOp(op, nodeInformation) out of the switch

127200b#diff-fc8829d2c3cb3722ece0dc21d2fb4c8b2cf03230f5b641113b7413faf1660fa7L117-R119

arnikola · 2020-10-22T14:52:44Z

src/query/test/compatibility/testdata/aggregators.test


-# Bug #5276.
+# FAILING issue #56. scalar() inside aggregate function arguments kills the server


What is meant by killing the server? Did this change make the failure in this case worse than it was before?

It seems that quantile(scalar(foo), ...) eventually becomes quantile(NaN, ...), which causes panic in following computations. I've added a check if q == NaN and a unit test, also updated the comment 9815297

panic: runtime error: index out of range [-9223372036854775808] goroutine 13088 [running]: github.com/m3db/m3/src/query/functions/aggregation.quantileFn(0x7ff8000000000001, 0xc007d7e370, 0xa, 0xa, 0xc00d2a11a0) go/src/github.com/m3db/m3/src/query/functions/aggregation/quantile.go:86 +0x1d5 github.com/m3db/m3/src/query/functions/aggregation.bucketedQuantileFn(0x7ff8000000000001, 0xc007d7e050, 0xa, 0xa, 0xc0005d8380, 0xa, 0x10, 0xa) go/src/github.com/m3db/m3/src/query/functions/aggregation/quantile.go:67 +0x17a github.com/m3db/m3/src/query/functions/aggregation.makeQuantileFn.func1(0xc007d7e050, 0xa, 0xa, 0xc0005d8380, 0xa, 0x10, 0x0) go/src/github.com/m3db/m3/src/query/functions/aggregation/quantile.go:42 +0x68 github.com/m3db/m3/src/query/functions/aggregation.(*baseNode).ProcessBlock(0xc00dbbb8c0, 0xc0060bdef0, 0x298a4f0, 0x1, 0x1ce15e0, 0xc00a19a600, 0x1, 0x1cf2680, 0x2a210e0, 0x1cd1540) go/src/github.com/m3db/m3/src/query/functions/aggregation/base.go:163 +0x480 <...>

arnikola · 2020-10-22T15:13:12Z

src/query/parser/promql/matchers.go

+			return nil, err
+		}
+
+		nodeInformation.Parameter = val


May be worth adding a test for this; may be a bit complex though since it would need to generate the op, get the node, then create a mock block and run it against functions (since we can't really inspect this otherwise; maybe add a NodeInformation() on parser.Params? Although that may lead to a deeper rabbithole)

We've discussed this with @linasm and reached a similar conclusion about it being not very straightforward. For now, the tests in aggregators.test and queries.json should provide some coverage. I may look into this further in a follow-up PR.

arnikola

Approved /w nits

linasm

Well done overall, had a few minor suggestions.

linasm · 2020-10-23T08:23:39Z

src/query/functions/aggregation/quantile.go

@@ -48,6 +48,10 @@ func bucketedQuantileFn(q float64, values []float64, bucket []int) float64 {
 		return math.NaN()
 	}

+	if math.IsNaN(q) {


Merge it with the if above.

6373c85#diff-a96271f005e4a6ee9a53e40d2c33b5eb7e51334213e8566789b9df5a824fa127L47-R47

linasm · 2020-10-23T08:27:36Z

src/query/functions/aggregation/quantile_test.go

+	expected := math.NaN()
+	actual := bucketedQuantileFn(math.NaN(), values, buckets)
+
+	test.EqualsWithNans(t, expected, actual)


Just make it a more direct require.True(t, math.IsNaN(actual))

6373c85#diff-001d927cd799353c8ec7e856337bb2124f50838d621944fab48ad14e605fcab5R128

linasm · 2020-10-23T08:30:03Z

src/query/functions/aggregation/quantile_test.go

+	buckets := []int{0, 1}
+
+	expected := math.NaN()
+	actual := bucketedQuantileFn(math.NaN(), values, buckets)


Also please cover the cases with empty values and empty buckets (if they are not currently covered).
Can do it in the same test function, especially if you inline those arguments a bit.

6373c85#diff-001d927cd799353c8ec7e856337bb2124f50838d621944fab48ad14e605fcab5R122-R125

linasm · 2020-10-23T10:23:34Z

src/query/functions/aggregation/quantile_test.go

@@ -117,6 +117,18 @@ func TestQuantileFnSingleNonNan(t *testing.T) {
 	test.EqualsWithNansWithDelta(t, expected, actual, math.Pow10(-5))
 }

+func TestQuantileNanAndEmptyArguments(t *testing.T) {
+	actual := []float64{


This is a good idea, but it will not report which case(s) fail.
There is a more standard approach for such tests in go, called 'test tables`, eg.:

m3/src/dbnode/ts/downsample/counter_resets_downsampler_test.go

Line 31 in 12dc12e

tests := []struct {

linasm

LGTM

vpranckaitis · 2020-10-23T11:10:12Z

@arnikola since I made quite a few changes while addressing the comments, I'm re-adding you for review

arnikola

lgtm

* master: Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) # Conflicts: # src/query/parser/promql/matchers.go

* master: [query] Improve precision for variance and stddev of equal values (#2799) Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) [query] Add "median" aggregation to Graphite aggregate() function (#2774) [r2] Ensure KeepOriginal is propagated when adding/reviving rules (#2796) [dbnode] Update default db read limits (#2784)

fix quantile argument not being passed through

c525180

vpranckaitis changed the title ~~[query] Fix quantile argument not being passed through~~ [query] Fix quantile() argument not being passed through Oct 21, 2020

vpranckaitis requested a review from linasm October 21, 2020 13:31

linasm reviewed Oct 21, 2020

View reviewed changes

vpranckaitis added 5 commits October 21, 2020 17:14

reorder cases

ff755dc

formatting

0897d9f

change a test metric in queries.json

4adff54

add a reference to another issue

352fc2a

formatting

952ab17

vpranckaitis requested a review from arnikola October 21, 2020 15:07

vpranckaitis assigned arnikola Oct 22, 2020

arnikola reviewed Oct 22, 2020

View reviewed changes

arnikola approved these changes Oct 22, 2020

View reviewed changes

arnikola removed their assignment Oct 22, 2020

vpranckaitis added 2 commits October 23, 2020 10:46

PR comments

127200b

fix quantile panic

9815297

vpranckaitis requested a review from linasm October 23, 2020 08:20

vpranckaitis assigned linasm Oct 23, 2020

linasm reviewed Oct 23, 2020

View reviewed changes

PR comments

6373c85

linasm reviewed Oct 23, 2020

View reviewed changes

refactor test

c5b23bb

linasm reviewed Oct 23, 2020

View reviewed changes

vpranckaitis unassigned linasm Oct 23, 2020

vpranckaitis requested a review from arnikola October 23, 2020 11:10

vpranckaitis assigned arnikola Oct 23, 2020

arnikola removed their assignment Oct 26, 2020

arnikola approved these changes Oct 26, 2020

View reviewed changes

Merge branch 'master' into vilius/pass_quantile_argument

a15c2c8

vpranckaitis merged commit d47dcc3 into master Oct 26, 2020

vpranckaitis deleted the vilius/pass_quantile_argument branch October 26, 2020 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Fix quantile() argument not being passed through #2780

[query] Fix quantile() argument not being passed through #2780

vpranckaitis commented Oct 21, 2020

CLAassistant commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

linasm Oct 21, 2020

vpranckaitis Oct 21, 2020

linasm Oct 21, 2020

vpranckaitis Oct 21, 2020

linasm Oct 21, 2020

vpranckaitis Oct 21, 2020

linasm Oct 21, 2020

vpranckaitis Oct 21, 2020

arnikola Oct 22, 2020 •

edited

Loading

vpranckaitis Oct 22, 2020

arnikola Oct 22, 2020

vpranckaitis Oct 23, 2020

arnikola Oct 22, 2020

vpranckaitis Oct 23, 2020

arnikola Oct 22, 2020

vpranckaitis Oct 23, 2020

arnikola left a comment

linasm left a comment

linasm Oct 23, 2020

vpranckaitis Oct 23, 2020

linasm Oct 23, 2020

vpranckaitis Oct 23, 2020

linasm Oct 23, 2020

vpranckaitis Oct 23, 2020

linasm Oct 23, 2020

vpranckaitis Oct 23, 2020

linasm left a comment

vpranckaitis commented Oct 23, 2020

arnikola left a comment

	switch op := getAggOpType(opType); op {
	switch getAggOpType(opType) {


		# Bug #5276.
		# FAILING issue #56. scalar() inside aggregate function arguments kills the server

[query] Fix quantile() argument not being passed through #2780

[query] Fix quantile() argument not being passed through #2780

Conversation

vpranckaitis commented Oct 21, 2020

CLAassistant commented Oct 21, 2020 • edited Loading

codecov bot commented Oct 21, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

vpranckaitis commented Oct 23, 2020

arnikola left a comment

Choose a reason for hiding this comment

CLAassistant commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

arnikola Oct 22, 2020 •

edited

Loading