Allow the implicit time field to be renamed

[jsternberg]

Fixes #6296.

[jsternberg]

@benbjohnson

[benbjohnson]

@jsternberg Can you set TimeFieldName to "time" instead of "" if it's not aliases. Then we won't have to worry about checking default values during processing.

[jsternberg]

The reason why I didn't do that is so there was less code to change and so I didn't have to add that logic to the parser. The diff would be a lot larger if that field is set to "time" by default.

I think I'm going to rename TimeFieldName to TimeAlias so that it's more consistent with the Alias attribute in Field. This might also make it more clear that the alias may be empty.

[benbjohnson]

Can you give some examples of what needs to change? It seems like adding time alias checking everywhere in the future would be error prone.

[jsternberg]

It depends on where we set the TimeAlias. If we set it as part of parseSelectStatement, then I have to modify all of the select statement tests inside of parser_test.go. If we set it in RewriteTimeFields, then I don't have to change it anywhere since that's not really used during tests, but I also don't think it's the appropriate place to put it.

I also think that checking for the empty string in one place isn't too hard and it only has to be done in one place.

[benbjohnson]

If it's just test code that's not that bad. We may only have one place to do the if statement right now but queries are going to get more complicated as we deal with referencing times in subqueries and that's just going to grow.

[jsternberg]

I don't think that really applies for this. This code does not rename the time field. It is merely a placeholder to hold onto the alias if there is a time present in the query when the query is rewritten. Another location that has to be changed is the code that sets this to begin with. Currently it's just this:

func (s *SelectStatement) RewriteTimeFields() {
    for i := 0; i < len(s.Fields); i++ {
        switch expr := s.Fields[i].Expr.(type) {
        case *VarRef:
            if expr.Val == "time" {
                s.TimeAlias = s.Fields[i].Alias
                s.Fields = append(s.Fields[:i], s.Fields[i+1:]...)
            }
        }
    }
}

This has to be changed to:

func (s *SelectStatement) RewriteTimeFields() {
    for i := 0; i < len(s.Fields); i++ {
        switch expr := s.Fields[i].Expr.(type) {
        case *VarRef:
            if expr.Val == "time" {
                if alias := s.Fields[i].Alias; alias != "" {
                    s.TimeAlias = alias
                }
                s.Fields = append(s.Fields[:i], s.Fields[i+1:]...)
            }
        }
    }
}

We don't allow aliases to be used in conditions at the moment. Is that something we're supposed to be doing? I don't really see this as affecting subqueries because these values are only used for setting the column names.

> select * from cpu
name: cpu
---------
time                    value
1460579388339842107     2
1460597252611368284     5

> select value as v from cpu where v > 2
>

If it makes it better, I can add a function to the SelectStatement that does the if statement. That might be a better solution as then it matches the same code we use for other fields. Something like:

func (s *SelectStatement) TimeFieldName() string {
    if s.TimeAlias != "" {
        return s.TimeAlias
    }
    return "time"
}

[benbjohnson]

OK, leave off the "time" for now. We can revisit if it becomes an issue.

[benbjohnson]

Otherwise lgtm

[e-dard]

LGTM 👍