fix(blooms): Not filters should always match (grafana#12358)

inloco · Apr 12, 2024 · 428e87e · 428e87e
1 parent 567fe89
commit 428e87e
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 26 deletions.
diff --git a/pkg/storage/bloom/v1/bloom_tester.go b/pkg/storage/bloom/v1/bloom_tester.go
@@ -90,13 +90,16 @@ func FiltersToBloomTest(b NGramBuilder, filters ...syntax.LineFilterExpr) BloomT
 
 func simpleFilterToBloomTest(b NGramBuilder, filter syntax.LineFilter) BloomTest {
 	switch filter.Ty {
-	case labels.MatchEqual, labels.MatchNotEqual:
-		var test BloomTest = newStringTest(b, filter.Match)
-		if filter.Ty == labels.MatchNotEqual {
-			test = newNotTest(test)
-		}
-		return test
-	case labels.MatchRegexp, labels.MatchNotRegexp:
+	case labels.MatchNotEqual, labels.MatchNotRegexp:
+		// We cannot test _negated_ filters with a bloom filter since blooms are probabilistic
+		// filters that can only tell us if a string _might_ exist.
+		// For example, for `!= "foo"`, the bloom filter might tell us that the string "foo" might exist
+		// but because we are not sure, we cannot discard that chunk because it might actually not be there.
+		// Therefore, we return a test that always returns true.
+		return MatchAll
+	case labels.MatchEqual:
+		return newStringTest(b, filter.Match)
+	case labels.MatchRegexp:
 		reg, err := regexpsyntax.Parse(filter.Match, regexpsyntax.Perl)
 		if err != nil {
 			// TODO: log error
@@ -111,11 +114,7 @@ func simpleFilterToBloomTest(b NGramBuilder, filter syntax.LineFilter) BloomTest
 			return MatchAll
 		}
 
-		var test BloomTest = matcherFilterWrapper{filter: matcher}
-		if filter.Ty == labels.MatchNotRegexp {
-			test = newNotTest(test)
-		}
-		return test
+		return matcherFilterWrapper{filter: matcher}
 	default:
 		return MatchAll
 	}

diff --git a/pkg/storage/bloom/v1/bloom_tester_test.go b/pkg/storage/bloom/v1/bloom_tester_test.go
@@ -47,16 +47,16 @@ func TestFiltersToBloomTests(t *testing.T) {
 			expectMatch: false,
 		},
 		{
-			name:        "notEq match",
+			name:        "notEq doesnt exist",
 			query:       `{app="fake"} != "nope"`,
 			bloom:       fakeBloom{"foo", "bar"},
 			expectMatch: true,
 		},
 		{
-			name:        "notEq no match",
+			name:        "notEq exists",
 			query:       `{app="fake"} != "foo"`,
 			bloom:       fakeBloom{"foo", "bar"},
-			expectMatch: false,
+			expectMatch: true, // Still should match because it's NotEQ
 		},
 		{
 			name:        "or filter both match",
@@ -89,22 +89,22 @@ func TestFiltersToBloomTests(t *testing.T) {
 			expectMatch: true,
 		},
 		{
-			name:        "Not or filter right no match",
+			name:        "NotEq OR filter right exists",
 			query:       `{app="fake"} != "nope" or "bar"`,
 			bloom:       fakeBloom{"foo", "bar"},
-			expectMatch: false,
+			expectMatch: true, // Still should match because it's NotEQ
 		},
 		{
-			name:        "Not or filter left no match",
+			name:        "Not OR filter left exists",
 			query:       `{app="fake"} != "foo" or "nope"`,
 			bloom:       fakeBloom{"foo", "bar"},
-			expectMatch: false,
+			expectMatch: true, // Still should match because it's NotEQ
 		},
 		{
-			name:        "Not or filter no match",
+			name:        "NotEq OR filter both exists",
 			query:       `{app="fake"} != "foo" or "bar"`,
 			bloom:       fakeBloom{"foo", "bar"},
-			expectMatch: false,
+			expectMatch: true, // Still should match because it's NotEQ
 		},
 		{
 			name:        "complex filter match",
@@ -125,10 +125,10 @@ func TestFiltersToBloomTests(t *testing.T) {
 			expectMatch: true,
 		},
 		{
-			name:        "regex match none",
+			name:        "regex match all notEq",
 			query:       `{app="fake"} !~ ".*"`,
 			bloom:       fakeBloom{"foo", "bar"},
-			expectMatch: false,
+			expectMatch: true, // Still should match,
 		},
 		{
 			name:        "regex match",
@@ -138,21 +138,27 @@ func TestFiltersToBloomTests(t *testing.T) {
 		},
 		{
 			name:        "regex no match",
-			query:       `{app="fake"} !~ "nope|.*foo.*"`,
+			query:       `{app="fake"} |~ ".*not.*"`,
 			bloom:       fakeBloom{"foo", "bar"},
 			expectMatch: false,
 		},
+		{
+			name:        "regex notEq right exists",
+			query:       `{app="fake"} !~ "nope|.*foo.*"`,
+			bloom:       fakeBloom{"foo", "bar"},
+			expectMatch: true, // Still should match because it's NotEQ
+		},
 		{
 			name:        "complex regex match",
 			query:       `{app="fake"} |~ "(nope|.*not.*|.*foo.*)" or "(no|ba)" !~ "noz.*" or "(nope|not)"`,
 			bloom:       fakeBloom{"foo", "bar", "baz", "fuzz"},
 			expectMatch: true,
 		},
 		{
-			name:        "complex regex no match",
+			name:        "complex regex with notEq exists",
 			query:       `{app="fake"} |~ "(nope|.*not.*|.*foo.*)" or "(no|ba)" !~ "noz.*"`,
 			bloom:       fakeBloom{"foo", "bar", "baz", "fuzz", "noz"},
-			expectMatch: false,
+			expectMatch: true, // Still should match because it's NotEQ
 		},
 		{
 			name:        "line filter after line format",