Skip to content

Commit

Permalink
[query] Fix bug with parsing Graphite find query (#1676)
Browse files Browse the repository at this point in the history
  • Loading branch information
arnikola authored May 30, 2019
1 parent c982091 commit 744e0fd
Show file tree
Hide file tree
Showing 13 changed files with 157 additions and 165 deletions.
4 changes: 2 additions & 2 deletions src/query/api/v1/handler/graphite/find_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,9 @@ func parseFindParamsToQueries(r *http.Request) (

clonedMatchers := make([]models.Matcher, len(matchers))
copy(clonedMatchers, matchers)
// NB: change terminator from `MatchNotRegexp` to `MatchRegexp` to ensure
// NB: change terminator from `MatchNotField` to `MatchField` to ensure
// segments with children are matched.
clonedMatchers[len(clonedMatchers)-1].Type = models.MatchRegexp
clonedMatchers[len(clonedMatchers)-1].Type = models.MatchField
childQuery := &storage.CompleteTagsQuery{
CompleteNameOnly: false,
FilterNameTags: filter,
Expand Down
4 changes: 2 additions & 2 deletions src/query/api/v1/handler/graphite/find_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func setupStorage(ctrl *gomock.Controller) storage.Storage {
matchers: []models.Matcher{
{Type: models.MatchEqual, Name: b("__g0__"), Value: b("foo")},
{Type: models.MatchRegexp, Name: b("__g1__"), Value: b(`b[^\.]*`)},
{Type: models.MatchNotRegexp, Name: b("__g2__"), Value: b(".*")},
{Type: models.MatchNotField, Name: b("__g2__")},
},
}

Expand All @@ -146,7 +146,7 @@ func setupStorage(ctrl *gomock.Controller) storage.Storage {
matchers: []models.Matcher{
{Type: models.MatchEqual, Name: b("__g0__"), Value: b("foo")},
{Type: models.MatchRegexp, Name: b("__g1__"), Value: b(`b[^\.]*`)},
{Type: models.MatchRegexp, Name: b("__g2__"), Value: b(".*")},
{Type: models.MatchField, Name: b("__g2__")},
},
}

Expand Down
146 changes: 76 additions & 70 deletions src/query/generated/proto/rpcpb/query.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/query/generated/proto/rpcpb/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ enum MatcherType {
REGEXP = 2;
NOTREGEXP = 3;
// EXISTS and NOTEXISTS apply only to
// matcher name rather than value
// matcher name rather than value.
EXISTS = 4;
NOTEXISTS = 5;
// ALL supercedes other matcher types
// and does no filtering.
ALL = 6;
}

message FetchResponse {
Expand Down
16 changes: 11 additions & 5 deletions src/query/graphite/storage/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,22 @@ import (
"github.com/m3db/m3/src/query/models"
)

var (
wildcard = []byte(".*")
const (
wildcard = "*"
)

func convertMetricPartToMatcher(
count int,
metric string,
) (models.Matcher, error) {
var matchType models.MatchType
if metric == wildcard {
return models.Matcher{
Type: models.MatchField,
Name: graphite.TagName(count),
}, nil
}

value, isRegex, err := graphite.GlobToRegexPattern(metric)
if err != nil {
return models.Matcher{}, err
Expand All @@ -54,8 +61,7 @@ func convertMetricPartToMatcher(

func matcherTerminator(count int) models.Matcher {
return models.Matcher{
Type: models.MatchNotRegexp,
Name: graphite.TagName(count),
Value: wildcard,
Type: models.MatchNotField,
Name: graphite.TagName(count),
}
}
19 changes: 16 additions & 3 deletions src/query/graphite/storage/converter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,20 @@ func TestConvertMetricPartToMatcher(t *testing.T) {
}
}

func TestConvertWildcardToMatcher(t *testing.T) {
metric := "*"
for i := 0; i < 100; i++ {
expected := models.Matcher{
Type: models.MatchField,
Name: graphite.TagName(i),
}

actual, err := convertMetricPartToMatcher(i, metric)
require.NoError(t, err)
assert.Equal(t, expected, actual)
}
}

func TestConvertAlphanumericMetricPartToMatcher(t *testing.T) {
metric := "abcdefg"
expected := models.Matcher{
Expand All @@ -61,9 +75,8 @@ func TestConvertAlphanumericMetricPartToMatcher(t *testing.T) {
func TestMatcherTerminator(t *testing.T) {
for i := 0; i < 100; i++ {
expected := models.Matcher{
Type: models.MatchNotRegexp,
Name: graphite.TagName(i),
Value: []byte(".*"),
Type: models.MatchNotField,
Name: graphite.TagName(i),
}

actual := matcherTerminator(i)
Expand Down
4 changes: 2 additions & 2 deletions src/query/graphite/storage/m3_wrapper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ func TestTranslateQuery(t *testing.T) {
{Type: models.MatchEqual, Name: graphite.TagName(3), Value: []byte("terminator")},
{Type: models.MatchEqual, Name: graphite.TagName(4), Value: []byte("will")},
{Type: models.MatchEqual, Name: graphite.TagName(5), Value: []byte("be")},
{Type: models.MatchRegexp, Name: graphite.TagName(6), Value: []byte(`[^\.]*`)},
{Type: models.MatchField, Name: graphite.TagName(6)},
{Type: models.MatchRegexp, Name: graphite.TagName(7), Value: []byte(`back[^\.]`)},
{Type: models.MatchNotRegexp, Name: graphite.TagName(8), Value: []byte(".*")},
{Type: models.MatchNotField, Name: graphite.TagName(8)},
}

assert.Equal(t, expected, matchers)
Expand Down
20 changes: 4 additions & 16 deletions src/query/models/matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ func (m MatchType) String() string {
return "=~"
case MatchNotRegexp:
return "!~"
case MatchField:
return "-"
case MatchNotField:
return "!-"
case MatchAll:
return "*"
default:
Expand Down Expand Up @@ -69,22 +73,6 @@ func (m Matcher) String() string {
return fmt.Sprintf("%s%s%q", m.Name, m.Type, m.Value)
}

// Matches returns whether the matcher matches the given string value.
func (m Matcher) Matches(s []byte) bool {
switch m.Type {
case MatchEqual:
return bytes.Equal(s, m.Value)
case MatchNotEqual:
return !bytes.Equal(s, m.Value)
case MatchRegexp:
return m.re.MatchString(string(s))
case MatchNotRegexp:
return !m.re.MatchString(string(s))
}

panic("labels.Matcher.Matches: invalid match type")
}

// ToTags converts Matchers to Tags
// NB (braskin): this only works for exact matches
func (m Matchers) ToTags(
Expand Down
Loading

0 comments on commit 744e0fd

Please sign in to comment.