Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions pkg/storage/bloom/v1/bloom_tester.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down
34 changes: 20 additions & 14 deletions pkg/storage/bloom/v1/bloom_tester_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
Loading