From 0fea48742f6850e88cdaa84b6cc971c68a0110ef Mon Sep 17 00:00:00 2001 From: Thomas Jackson Date: Fri, 3 Sep 2021 10:41:08 -0700 Subject: [PATCH] Don't allow NodeReplacer on a node that has multiple selectors 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 --- pkg/proxystorage/proxy.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/proxystorage/proxy.go b/pkg/proxystorage/proxy.go index 097ffd6f2..e53c5e688 100644 --- a/pkg/proxystorage/proxy.go +++ b/pkg/proxystorage/proxy.go @@ -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) { @@ -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 @@ -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