[query] Fix bug with parsing Graphite find query #1676
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1676 +/- ##
========================================
- Coverage 71.8% 71.8% -0.1%
========================================
Files 968 968
Lines 81169 81177 +8
========================================
- Hits 58345 58339 -6
- Misses 18990 19002 +12
- Partials 3834 3836 +2
Continue to review full report at Codecov.
|
|
Ah nice, I like the approach of using explicit match types too. Good stuff! |
| @@ -38,6 +38,9 @@ enum MatcherType { | |||
| // matcher name rather than value | |||
| EXISTS = 4; | |||
| NOTEXISTS = 5; | |||
| // ALL superceeds other matcher types | |||
| // and does no filtering | |||
| @@ -45,6 +45,20 @@ func TestConvertMetricPartToMatcher(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestConverWildcardToMatcher(t *testing.T) { | |||
There was a problem hiding this comment.
nit: spelling: Convert
| @@ -45,6 +45,20 @@ func TestConvertMetricPartToMatcher(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestConverWildcardToMatcher(t *testing.T) { | |||
| metric := "*" | |||
| for i := 0; i < 100; i++ { | |||
There was a problem hiding this comment.
This is kind of a weird test -- why not just run this once?
There was a problem hiding this comment.
Just to ensure that the metric name is also converted to expected (i.e. __g100__) as well
| @@ -38,6 +38,10 @@ func (m MatchType) String() string { | |||
| return "=~" | |||
| case MatchNotRegexp: | |||
| return "!~" | |||
| case MatchField: | |||
| return "-" | |||
There was a problem hiding this comment.
For my understanding, how did you come up with these mappings?
There was a problem hiding this comment.
Made 'em up :p
It's not really a "standard" matcher type, and the only thing this affects is the String() method on MatchTypes so we should be fine here
| @@ -192,6 +192,17 @@ func matcherToQuery(matcher models.Matcher) (idx.Query, error) { | |||
| } | |||
| return query, nil | |||
|
|
|||
| case models.MatchNotField: | |||
| negate = true | |||
| fallthrough | |||
There was a problem hiding this comment.
Nice - good use of fallthrough.
What this PR does / why we need it:
This fixes an issue with parsing graphite find queries. Previously,
foo.*would get split up intofoo, and*, which would go through a graphite globber and become[^\.]*, which would then not get correctly identified as aNewFieldQueryin the storage/index (this would convert regex values looking for.*only to aNewFieldQuery.This PR adds
MatchFieldandMatchNotFieldmatch types, and explicitly makes use of them in graphite queries, avoiding relying on implicit conversion in the storage layer.Does this PR introduce a user-facing and/or backwards incompatible change?: