Skip to content

Commit

Permalink
Don't allow NodeReplacer on a node that has multiple selectors
Browse files Browse the repository at this point in the history
At this level we have no way to know if those selectors will be on the
same node (combining them into a single downstream query -- e.g.
aggregation imposes that limitation). So instead we simply disallow
NodeReplace for anything with multiple selectors as children.

Fixes #456
  • Loading branch information
jacksontj committed Sep 3, 2021
1 parent 2332c39 commit 0fea487
Showing 1 changed file with 14 additions and 1 deletion.
15 changes: 14 additions & 1 deletion pkg/proxystorage/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,11 @@ func (p *ProxyStorage) NodeReplacer(ctx context.Context, s *parser.EvalStmt, nod
return ok
}

isVectorSelector := func(node parser.Node) bool {
_, ok := node.(*parser.VectorSelector)
return ok
}

// If we are a child of a subquery; we just skip replacement (since it already did a nodereplacer for those)
for _, n := range path {
if isSubQuery(n) {
Expand All @@ -219,8 +224,9 @@ func (p *ProxyStorage) NodeReplacer(ctx context.Context, s *parser.EvalStmt, nod
// rules around combining). We'll skip this node and let a lower layer take this on
aggFinder := &BooleanFinder{Func: isAgg}
offsetFinder := &OffsetFinder{}
vecFinder := &BooleanFinder{Func: isVectorSelector}

visitor := NewMultiVisitor([]parser.Visitor{aggFinder, offsetFinder})
visitor := NewMultiVisitor([]parser.Visitor{aggFinder, offsetFinder, vecFinder})

if _, err := parser.Walk(ctx, visitor, s, node, nil, nil); err != nil {
return nil, err
Expand All @@ -233,6 +239,13 @@ func (p *ProxyStorage) NodeReplacer(ctx context.Context, s *parser.EvalStmt, nod
}
}

// If there is more than 1 vector selector here and we are not a subquery
// we can't combine as we don't know if those 2 selectors will for-sure
// land on the same node
if vecFinder.Found > 1 && !isSubQuery(node) {
return nil, nil
}

// If the tree below us is not all the same offset, then we can't do anything below -- we'll need
// to wait until further in execution where they all match
var offset time.Duration
Expand Down

0 comments on commit 0fea487

Please sign in to comment.