Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Oct 26, 2020
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c525180
fix quantile argument not being passed through
vpranckaitis Oct 21, 2020
ff755dc
reorder cases
vpranckaitis Oct 21, 2020
0897d9f
formatting
vpranckaitis Oct 21, 2020
4adff54
change a test metric in queries.json
vpranckaitis Oct 21, 2020
352fc2a
add a reference to another issue
vpranckaitis Oct 21, 2020
952ab17
formatting
vpranckaitis Oct 21, 2020
127200b
PR comments
vpranckaitis Oct 23, 2020
9815297
fix quantile panic
vpranckaitis Oct 23, 2020
6373c85
PR comments
vpranckaitis Oct 23, 2020
c5b23bb
refactor test
vpranckaitis Oct 23, 2020
a15c2c8
Merge branch 'master' into vilius/pass_quantile_argument
vpranckaitis Oct 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 35 additions & 24 deletions scripts/comparator/basic_queries/queries.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,106 +1,117 @@
[
{
"queryGroup":"scalar",
"queries":[
"queryGroup": "scalar",
"queries": [
"42",
"time()"
],
"steps" : [
"steps": [
"1m"
]
},
{
"queryGroup":"fetch",
"queries":[
"queryGroup": "fetch",
"queries": [
"quail",
"quail offset 60s"
],
"steps" : [
"steps": [
"15s",
"30s",
"1m"
]
},
{
"queryGroup":"temporal",
"queries":[
"queryGroup": "temporal",
"queries": [
"rate(multi_1[1m])",
"irate(multi_1[5m])",
"delta(multi_1[123s])",
"idelta(multi_1[1m] offset 1h)",
"deriv(multi_1[3m])"
],
"steps" : [
"steps": [
"15s",
"30s",
"1m",
"5m"
]
},
{
"queryGroup":"binary",
"queries":[
"queryGroup": "binary",
"queries": [
"quail*1",
"1-quail",
"quail*quail",
"quail offset 1m / quail"
],
"steps" : [
"steps": [
"15s",
"30s",
"1m"
]
},
{
"queryGroup":"aggregation",
"queries":[
"queryGroup": "aggregation",
"queries": [
"sum(foobar{foobar=\"qux\"})",
"sum(foobar{foobar=\"qux\"}) - 1",
"sum(foobar{foobar=\"qux\"} offset 1m)"
],
"steps" : [
"steps": [
"15s",
"30s",
"1m"
]
},
{
"queryGroup":"transform",
"queries":[
"queryGroup": "transform",
"queries": [
"clamp_max(quail, 0.5)",
"clamp_min(quail offset 60s, 0.5)",
"sum(foobar{foobar=\"qux\"}) - 1",
"sum(foobar{foobar=\"qux\"} offset 1m)"
],
"steps" : [
"steps": [
"15s",
"30s",
"1m"
]
},
{
"queryGroup":"label",
"queries":[
"queryGroup": "label",
"queries": [
"label_replace(quail,\"foo\", \"$1!\", \"name\", \"(.*)\")",
"label_replace(quail offset 1m,\"foo\", \"$1!\", \"name\", \"(.*)\")",
"label_replace(quail,\"foo\", \"$1!\", \"name\", \"(.*)\")-100",
"label_join(quail,\"quince\", \"!\", \"foobar\", \"name\")"
],
"steps" : [
"steps": [
"15s",
"30s",
"1m"
]
},
{
"queryGroup":"topk",
"queryGroup": "topk",
"reruns": 5,
"queries":[
"queries": [
"topk(2, quack)",
"topk(2, avg_over_time(quack[30s]))",
"topk(2, avg(avg_over_time(quack[30s])) by (name))"
],
"steps" : [
"steps": [
"1m"
]
},
{
"queryGroup": "quantile",
"queries": [
"quantile(0, multi_10)",
"quantile(0.8, avg_over_time(multi_10[30s]))",
"quantile(1, avg(avg_over_time(multi_10[30s])) by (name))"
],
"steps": [
"1m"
]
}
Expand Down
24 changes: 16 additions & 8 deletions src/query/parser/promql/matchers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0897d9f#diff-fc8829d2c3cb3722ece0dc21d2fb4c8b2cf03230f5b641113b7413faf1660fa7R96-R118

Copy link
Collaborator

@arnikola arnikola Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case common.UnknownOpType:
return nil, fmt.Errorf("operator not supported: %s", opType)
}

if op == aggregation.BottomKType || op == aggregation.TopKType {
case aggregation.BottomKType, aggregation.TopKType:
val, err := resolveScalarArgument(expr.Param)
if err != nil {
return nil, err
}

nodeInformation.Parameter = val
return aggregation.NewTakeOp(op, nodeInformation)
}

if op == aggregation.CountValuesType {
case aggregation.CountValuesType:
nodeInformation.StringParameter = expr.Param.String()
return aggregation.NewCountValuesOp(op, nodeInformation)
}

return aggregation.NewAggregationOp(op, nodeInformation)
case aggregation.QuantileType:
linasm marked this conversation as resolved.
Show resolved Hide resolved
val, err := resolveScalarArgument(expr.Param)
if err != nil {
return nil, err
}

nodeInformation.Parameter = val
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return aggregation.NewAggregationOp(op, nodeInformation)

default:
return aggregation.NewAggregationOp(op, nodeInformation)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

127200b#diff-fc8829d2c3cb3722ece0dc21d2fb4c8b2cf03230f5b641113b7413faf1660fa7L117-R119

}

func getAggOpType(opType promql.ItemType) string {
Expand Down
11 changes: 5 additions & 6 deletions src/query/test/compatibility/testdata/aggregators.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,13 +286,12 @@ load 10s
data{test="uneven samples",point="c"} 4
foo .8

# FAILING issue #8 (quantile)
#eval instant at 1m quantile without(point)(0.8, data)
# {test="two samples"} 0.8
# {test="three samples"} 1.6
# {test="uneven samples"} 2.8
eval instant at 1m quantile without(point)(0.8, data)
{test="two samples"} 0.8
{test="three samples"} 1.6
{test="uneven samples"} 2.8

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
<...>

#eval instant at 1m quantile without(point)(scalar(foo), data)
# {test="two samples"} 0.8
# {test="three samples"} 1.6
Expand Down