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.

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 alias, maintain column sort order #3564

Merged
merged 3 commits into from
Aug 6, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- [#3529](https://github.com/influxdb/influxdb/pull/3529): Add TLS support for OpenTSDB plugin. Thanks @nathanielc
- [#3421](https://github.com/influxdb/influxdb/issues/3421): Should update metastore and cluster if IP or hostname changes
- [#3502](https://github.com/influxdb/influxdb/pull/3502): Importer for 0.8.9 data via the CLI
- [#3564](https://github.com/influxdb/influxdb/pull/3564): Fix alias, maintain column sort order

### Bugfixes
- [#3405](https://github.com/influxdb/influxdb/pull/3405): Prevent database panic when fields are missing. Thanks @jhorwit2
Expand All @@ -22,6 +23,7 @@
- [#3545](https://github.com/influxdb/influxdb/issues/3545): Fix parsing string fields with newlines
- [#3579](https://github.com/influxdb/influxdb/issues/3579): Revert breaking change to `client.NewClient` function
- [#3580](https://github.com/influxdb/influxdb/issues/3580): Do not allow wildcards with fields in select statements
- [#3530](https://github.com/influxdb/influxdb/pull/3530): Aliasing a column no longer works

## v0.9.2 [2015-07-24]

Expand Down
80 changes: 76 additions & 4 deletions cmd/influxd/run/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,7 @@ func TestServer_Query_Tags(t *testing.T) {
&Query{
name: "field with two tags should succeed",
command: `SELECT host, value, core FROM db0.rp0.cpu`,
exp: fmt.Sprintf(`{"results":[{"series":[{"name":"cpu","tags":{"host":"server01"},"columns":["time","core","value"],"values":[["%s",4,100]]},{"name":"cpu","tags":{"host":"server02"},"columns":["time","core","value"],"values":[["%s",2,50]]}]}]}`, now.Format(time.RFC3339Nano), now.Add(1).Format(time.RFC3339Nano)),
exp: fmt.Sprintf(`{"results":[{"series":[{"name":"cpu","tags":{"host":"server01"},"columns":["time","value","core"],"values":[["%s",100,4]]},{"name":"cpu","tags":{"host":"server02"},"columns":["time","value","core"],"values":[["%s",50,2]]}]}]}`, now.Format(time.RFC3339Nano), now.Add(1).Format(time.RFC3339Nano)),
},
&Query{
name: "select * with tags should succeed",
Expand Down Expand Up @@ -1014,6 +1014,78 @@ func TestServer_Query_Tags(t *testing.T) {
}
}

// Ensure the server correctly queries with an alias.
func TestServer_Query_Alias(t *testing.T) {
t.Parallel()
s := OpenServer(NewConfig(), "")
defer s.Close()

if err := s.CreateDatabaseAndRetentionPolicy("db0", newRetentionPolicyInfo("rp0", 1, 1*time.Hour)); err != nil {
t.Fatal(err)
}

writes := []string{
fmt.Sprintf("cpu value=1i,steps=3i %d", mustParseTime(time.RFC3339Nano, "2000-01-01T00:00:00Z").UnixNano()),
fmt.Sprintf("cpu value=2i,steps=4i %d", mustParseTime(time.RFC3339Nano, "2000-01-01T00:01:00Z").UnixNano()),
}
test := NewTest("db0", "rp0")
test.write = strings.Join(writes, "\n")

test.addQueries([]*Query{
&Query{
name: "baseline query - SELECT * FROM db0.rp0.cpu",
command: `SELECT * FROM db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","steps","value"],"values":[["2000-01-01T00:00:00Z",3,1],["2000-01-01T00:01:00Z",4,2]]}]}]}`,
},
&Query{
name: "basic query with alias - SELECT steps, value as v FROM db0.rp0.cpu",
command: `SELECT steps, value as v FROM db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","steps","v"],"values":[["2000-01-01T00:00:00Z",3,1],["2000-01-01T00:01:00Z",4,2]]}]}]}`,
},
&Query{
name: "double aggregate sum - SELECT sum(value), sum(steps) FROM db0.rp0.cpu",
command: `SELECT sum(value), sum(steps) FROM db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","sum","sum"],"values":[["1970-01-01T00:00:00Z",3,7]]}]}]}`,
},
&Query{
name: "double aggregate sum reverse order - SELECT sum(steps), sum(value) FROM db0.rp0.cpu",
command: `SELECT sum(steps), sum(value) FROM db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","sum","sum"],"values":[["1970-01-01T00:00:00Z",7,3]]}]}]}`,
},
&Query{
name: "double aggregate sum with alias - SELECT sum(value) as sumv, sum(steps) as sums FROM db0.rp0.cpu",
command: `SELECT sum(value) as sumv, sum(steps) as sums FROM db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","sumv","sums"],"values":[["1970-01-01T00:00:00Z",3,7]]}]}]}`,
},
&Query{
name: "double aggregate with same value - SELECT sum(value), mean(value) FROM db0.rp0.cpu",
command: `SELECT sum(value), mean(value) FROM db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","sum","mean"],"values":[["1970-01-01T00:00:00Z",3,1.5]]}]}]}`,
},
&Query{
name: "double aggregate with same value and same alias - SELECT mean(value) as mv, max(value) as mv FROM db0.rp0.cpu",
command: `SELECT mean(value) as mv, max(value) as mv FROM db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","mv","mv"],"values":[["1970-01-01T00:00:00Z",1.5,2]]}]}]}`,
},
}...)

if err := test.init(s); err != nil {
t.Fatalf("test init failed: %s", err)
}

for _, query := range test.queries {
if query.skip {
t.Logf("SKIP:: %s", query.name)
continue
}
if err := query.Execute(s); err != nil {
t.Error(query.Error(err))
} else if !query.success() {
t.Error(query.failureMessage())
}
}
}

// Ensure the server will succeed and error for common scenarios.
func TestServer_Query_Common(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -1226,7 +1298,7 @@ func TestServer_Query_SelectRawCalculus(t *testing.T) {
&Query{
name: "calculate single derivate",
command: `SELECT derivative(value) from db0.rp0.cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","value"],"values":[["2010-07-01T18:47:02Z",-200]]}]}]}`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","derivative"],"values":[["2010-07-01T18:47:02Z",-200]]}]}]}`,
},
}...)

Expand Down Expand Up @@ -2134,7 +2206,7 @@ func TestServer_Query_Where_Fields(t *testing.T) {
name: "string AND query, all fields in SELECT",
params: url.Values{"db": []string{"db0"}},
command: `SELECT alert_id,tenant_id,_cust FROM cpu WHERE alert_id='alert' AND tenant_id='tenant'`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","_cust","alert_id","tenant_id"],"values":[["2015-02-28T01:03:36.703820946Z","johnson brothers","alert","tenant"]]}]}]}`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","alert_id","tenant_id","_cust"],"values":[["2015-02-28T01:03:36.703820946Z","alert","tenant","johnson brothers"]]}]}]}`,
},
&Query{
name: "string AND query, all fields in SELECT, one in parenthesis",
Expand Down Expand Up @@ -3086,7 +3158,7 @@ func TestServer_Query_EvilIdentifiers(t *testing.T) {
&Query{
name: `query evil identifiers`,
command: `SELECT "select", "in-bytes" FROM cpu`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","in-bytes","select"],"values":[["2000-01-01T00:00:00Z",2,1]]}]}]}`,
exp: `{"results":[{"series":[{"name":"cpu","columns":["time","select","in-bytes"],"values":[["2000-01-01T00:00:00Z",1,2]]}]}]}`,
params: url.Values{"db": []string{"db0"}},
},
}...)
Expand Down
26 changes: 26 additions & 0 deletions influxql/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -1964,6 +1964,32 @@ func (s *ShowFieldKeysStatement) RequiredPrivileges() ExecutionPrivileges {
// Fields represents a list of fields.
type Fields []*Field

// AliasNames returns a list of calculated field names in
// order of alias, function name, then field.
func (a Fields) AliasNames() []string {
names := []string{}
for _, f := range a {
names = append(names, f.Name())
}
return names
}

// Names returns a list of raw field names.
func (a Fields) Names() []string {
names := []string{}
for _, f := range a {
var name string
switch expr := f.Expr.(type) {
case *Call:
name = expr.Name
case *VarRef:
name = expr.Val
}
names = append(names, name)
}
return names
}

// String returns a string representation of the fields.
func (a Fields) String() string {
var str []string
Expand Down
37 changes: 28 additions & 9 deletions tsdb/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,18 @@ func (e *Executor) executeRaw(out chan *influxql.Row) {
}
}

// Get the union of SELECT fields across all mappers.
selectFields := newStringSet()
for _, m := range e.mappers {
selectFields.add(m.Fields()...)
// Get the distinct fields across all mappers.
var selectFields, aliasFields []string
if e.stmt.HasWildcard() {
sf := newStringSet()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this still be a stringset? What about select mean(value), max(value) from foo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For select * a stringSet works perfect as it gets you the distinct fields sorted alphabetically. It's when it wasn't a select * that the problem was encountered, and what this PR fixes.

select mean(value), max(value) from foo scenario works and is tested with this test:

https://github.com/influxdb/influxdb/pull/3564/files#diff-09d8dc75f001e1e2a64439ee67dd14cbR1062

for _, m := range e.mappers {
sf.add(m.Fields()...)
}
selectFields = sf.list()
aliasFields = selectFields
} else {
selectFields = e.stmt.Fields.Names()
aliasFields = e.stmt.Fields.AliasNames()
}

// Used to read ahead chunks from mappers.
Expand Down Expand Up @@ -290,7 +298,8 @@ func (e *Executor) executeRaw(out chan *influxql.Row) {
chunkSize: e.chunkSize,
name: chunkedOutput.Name,
tags: chunkedOutput.Tags,
selectNames: selectFields.list(),
selectNames: selectFields,
aliasNames: aliasFields,
fields: e.stmt.Fields,
c: out,
}
Expand Down Expand Up @@ -562,8 +571,9 @@ type limitedRowWriter struct {
offset int
name string
tags map[string]string
selectNames []string
fields influxql.Fields
selectNames []string
aliasNames []string
c chan *influxql.Row

currValues []*MapperValue
Expand Down Expand Up @@ -658,6 +668,7 @@ func (r *limitedRowWriter) processValues(values []*MapperValue) *influxql.Row {
}()

selectNames := r.selectNames
aliasNames := r.aliasNames

if r.transformer != nil {
values = r.transformer.Process(values)
Expand All @@ -679,21 +690,24 @@ func (r *limitedRowWriter) processValues(values []*MapperValue) *influxql.Row {
// time should always be in the list of names they get back
if !hasTime {
selectNames = append([]string{"time"}, selectNames...)
aliasNames = append([]string{"time"}, aliasNames...)
}

// since selectNames can contain tags, we need to strip them out
selectFields := make([]string, 0, len(selectNames))
aliasFields := make([]string, 0, len(selectNames))

for _, n := range selectNames {
for i, n := range selectNames {
if _, found := r.tags[n]; !found {
selectFields = append(selectFields, n)
aliasFields = append(aliasFields, aliasNames[i])
}
}

row := &influxql.Row{
Name: r.name,
Tags: r.tags,
Columns: selectFields,
Columns: aliasFields,
}

// Kick out an empty row it no results available.
Expand All @@ -710,7 +724,12 @@ func (r *limitedRowWriter) processValues(values []*MapperValue) *influxql.Row {

if singleValue {
vals[0] = time.Unix(0, v.Time).UTC()
vals[1] = v.Value.(interface{})
switch val := v.Value.(type) {
case map[string]interface{}:
vals[1] = val[selectFields[1]]
default:
vals[1] = v.Value.(interface{})
}
} else {
fields := v.Value.(map[string]interface{})

Expand Down