From e9b6ce9de444093906676c975a797751ce869a4f Mon Sep 17 00:00:00 2001 From: Kaviraj Kanagaraj Date: Fri, 5 Apr 2024 19:23:02 +0200 Subject: [PATCH] fix(logql): Linefilter behaviour with chained `or` (#12463) Signed-off-by: Kaviraj --- pkg/logql/syntax/ast.go | 65 ++++++++++++++- pkg/logql/syntax/ast_test.go | 153 +++++++++++++++++++++++++++++++++++ 2 files changed, 217 insertions(+), 1 deletion(-) diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go index b0649570e833..0cf43ae71c7d 100644 --- a/pkg/logql/syntax/ast.go +++ b/pkg/logql/syntax/ast.go @@ -336,7 +336,18 @@ type LineFilter struct { type LineFilterExpr struct { LineFilter - Left *LineFilterExpr + Left *LineFilterExpr + // Or in LineFilterExpr works as follows. + // + // Case 1: With MatchEqual operators(|= or |~, etc) + // example: `{app="loki"} |= "test" |= "foo" or "bar"` + // expectation: match "test" AND (either "foo" OR "bar") + // + // Case 2: With NotMatchEqual operators (!= or !~, etc) + // example: `{app="loki"} != "test" != "foo" or "bar"` + // expectation: match !"test" AND !"foo" AND !"bar", Basically exactly as if `{app="loki"} != "test" != "foo" != "bar". + + // See LineFilterExpr tests for more examples. Or *LineFilterExpr IsOrChild bool implicit @@ -362,10 +373,62 @@ func newOrLineFilter(left, right *LineFilterExpr) *LineFilterExpr { } // !(left or right) == (!left and !right). + + // NOTE: Consider, we have chain of "or", != "foo" or "bar" or "baz" + // we parse from right to left, so first time left="bar", right="baz", and we don't know the actual `Ty` (equal: |=, notequal: !=, regex: |~, etc). So + // it will have default (0, LineMatchEqual). + // we only know real `Ty` in next stage, where left="foo", right="bar or baz", at this time, `Ty` is LineMatchNotEqual(!=). + // Now we need to update not just `right.Ty = left.Ty`, we also have to update all the `right.Or` until `right.Or` is nil. + tmp := right + for tmp.Or != nil { + tmp.Or.Ty = left.Ty + tmp = tmp.Or + } + return newNestedLineFilterExpr(left, right) } func newNestedLineFilterExpr(left *LineFilterExpr, right *LineFilterExpr) *LineFilterExpr { + // NOTE: When parsing "or" chains in linefilter, particularly variations of NOT filters (!= or !~), we need to transform + // say (!= "foo" or "bar "baz") => (!="foo" != "bar" != "baz") + if right.Or != nil && !(right.Ty == log.LineMatchEqual || right.Ty == log.LineMatchRegexp || right.Ty == log.LineMatchPattern) { + right.Or.IsOrChild = false + tmp := right.Or + right.Or = nil + right = newNestedLineFilterExpr(right, tmp) + } + + // NOTE: Before supporting `or` in linefilter, `right` will always be a leaf node (right.next == nil) + // After supporting `or` in linefilter, `right` may not be a leaf node (e.g: |= "a" or "b). Here "b".Left = "a") + // We traverse the tree recursively untile we make `right` leaf node. + // Consider the following expression. {app="loki"} != "test" != "foo" or "bar or "car"" + // It first creates following tree on the left and transformed into the one on the right. + // ┌────────────┐ + // ┌────────────┐ │ root │ + // │ root │ └──────┬─────┘ + // └──────┬─────┘ │ + // │ │ + // │ │ + // │ ─────────► ┌────────────────┴─────────────┐ + // ┌────────────────┴─────────────┐ │ │ + // │ │ ┌───┴────┐ │ + // │ │ │ foo │ ┌──┴────┐ + // │ ┌─┴────┐ └───┬────┘ │ bar │ + // ┌───┴────┐ │ bar │ │ └───────┘ + // │ test │ └──┬───┘ │ + // └────────┘ │ │ + // │ ┌────────┘ + // │ │ + // ┌─────────┘ │ + // │ │ + // │ ┌───┴───┐ + // │ │ test │ + // ┌─┴────┐ └───────┘ + // │ foo │ + // └──────┘ + if right.Left != nil { + left = newNestedLineFilterExpr(left, right.Left) + } return &LineFilterExpr{ Left: left, LineFilter: right.LineFilter, diff --git a/pkg/logql/syntax/ast_test.go b/pkg/logql/syntax/ast_test.go index 2ba435e0fe2d..ec458849331e 100644 --- a/pkg/logql/syntax/ast_test.go +++ b/pkg/logql/syntax/ast_test.go @@ -401,6 +401,34 @@ func Test_FilterMatcher(t *testing.T) { }, []linecheck{{"foo", true}, {"bar", true}, {"none", false}}, }, + { + `{app="foo"} |= "test" |= "foo" or "bar"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test foo", true}, {"test bar", true}, {"none", false}}, + }, + { + `{app="foo"} |= "test" |= "foo" or "bar" or "baz"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test foo", true}, {"test bar", true}, {"test baz", true}, {"baz", false}, {"bar", false}, {"foo", false}, {"none", false}}, + }, + { + `{app="foo"} |= "test" |= "foo" or "bar" or "baz" |= "car"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"car test foo", true}, {"car test bar", true}, {"car test baz", true}, {"baz", false}, {"bar", false}, {"test", false}, {"foo", false}, {"car", false}, {"none", false}}, + }, + { + `{app="foo"} |= "test" |= "foo" or "bar" or "baz"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test foo", true}, {"test bar", true}, {"test baz", true}, {"none", false}}, + }, { `{app="foo"} |= "foo" or "bar" |= "buzz" or "fizz"`, []*labels.Matcher{ @@ -408,6 +436,34 @@ func Test_FilterMatcher(t *testing.T) { }, []linecheck{{"foo buzz", true}, {"bar fizz", true}, {"foo", false}, {"bar", false}, {"none", false}}, }, + { + `{app="foo"} |~ "foo" or "bar"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"foo", true}, {"bar", true}, {"none", false}}, + }, + { + `{app="foo"} |~ "test" |~ "foo" or "bar"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test foo", true}, {"test bar", true}, {"none", false}}, + }, + { + `{app="foo"} |~ "test" |~ "foo" or "bar" or "baz"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test foo", true}, {"test bar", true}, {"test baz", true}, {"baz", false}, {"bar", false}, {"foo", false}, {"none", false}}, + }, + { + `{app="foo"} |~ "test" |~ "foo" or "bar" or "baz" |~ "car"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"car test foo", true}, {"car test bar", true}, {"car test baz", true}, {"baz", false}, {"bar", false}, {"test", false}, {"foo", false}, {"car", false}, {"none", false}}, + }, { `{app="foo"} != "foo" or "bar"`, []*labels.Matcher{ @@ -415,6 +471,28 @@ func Test_FilterMatcher(t *testing.T) { }, []linecheck{{"foo", false}, {"bar", false}, {"none", true}}, }, + { + `{app="foo"} != "test" != "foo" or "bar"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test", false}, {"foo", false}, {"bar", false}, {"none", true}}, + }, + { + `{app="foo"} != "test" != "foo" or "bar" or "baz"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test", false}, {"foo", false}, {"bar", false}, {"baz", false}, {"none", true}}, + }, + { + `{app="foo"} != "test" != "foo" or "bar" or "baz" != "car"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test", false}, {"foo", false}, {"bar", false}, {"baz", false}, {"car", false}, {"none", true}}, + }, + { `{app="foo"} |~ "foo" or "bar"`, []*labels.Matcher{ @@ -429,6 +507,27 @@ func Test_FilterMatcher(t *testing.T) { }, []linecheck{{"foo", false}, {"bar", false}, {"none", true}}, }, + { + `{app="foo"} !~ "test" !~ "foo" or "bar"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test", false}, {"foo", false}, {"bar", false}, {"none", true}}, + }, + { + `{app="foo"} !~ "test" !~ "foo" or "bar" or "baz"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test", false}, {"foo", false}, {"bar", false}, {"baz", false}, {"none", true}}, + }, + { + `{app="foo"} !~ "test" !~ "foo" or "bar" or "baz" !~ "car"`, + []*labels.Matcher{ + mustNewMatcher(labels.MatchEqual, "app", "foo"), + }, + []linecheck{{"test", false}, {"foo", false}, {"bar", false}, {"baz", false}, {"car", false}, {"none", true}}, + }, { `{app="foo"} |= ip("127.0.0.1") or "foo"`, []*labels.Matcher{ @@ -533,6 +632,18 @@ func TestStringer(t *testing.T) { in: `{app="foo"} |= "foo" or "bar"`, out: `{app="foo"} |= "foo" or "bar"`, }, + { + in: `{app="foo"} |= "foo" or "bar" or "baz"`, + out: `{app="foo"} |= "foo" or "bar" or "baz"`, + }, + { + in: `{app="foo"} |= "foo" or "bar" or "baz" |= "car"`, + out: `{app="foo"} |= "foo" or "bar" or "baz" |= "car"`, + }, + { + in: `{app="foo"} |= "foo" or "bar" or "baz" |= "car" |= "a" or "b" or "c"`, + out: `{app="foo"} |= "foo" or "bar" or "baz" |= "car" |= "a" or "b" or "c"`, + }, { in: `{app="foo"} |~ "foo" or "bar" or "baz"`, out: `{app="foo"} |~ "foo" or "bar" or "baz"`, @@ -569,10 +680,52 @@ func TestStringer(t *testing.T) { in: `{app="foo"} != "foo" or "bar"`, out: `{app="foo"} != "foo" != "bar"`, }, + { + in: `{app="foo"} != "test" != "foo" or "bar"`, + out: `{app="foo"} != "test" != "foo" != "bar"`, + }, + { + in: `{app="foo"} != "test" != "foo" or "bar" or "baz"`, + out: `{app="foo"} != "test" != "foo" != "bar" != "baz"`, + }, + { + in: `{app="foo"} != "foo" or "bar" or "baz" != "car"`, + out: `{app="foo"} != "foo" != "bar" != "baz" != "car"`, + }, + { + in: `{app="foo"} != "foo" or "bar" or "baz" != "car" != "a" or "b" or "c"`, + out: `{app="foo"} != "foo" != "bar" != "baz" != "car" != "a" != "b" != "c"`, + }, + { + // Mix of != and |= + in: `{app="foo"} |= "foo" or "bar" or "baz" != "car" != "a" or "b" or "c"`, + out: `{app="foo"} |= "foo" or "bar" or "baz" != "car" != "a" != "b" != "c"`, + }, { in: `{app="foo"} !~ "foo" or "bar"`, out: `{app="foo"} !~ "foo" !~ "bar"`, }, + { + in: `{app="foo"} !~ "test" !~ "foo" or "bar"`, + out: `{app="foo"} !~ "test" !~ "foo" !~ "bar"`, + }, + { + in: `{app="foo"} !~ "test" !~ "foo" or "bar" or "baz"`, + out: `{app="foo"} !~ "test" !~ "foo" !~ "bar" !~ "baz"`, + }, + { + in: `{app="foo"} !~ "foo" or "bar" or "baz" !~ "car"`, + out: `{app="foo"} !~ "foo" !~ "bar" !~ "baz" !~ "car"`, + }, + { + in: `{app="foo"} !~ "foo" or "bar" or "baz" !~ "car" !~ "a" or "b" or "c"`, + out: `{app="foo"} !~ "foo" !~ "bar" !~ "baz" !~ "car" !~ "a" !~ "b" !~ "c"`, + }, + { + // Mix of !~ and |~ + in: `{app="foo"} |~ "foo" or "bar" or "baz" !~ "car" !~ "a" or "b" or "c"`, + out: `{app="foo"} |~ "foo" or "bar" or "baz" !~ "car" !~ "a" !~ "b" !~ "c"`, + }, { in: `{app="foo"} != ip("127.0.0.1") or "foo"`, out: `{app="foo"} != ip("127.0.0.1") != "foo"`,