sql: SET TIME ZONE

[vivekmenezes]

closes #2327

[tamird]

why not put a time.Location in here?

[vivekmenezes]

Its a feature very rarely used and I see sql/executor being on the highly
travelled path and I'd hate to slow it down. As it is I hate the fact that
we need to move location info from session to eval context. perhaps we
should add session to evalcontext, and pick up the pieces from there when
needed?

On Mon, Sep 28, 2015 at 12:24 PM Tamir Duberstein [email protected]
wrote:

In sql/parser/eval.go
#2698 (comment):

@@ -448,6 +448,8 @@ type EvalContext struct {
NodeID uint32
StmtTimestamp time.Time
TxnTimestamp time.Time

• Location string

why not put a time.Location in here?

—
Reply to this email directly or view it on GitHub
https://github.com/cockroachdb/cockroach/pull/2698/files#r40572527.

[petermattis]

There is no requirement that the members of EvalContext need to be primitive types or structs. We could very easily add a GetTimeZone function pointer.

[vivekmenezes]

EvalContext is in sql/parser and cannot depend on sql.

[bdarnell]

Even if time.LoadLocation is slow, we could put a cache in front of it. I'd prefer to make this a time.Location and just resolve the session offset/location to a time zone in sql/executor.go.

[petermattis]

@vivekmenezes Not sure which comment you were responding to. I certainly wasn't suggesting making sql/parser depend on sql. If you added a GetLocation func() *time.Location function pointer to EvalContext, you could initialize that function pointer to parse the location from the session. sql/parser would effectively be calling into sql code, but there would be no dependency from sql/parser on the sql package.

[petermattis]

CircleCI certainly should have the zoneinfo files. I think the failure might be Linux vs Mac OS X and something that needs to be fixed. It looks like the location name you pass in to time.LoadLocation is used to read a file of that name within zoneinfo.zip. On Mac OS X, filenames are case insensitive. On Linux they are case sensitive. Try change the logic_test to use Europe/Rome instead of EUROPE/ROME.

[bdarnell]

Same as whose local? I don't think we should ever use the server's local time for anything; it should either be a client-supplied timezone or UTC.

[vivekmenezes]

That calls for never allowing the use of Local/Default; I think that is the correct approach.

[petermattis]

Or it calls for defining local/default as UTC. We almost certainly want to allow SET TIME ZONE DEFAULT to reset to UTC if UTC is the default time zone the session starts in.

[vivekmenezes]

done

[bdarnell]

This comment still says "same as local"; it should say that both local and default are defined as UTC.

[bdarnell]

I think the circleci failure is a case-sensitivity issue. Filenames are case-sensitive on linux but not osx. Change the test from EUROPE/ROME to Europe/Rome.

[bdarnell]

Also, our deployment docker image is built on the busybox base, which does not include a zoneinfo database. We'll need to either install zoneinfo ourselves when we build the image, or switch to a richer base image (or require that users mount the host's zoneinfo directory into the container when running, so we don't have to roll new docker images whenever zoneinfo changes).

We need to make sure we have some acceptance-style tests that use the deploy image instead of the builder image to be able to detect things like missing zoneinfo. (this doesn't have to be a part of this change)

[petermattis]

Good point about the zoneinfo file being needed in the deployment image.
Note that the acceptance tests are already run against the deployment image
(see build/push-docker-deploy.sh). It would not be unreasonable to run
all of the SQL logic tests against the production image.

On Mon, Sep 28, 2015 at 3:08 PM, Ben Darnell [email protected]
wrote:

Also, our deployment docker image is built on the busybox base, which
does not include a zoneinfo database. We'll need to either install
zoneinfo ourselves when we build the image, or switch to a richer base
image (or require that users mount the host's zoneinfo directory into the
container when running, so we don't have to roll new docker images whenever
zoneinfo changes).

We need to make sure we have some acceptance-style tests that use the
deploy image instead of the builder image to be able to detect things like
missing zoneinfo. (this doesn't have to be a part of this change)

—
Reply to this email directly or view it on GitHub
#2698 (comment)
.

[vivekmenezes]

I had to make this change because IntVal is only allowed to be positive.

[petermattis]

Yep, that's correct.

[spencerkimball]

Strange not to make this a function on a Session object.

[vivekmenezes]

I think we much prefer pure functions that take in parameters by value.

On Tue, Sep 29, 2015 at 9:27 AM Spencer Kimball [email protected]
wrote:

In sql/executor.go
#2698 (comment):

@@ -345,3 +350,13 @@ func (p parameters) Arg(name string) (parser.Datum, bool) {
panic(fmt.Sprintf("unexpected type %T", t))
}
}
+
+func getTimeZone(s Session) (*time.Location, error) {

Strange not to make this a function on a Session object.

—
Reply to this email directly or view it on GitHub
https://github.com/cockroachdb/cockroach/pull/2698/files#r40670225.

[tamird]

@vivekmenezes it's possible to have a method that takes its receiver by value. Why would we prefer a function?

[vivekmenezes]

holy shit, i never expected that!

http://play.golang.org/p/c1U2XZSW8O

[petermattis]

(almost) LGTM. This was more complicated than I expected.

[bdarnell]

ctx.Location == nil should be equivalent to ctx.Location == time.UTC. If we don't have a specified time zone we should still parse whatever is parseable in UTC (which I hope is only those time zones which are unambiguous).

[vivekmenezes]

using time.Parse() is pretty unpredictable, and can easily result in us parsing the time incorrectly. For instance, interpretting a PST timestamp as a UTC timestamp. I'd rather err on the side of being predictable.

[bdarnell]

We may want to just remove the timestampWithNamedZoneFormat then. From my experimentation it seems extremely limited.

[vivekmenezes]

Any other comments on this PR?

[bdarnell]

LGTM

[petermattis]

Minor nit: our convention so far has been to place methods on protos in a file with the same base name as the .proto. So this should go in session.go.

[petermattis]

LGTM

[tamird]

do we really want to discard the original error?

[vivekmenezes]

It's an error specific to the particular format. Hmmm, perhaps err will be more instructive to the user.

[tamird]

why not use the oneof generated member here?

switch t := s.Timezone.(type) {
  case nil:
    return time.UTC, nil
  case *Session_Location:
    return time.LoadLocation(t.location)
  case *Session_Offset:
    return time.FixedZone("", int(t.offset)), nil
  default:
    return nil, util.Errorf("unhandled timezone variant type %T", t)
}

[vivekmenezes]

done