-
Notifications
You must be signed in to change notification settings - Fork 74
all: make coalesce a logical node #587
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| // Copyright (c) The Thanos Community Authors. | ||
| // Licensed under the Apache License 2.0. | ||
|
|
||
| package logicalplan | ||
|
|
||
| import ( | ||
| "github.com/prometheus/prometheus/util/annotations" | ||
|
|
||
| "github.com/thanos-io/promql-engine/query" | ||
| ) | ||
|
|
||
| type CoalesceOptimizer struct{} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the optimizer be called something like ConcurrentDecodeOptimizer?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the idea is to allow for things like down the line.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a different optimizer should be responsible for sharding aggregations? |
||
|
|
||
| func (c CoalesceOptimizer) Optimize(expr Node, opts *query.Options) (Node, annotations.Annotations) { | ||
| numShards := opts.NumShards() | ||
|
|
||
| TraverseBottomUp(nil, &expr, func(parent, e *Node) bool { | ||
| switch t := (*e).(type) { | ||
| case *VectorSelector: | ||
| if parent != nil { | ||
| // we coalesce matrix selectors in a different branch | ||
| if _, ok := (*parent).(*MatrixSelector); ok { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about subqueries?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can parent of vectorselector be a subquery? I guess you could have |
||
| return false | ||
| } | ||
| } | ||
| exprs := make([]Node, numShards) | ||
| for i := range numShards { | ||
| vs := t.Clone().(*VectorSelector) | ||
| vs.Shard = i | ||
| vs.NumShards = numShards | ||
| exprs[i] = vs | ||
| } | ||
| *e = &Coalesce{Exprs: exprs} | ||
| case *MatrixSelector: | ||
yeya24 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // handled in *parser.Call branch | ||
| return false | ||
| case *FunctionCall: | ||
| // non-recursively handled in execution.go | ||
| if t.Func.Name == "absent_over_time" { | ||
| return true | ||
| } | ||
| var ( | ||
| ms *MatrixSelector | ||
| marg int | ||
| ) | ||
| for i := range t.Args { | ||
| if arg, ok := t.Args[i].(*MatrixSelector); ok { | ||
| ms = arg | ||
| marg = i | ||
| } | ||
| } | ||
| if ms == nil { | ||
| return false | ||
| } | ||
| exprs := make([]Node, numShards) | ||
| for i := range numShards { | ||
| aux := ms.Clone().(*MatrixSelector) | ||
| aux.VectorSelector.Shard = i | ||
| aux.VectorSelector.NumShards = numShards | ||
| f := t.Clone().(*FunctionCall) | ||
| f.Args[marg] = aux | ||
| exprs[i] = f | ||
| } | ||
| *e = &Coalesce{Exprs: exprs} | ||
| } | ||
| return true | ||
| }) | ||
| return expr, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,6 +219,19 @@ func unmarshalNode(data []byte) (Node, error) { | |
| return nil, err | ||
| } | ||
| return u, nil | ||
| case CoalesceNode: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this cause remote execution to be planned in the central node? |
||
| n := &Coalesce{} | ||
| if err := json.Unmarshal(t.Data, n); err != nil { | ||
| return nil, err | ||
| } | ||
| for _, c := range t.Children { | ||
| child, err := unmarshalNode(c) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| n.Exprs = append(n.Exprs, child) | ||
| } | ||
| return n, nil | ||
| } | ||
| return nil, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,14 +25,16 @@ const ( | |
| NumberLiteralNode = "number_literal" | ||
| StringLiteralNode = "string_literal" | ||
| SubqueryNode = "subquery" | ||
| CheckDuplicateNode = "check_duplicate" | ||
| StepInvariantNode = "step_invariant" | ||
| ParensNode = "parens" | ||
| UnaryNode = "unary" | ||
|
|
||
| RemoteExecutionNode = "remote_exec" | ||
| DeduplicateNode = "dedup" | ||
| NoopNode = "noop" | ||
|
|
||
| CheckDuplicateNode = "check_duplicate" | ||
| CoalesceNode = "coalesce" | ||
| ) | ||
|
|
||
| type Cloneable interface { | ||
|
|
@@ -78,6 +80,9 @@ type VectorSelector struct { | |
| // CounterResetHint, Count and Sum values populated. Histogram buckets and spans | ||
| // will not be used during query evaluation. | ||
| DecodeNativeHistogramStats bool | ||
|
|
||
| Shard int | ||
| NumShards int | ||
| } | ||
|
|
||
| func (f *VectorSelector) Clone() Node { | ||
|
|
@@ -94,6 +99,11 @@ func (f *VectorSelector) Clone() Node { | |
| clone.Timestamp = &ts | ||
| } | ||
|
|
||
| clone.Shard = f.Shard | ||
| clone.NumShards = f.NumShards | ||
| clone.DecodeNativeHistogramStats = f.DecodeNativeHistogramStats | ||
| clone.BatchSize = f.BatchSize | ||
|
|
||
| return &clone | ||
| } | ||
|
|
||
|
|
@@ -570,3 +580,33 @@ func isAvgAggregation(expr *Node) bool { | |
| } | ||
| return false | ||
| } | ||
|
|
||
| type Coalesce struct { | ||
| // We assume to always have at least one expression | ||
| Exprs []Node `json:"-"` | ||
| } | ||
|
|
||
| func (c *Coalesce) ReturnType() parser.ValueType { return parser.ValueTypeVector } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should do Exprs[0].ReturnType()? |
||
|
|
||
| func (c *Coalesce) Type() NodeType { return CoalesceNode } | ||
|
|
||
| func (c *Coalesce) Clone() Node { | ||
| clone := *c | ||
| clone.Exprs = make([]Node, 0, len(c.Exprs)) | ||
| for _, arg := range c.Exprs { | ||
| clone.Exprs = append(clone.Exprs, arg.Clone()) | ||
| } | ||
| return &clone | ||
| } | ||
|
|
||
| func (c *Coalesce) Children() []*Node { | ||
| children := make([]*Node, 0, len(c.Exprs)) | ||
| for i := range c.Exprs { | ||
| children = append(children, &c.Exprs[i]) | ||
| } | ||
| return children | ||
| } | ||
|
|
||
| func (c *Coalesce) String() string { | ||
| return c.Exprs[0].String() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,10 @@ type Options struct { | |
| DecodingConcurrency int | ||
| } | ||
|
|
||
| func (o *Options) NumShards() int { | ||
| return max(o.DecodingConcurrency, 1) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am thinking this shard size could be different for different operators? I wonder if the default value works for all operators
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default is numCores / 2?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I meant if different operators should use the same concurrency |
||
|
|
||
| func (o *Options) NumSteps() int { | ||
| // Instant evaluation is executed as a range evaluation with one step. | ||
| if o.Step.Milliseconds() == 0 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,23 +58,17 @@ func (p Scanners) NewVectorSelector( | |
| selector = newHistogramStatsSelector(selector) | ||
| } | ||
|
|
||
| operators := make([]model.VectorOperator, 0, opts.DecodingConcurrency) | ||
| for i := range opts.DecodingConcurrency { | ||
| operator := exchange.NewConcurrent( | ||
| NewVectorSelector( | ||
| model.NewVectorPool(opts.StepsBatch), | ||
| selector, | ||
| opts, | ||
| logicalNode.Offset, | ||
| logicalNode.BatchSize, | ||
| logicalNode.SelectTimestamp, | ||
| i, | ||
| opts.DecodingConcurrency, | ||
| ), 2, opts) | ||
| operators = append(operators, operator) | ||
| } | ||
|
|
||
| return exchange.NewCoalesce(model.NewVectorPool(opts.StepsBatch), opts, logicalNode.BatchSize*int64(opts.DecodingConcurrency), operators...), nil | ||
| return exchange.NewConcurrent( | ||
| NewVectorSelector( | ||
| model.NewVectorPool(opts.StepsBatch), | ||
| selector, | ||
| opts, | ||
| logicalNode.Offset, | ||
| logicalNode.BatchSize, | ||
| logicalNode.SelectTimestamp, | ||
| logicalNode.Shard, | ||
| logicalNode.NumShards, | ||
| ), 2, opts), nil | ||
| } | ||
|
|
||
| func (p Scanners) NewMatrixSelector( | ||
|
|
@@ -126,28 +120,23 @@ func (p Scanners) NewMatrixSelector( | |
| selector = newHistogramStatsSelector(selector) | ||
| } | ||
|
|
||
| operators := make([]model.VectorOperator, 0, opts.DecodingConcurrency) | ||
| for i := range opts.DecodingConcurrency { | ||
| operator, err := NewMatrixSelector( | ||
| model.NewVectorPool(opts.StepsBatch), | ||
| selector, | ||
| call.Func.Name, | ||
| arg, | ||
| arg2, | ||
| opts, | ||
| logicalNode.Range, | ||
| vs.Offset, | ||
| vs.BatchSize, | ||
| i, | ||
| opts.DecodingConcurrency, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| operators = append(operators, exchange.NewConcurrent(operator, 2, opts)) | ||
| mat, err := NewMatrixSelector( | ||
| model.NewVectorPool(opts.StepsBatch), | ||
| selector, | ||
| call.Func.Name, | ||
| arg, | ||
| arg2, | ||
| opts, | ||
| logicalNode.Range, | ||
| vs.Offset, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could simplify this by passing in the entire |
||
| vs.BatchSize, | ||
| vs.Shard, | ||
| vs.NumShards, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return exchange.NewCoalesce(model.NewVectorPool(opts.StepsBatch), opts, vs.BatchSize*int64(opts.DecodingConcurrency), operators...), nil | ||
| return exchange.NewConcurrent(mat, 2, opts), nil | ||
| } | ||
|
|
||
| type histogramStatsSelector struct { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be in the logical plan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sector labels of the vector sector of the absent function? I think we could put it into the functioncall node and populate it in the plan yeah