From d3b6c1b010b5c36ab77105988bc09a0f9764cf59 Mon Sep 17 00:00:00 2001 From: John Shahid Date: Wed, 22 Oct 2014 15:22:59 -0400 Subject: [PATCH 1/6] FromClause.GetString() shouldn't assume 2 series in the merge Fix #1047 --- integration/data_test.go | 22 ++++++++++++++++++++++ parser/from_clause.go | 10 ++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/integration/data_test.go b/integration/data_test.go index cdce02a9465..c99f22ae0dc 100644 --- a/integration/data_test.go +++ b/integration/data_test.go @@ -267,6 +267,28 @@ func (self *DataTestSuite) TestModeWithNils(c *C) { c.Assert(maps[0]["m2"], Equals, nil) } +func (self *DataTestSuite) TestMergeRegexOneSeries(c *C) { + data := ` +[ + { + "name": "test_merge_1", + "columns": ["time", "value"], + "points": [ + [1401321700000, "m11"], + [1401321600000, "m12"] + ] + } +]` + + self.client.WriteJsonData(data, c, influxdb.Millisecond) + serieses := self.client.RunQuery("select * from merge(/.*/)", c, "m") + c.Assert(serieses, HasLen, 1) + maps := ToMap(serieses[0]) + c.Assert(maps, HasLen, 2) + c.Assert(maps[0]["value"], Equals, "m11") + c.Assert(maps[1]["value"], Equals, "m12") +} + func (self *DataTestSuite) TestMergeRegex(c *C) { data := ` [ diff --git a/parser/from_clause.go b/parser/from_clause.go index c058abf7cbc..5dfa850792f 100644 --- a/parser/from_clause.go +++ b/parser/from_clause.go @@ -48,8 +48,14 @@ func (self *FromClause) GetString() string { buffer := bytes.NewBufferString("") switch self.Type { case FromClauseMerge: - fmt.Fprintf(buffer, "%s%s merge %s %s", self.Names[0].Name.GetString(), self.Names[1].GetAliasString(), - self.Names[1].Name.GetString(), self.Names[1].GetAliasString()) + fmt.Fprintf(buffer, "merge(") + for i, n := range self.Names { + if i > 0 { + buffer.WriteRune('|') + } + buffer.WriteString(n.Name.GetString()) + } + buffer.WriteRune(')') case FromClauseInnerJoin: fmt.Fprintf(buffer, "%s%s inner join %s%s", self.Names[0].Name.GetString(), self.Names[0].GetAliasString(), self.Names[1].Name.GetString(), self.Names[1].GetAliasString()) From cace54d8c805a1980dc6cf8e1cb25d3d35f305f5 Mon Sep 17 00:00:00 2001 From: John Shahid Date: Wed, 22 Oct 2014 16:35:51 -0400 Subject: [PATCH 2/6] Make sure the query string has valid regex and add a test --- coordinator/coordinator.go | 11 ++++------- parser/from_clause.go | 4 ++-- parser/parser_test.go | 19 +++++++++++++++++++ parser/rewrite.go | 21 +++++++++++++++++++++ 4 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 parser/rewrite.go diff --git a/coordinator/coordinator.go b/coordinator/coordinator.go index 5b6f77e3b23..e6a955274b9 100644 --- a/coordinator/coordinator.go +++ b/coordinator/coordinator.go @@ -279,14 +279,11 @@ func (self *Coordinator) expandRegex(spec *parser.QuerySpec) { } if f := q.FromClause; f.Type == parser.FromClauseMergeFun { - series := self.clusterConfiguration.MetaStore.GetSeriesForDatabaseAndRegex(spec.Database(), q.FromClause.Regex) - f.Type = parser.FromClauseMerge - f.Regex = nil - for _, s := range series { - f.Names = append(f.Names, &parser.TableName{ - Name: &parser.Value{Name: s, Type: parser.ValueTableName}, - }) + f := func(r *regexp.Regexp) []string { + return self.clusterConfiguration.MetaStore.GetSeriesForDatabaseAndRegex(spec.Database(), r) } + + parser.RewriteMergeQuery(q, f) } } diff --git a/parser/from_clause.go b/parser/from_clause.go index 5dfa850792f..5429baa2730 100644 --- a/parser/from_clause.go +++ b/parser/from_clause.go @@ -48,14 +48,14 @@ func (self *FromClause) GetString() string { buffer := bytes.NewBufferString("") switch self.Type { case FromClauseMerge: - fmt.Fprintf(buffer, "merge(") + fmt.Fprintf(buffer, "merge(/") for i, n := range self.Names { if i > 0 { buffer.WriteRune('|') } buffer.WriteString(n.Name.GetString()) } - buffer.WriteRune(')') + buffer.WriteString("/)") case FromClauseInnerJoin: fmt.Fprintf(buffer, "%s%s inner join %s%s", self.Names[0].Name.GetString(), self.Names[0].GetAliasString(), self.Names[1].Name.GetString(), self.Names[1].GetAliasString()) diff --git a/parser/parser_test.go b/parser/parser_test.go index adf72016b5c..a9604ab3f2f 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -32,6 +32,25 @@ func (self *QueryParserSuite) TestInvalidFromClause(c *C) { c.Assert(err, ErrorMatches, ".*\\$undefined.*") } +func (self *QueryParserSuite) TestParseMergeGetString(c *C) { + f := func(r *regexp.Regexp) []string { + return []string{"foobar"} + } + + query := "select * from merge(/.*foo.*/)" + qs, err := ParseQuery(query) + c.Assert(err, IsNil) + c.Assert(qs, HasLen, 1) + RewriteMergeQuery(qs[0].SelectQuery, f) + fmt.Printf("Parsing %s\n", qs[0].GetQueryStringWithTimeCondition()) + actualQs, err := ParseQuery(qs[0].GetQueryStringWithTimeCondition()) + c.Assert(err, IsNil) + c.Assert(actualQs, HasLen, 1) + RewriteMergeQuery(actualQs[0].SelectQuery, f) + actualQs[0].SelectQuery.startTimeSpecified = false + c.Assert(actualQs[0].SelectQuery, DeepEquals, qs[0].SelectQuery) +} + func (self *QueryParserSuite) TestInvalidExplainQueries(c *C) { query := "explain select foo, baz group by time(1d)" _, err := ParseQuery(query) diff --git a/parser/rewrite.go b/parser/rewrite.go new file mode 100644 index 00000000000..6fac2bff8f8 --- /dev/null +++ b/parser/rewrite.go @@ -0,0 +1,21 @@ +package parser + +import "regexp" + +type RegexMatcher func(r *regexp.Regexp) []string + +func RewriteMergeQuery(query *SelectQuery, rm RegexMatcher) { + if query.FromClause.Type != FromClauseMergeFun { + return + } + + series := rm(query.FromClause.Regex) + f := query.FromClause + f.Type = FromClauseMerge + f.Regex = nil + for _, s := range series { + f.Names = append(f.Names, &TableName{ + Name: &Value{Name: s, Type: ValueTableName}, + }) + } +} From 4fbe259722e7acd05a76017465034345244e55a3 Mon Sep 17 00:00:00 2001 From: John Shahid Date: Wed, 22 Oct 2014 16:44:01 -0400 Subject: [PATCH 3/6] add some docs --- parser/parser_test.go | 2 ++ parser/rewrite.go | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/parser/parser_test.go b/parser/parser_test.go index a9604ab3f2f..92e97817706 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -32,6 +32,8 @@ func (self *QueryParserSuite) TestInvalidFromClause(c *C) { c.Assert(err, ErrorMatches, ".*\\$undefined.*") } +// Make sure that GetQueryStringWithTimeCondition() works for regex +// merge queries. func (self *QueryParserSuite) TestParseMergeGetString(c *C) { f := func(r *regexp.Regexp) []string { return []string{"foobar"} diff --git a/parser/rewrite.go b/parser/rewrite.go index 6fac2bff8f8..047f70ee458 100644 --- a/parser/rewrite.go +++ b/parser/rewrite.go @@ -4,6 +4,13 @@ import "regexp" type RegexMatcher func(r *regexp.Regexp) []string +// Given a function that returns the series names that match the given +// regex, this function will rewrite the query such that the matching +// serires names are included in the query. E.g. if we have series +// names foobar, foobaz and barbaz and a query +// select * from merge(/foo.*/) +// the query will be rewritten to +// select * from merge(foobar, foobaz) func RewriteMergeQuery(query *SelectQuery, rm RegexMatcher) { if query.FromClause.Type != FromClauseMergeFun { return From 6802eda76809dbbfc998128f3dac25d8400084fd Mon Sep 17 00:00:00 2001 From: John Shahid Date: Wed, 22 Oct 2014 17:58:04 -0400 Subject: [PATCH 4/6] Rename MergeFun to MergeRegex --- coordinator/coordinator.go | 10 ++++------ engine/engine.go | 2 +- parser/from_clause.go | 8 ++++---- parser/parser.go | 2 +- parser/parser_test.go | 2 +- parser/query.yacc | 2 +- parser/query_types.h | 2 +- parser/rewrite.go | 2 +- 8 files changed, 14 insertions(+), 16 deletions(-) diff --git a/coordinator/coordinator.go b/coordinator/coordinator.go index e6a955274b9..c3f2233bac4 100644 --- a/coordinator/coordinator.go +++ b/coordinator/coordinator.go @@ -278,13 +278,11 @@ func (self *Coordinator) expandRegex(spec *parser.QuerySpec) { return } - if f := q.FromClause; f.Type == parser.FromClauseMergeFun { - f := func(r *regexp.Regexp) []string { - return self.clusterConfiguration.MetaStore.GetSeriesForDatabaseAndRegex(spec.Database(), r) - } - - parser.RewriteMergeQuery(q, f) + f := func(r *regexp.Regexp) []string { + return self.clusterConfiguration.MetaStore.GetSeriesForDatabaseAndRegex(spec.Database(), r) } + + parser.RewriteMergeQuery(q, f) } // We call this function only if we have a Select query (not continuous) or Delete query diff --git a/engine/engine.go b/engine/engine.go index d46f822bb65..8b72b53aea5 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -25,7 +25,7 @@ func NewQueryEngine(next Processor, query *parser.SelectQuery, shards []uint32) tables[i] = name.Name.Name } engine = NewMergeEngine(shards, query.Ascending, engine) - case parser.FromClauseMergeFun: + case parser.FromClauseMergeRegex: // At this point the regex should be expanded to the list of // tables that will be queries panic("QueryEngine cannot be called with merge function") diff --git a/parser/from_clause.go b/parser/from_clause.go index 5429baa2730..60621576d35 100644 --- a/parser/from_clause.go +++ b/parser/from_clause.go @@ -13,10 +13,10 @@ import "fmt" type FromClauseType int const ( - FromClauseArray FromClauseType = C.FROM_ARRAY - FromClauseMerge FromClauseType = C.FROM_MERGE - FromClauseInnerJoin FromClauseType = C.FROM_INNER_JOIN - FromClauseMergeFun FromClauseType = C.FROM_MERGE_FUNCTION + FromClauseArray FromClauseType = C.FROM_ARRAY + FromClauseMerge FromClauseType = C.FROM_MERGE + FromClauseInnerJoin FromClauseType = C.FROM_INNER_JOIN + FromClauseMergeRegex FromClauseType = C.FROM_MERGE_REGEX ) func (self *TableName) GetAlias() string { diff --git a/parser/parser.go b/parser/parser.go index 200b9da9e5c..31830f0dd4c 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -473,7 +473,7 @@ func GetFromClause(fromClause *C.from_clause) (*FromClause, error) { var regex *regexp.Regexp switch t { - case FromClauseMergeFun: + case FromClauseMergeRegex: val, err := GetValue(fromClause.regex_value) if err != nil { return nil, err diff --git a/parser/parser_test.go b/parser/parser_test.go index 92e97817706..23e88475146 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -287,7 +287,7 @@ func (self *QueryParserSuite) TestParseFromWithMergeRegex(c *C) { q, err := ParseSelectQuery("select count(*) from merge(/.*/) where time>now()-1d;") c.Assert(err, IsNil) fromClause := q.GetFromClause() - c.Assert(fromClause.Type, Equals, FromClauseMergeFun) + c.Assert(fromClause.Type, Equals, FromClauseMergeRegex) c.Assert(fromClause.Regex, NotNil) } diff --git a/parser/query.yacc b/parser/query.yacc index fe2a086ce77..cf584dea7ff 100644 --- a/parser/query.yacc +++ b/parser/query.yacc @@ -426,7 +426,7 @@ FROM_CLAUSE: FROM MERGE '(' REGEX_VALUE ')' { $$ = calloc(1, sizeof(from_clause)); - $$->from_clause_type = FROM_MERGE_FUNCTION; + $$->from_clause_type = FROM_MERGE_REGEX; $$->regex_value = $4; } | diff --git a/parser/query_types.h b/parser/query_types.h index 85a5eb03b00..2c69848a138 100644 --- a/parser/query_types.h +++ b/parser/query_types.h @@ -73,7 +73,7 @@ typedef struct { FROM_ARRAY, FROM_MERGE, FROM_INNER_JOIN, - FROM_MERGE_FUNCTION + FROM_MERGE_REGEX } from_clause_type; // in case of merge or join, it's guaranteed that the names array // will have two table names only and they aren't regex. diff --git a/parser/rewrite.go b/parser/rewrite.go index 047f70ee458..2b14df56ce8 100644 --- a/parser/rewrite.go +++ b/parser/rewrite.go @@ -12,7 +12,7 @@ type RegexMatcher func(r *regexp.Regexp) []string // the query will be rewritten to // select * from merge(foobar, foobaz) func RewriteMergeQuery(query *SelectQuery, rm RegexMatcher) { - if query.FromClause.Type != FromClauseMergeFun { + if query.FromClause.Type != FromClauseMergeRegex { return } From 3300b3434f76dec2305b67bb703cd532e3bdd0bd Mon Sep 17 00:00:00 2001 From: John Shahid Date: Wed, 22 Oct 2014 18:05:25 -0400 Subject: [PATCH 5/6] Support merge with a list of series names --- parser/from_clause.go | 6 +++--- parser/parser_test.go | 1 - parser/query.yacc | 7 +++++++ parser/rewrite.go | 2 +- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/parser/from_clause.go b/parser/from_clause.go index 60621576d35..915f7981852 100644 --- a/parser/from_clause.go +++ b/parser/from_clause.go @@ -48,14 +48,14 @@ func (self *FromClause) GetString() string { buffer := bytes.NewBufferString("") switch self.Type { case FromClauseMerge: - fmt.Fprintf(buffer, "merge(/") + fmt.Fprintf(buffer, "merge(") for i, n := range self.Names { if i > 0 { - buffer.WriteRune('|') + buffer.WriteRune(',') } buffer.WriteString(n.Name.GetString()) } - buffer.WriteString("/)") + buffer.WriteString(")") case FromClauseInnerJoin: fmt.Fprintf(buffer, "%s%s inner join %s%s", self.Names[0].Name.GetString(), self.Names[0].GetAliasString(), self.Names[1].Name.GetString(), self.Names[1].GetAliasString()) diff --git a/parser/parser_test.go b/parser/parser_test.go index 23e88475146..b412a67cb2e 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -44,7 +44,6 @@ func (self *QueryParserSuite) TestParseMergeGetString(c *C) { c.Assert(err, IsNil) c.Assert(qs, HasLen, 1) RewriteMergeQuery(qs[0].SelectQuery, f) - fmt.Printf("Parsing %s\n", qs[0].GetQueryStringWithTimeCondition()) actualQs, err := ParseQuery(qs[0].GetQueryStringWithTimeCondition()) c.Assert(err, IsNil) c.Assert(actualQs, HasLen, 1) diff --git a/parser/query.yacc b/parser/query.yacc index cf584dea7ff..9e256df24bd 100644 --- a/parser/query.yacc +++ b/parser/query.yacc @@ -423,6 +423,13 @@ FROM_CLAUSE: $$->from_clause_type = FROM_MERGE; } | + FROM MERGE '(' SIMPLE_TABLE_VALUES ')' + { + $$ = calloc(1, sizeof(from_clause)); + $$->names = $4; + $$->from_clause_type = FROM_MERGE; + } + | FROM MERGE '(' REGEX_VALUE ')' { $$ = calloc(1, sizeof(from_clause)); diff --git a/parser/rewrite.go b/parser/rewrite.go index 2b14df56ce8..b98dab396f5 100644 --- a/parser/rewrite.go +++ b/parser/rewrite.go @@ -22,7 +22,7 @@ func RewriteMergeQuery(query *SelectQuery, rm RegexMatcher) { f.Regex = nil for _, s := range series { f.Names = append(f.Names, &TableName{ - Name: &Value{Name: s, Type: ValueTableName}, + Name: &Value{Name: s, Type: ValueSimpleName}, }) } } From b00b853c281b5860735a426cca78852ed0003740 Mon Sep 17 00:00:00 2001 From: John Shahid Date: Wed, 22 Oct 2014 18:16:26 -0400 Subject: [PATCH 6/6] add a test to make sure merge cannot be used with multiple regexes --- parser/parser_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/parser/parser_test.go b/parser/parser_test.go index b412a67cb2e..6ea63429b2a 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -34,6 +34,12 @@ func (self *QueryParserSuite) TestInvalidFromClause(c *C) { // Make sure that GetQueryStringWithTimeCondition() works for regex // merge queries. +func (self *QueryParserSuite) TestMergeMultipleRegex(c *C) { + query := "select * from merge(/.*foo.*/, /.*bar.*/)" + _, err := ParseQuery(query) + c.Assert(err, NotNil) +} + func (self *QueryParserSuite) TestParseMergeGetString(c *C) { f := func(r *regexp.Regexp) []string { return []string{"foobar"}