Skip to content

Commit

Permalink
fix(logql): Linefilter behaviour with chained or (#12463)
Browse files Browse the repository at this point in the history 
Signed-off-by: Kaviraj <[email protected]>
  • Loading branch information
@kavirajk
kavirajk authored Apr 5, 2024
1 parent 57e91cd commit e9b6ce9
Show file tree
Hide file tree
Showing 2 changed files with 217 additions and 1 deletion.
65 changes: 64 additions & 1 deletion pkg/logql/syntax/ast.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
153 changes: 153 additions & 0 deletions pkg/logql/syntax/ast_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,20 +401,98 @@ 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{
mustNewMatcher(labels.MatchEqual, "app", "foo"),
},
[]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{
mustNewMatcher(labels.MatchEqual, "app", "foo"),
},
[]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{
Expand All @@ -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{
Expand Down Expand Up @@ -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"`,
Expand Down Expand Up @@ -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"`,
Expand Down

0 comments on commit e9b6ce9

Please sign in to comment.