sql: Fix for invalid value for parameter "TimeZone": "GMT-08:00" #41776 #42119
Conversation
|
This change is |
|
|
|
Hi @droidnoob, thanks for the PR! Sorry this got left for a few days - we'll take a look at it soon. |
|
@jordanlewis Is there any changes to be made in this? |
There was a problem hiding this comment.
Thanks! I have a couple of nits - and a correction on psql behaviour now that I have access to a computer.
Two more high level comments:
- You have attached a bunch of extraneous changes from other parts of the codebase as a result of (what looks like) a bad merge -- we shouldn't have these changes visible in this PR. Ideally, we should have one commit with all changes. You should squash commits if you have multiple - you may need to
git push --forceto push it onto github. Let me know if you need help with this! - It'd be great if you can a basic integration test onto
pkg/sql/logictest/testdata/logic_test/timestampthat does something like this (you can test this by usingmake testbaselogic FILES='timestamp' TESTFLAGS='-rewrite'):
SET TIME ZONE 'GMT+1';
SELECT '2001-01-01 00:00:00'::timestamp::timestamptz # which should return `2001-01-01 00:00:00+01:00`
|
|
||
| func TestTimeZoneOffsetStringConversion(t *testing.T) { | ||
| testCases := []struct { | ||
| os int64 |
There was a problem hiding this comment.
nit: can we rename this to offsetSecs and timezone? these names are not very descriptive :) also prefer if we rearrange the arguments to be {timezone, offsetSecs, ok}
also, thanks for adding negative tests -- can we test that the second argument ok returns as expected as well?
pkg/util/timeutil/time_zone_util.go
Outdated
| // (case insensitive) | ||
| func TimeZoneOffsetStringConversion(s string) (offset int64, ok bool) { | ||
| pattern := `(?i)(GMT|UTC)[+-]((((0?\d)|(1[0-5])):([0-5]\d))|(((0?\d)|(1[0-5])):([0-5]\d):([0-5]\d))|((1+[0-6])|(0?\d)))\b$` | ||
| re := regexp.MustCompile(pattern) |
There was a problem hiding this comment.
nit: we typically put MustCompile as a global var, so it only has to execute once to generate the regex and it will panic at init time rather than runtime if the regex does not compile :)
pkg/util/timeutil/time_zone_util.go
Outdated
| // Unsupported time zone strings :- GMT/UTC±16:00 to upper, GMT/UTC±6.5 | ||
| // (case insensitive) | ||
| func TimeZoneOffsetStringConversion(s string) (offset int64, ok bool) { | ||
| pattern := `(?i)(GMT|UTC)[+-]((((0?\d)|(1[0-5])):([0-5]\d))|(((0?\d)|(1[0-5])):([0-5]\d):([0-5]\d))|((1+[0-6])|(0?\d)))\b$` |
There was a problem hiding this comment.
this pattern is getting a little complicated :( -- we're doing a lot of offset checking using strings whereas i think it'd be cleaner to do it by comparing the offset against the min/max.
i think it may be cleaner to do something like
(?i)(GMT|UTC)([+-])(\d{1,3}(:[0-5]\d){0,2}) instead (allow up to 1-3 digits first, and there can be between 0 and 2 digit components), and we can check more out of bounds conditions by comparing (offsetSecs) to 167*60*60 + 59*60 and -(167*60*60 + 59*60)
There was a problem hiding this comment.
Thank you for simplifying the regex. I agree this is a better approach
pkg/util/timeutil/time_zone_util.go
Outdated
| return 0, false | ||
| } | ||
|
|
||
| prefix := "+" |
There was a problem hiding this comment.
this one is totally optional: if we use capture groups (https://stackoverflow.com/questions/30483652/how-to-get-capturing-group-functionality-in-go-regular-expressions), i think this can be a one liner
this may also simplify the split bit below to not depend on prefix
e.g. for (GMT|UTC)([+-)], if we use re.FindStringSubmatch(s)[0][2] to get + or -
pkg/util/timeutil/time_zone_util.go
Outdated
|
|
||
| // TimeZoneOffsetStringConversion converts a time string to offset seconds | ||
| // Supported time zone strings :- GMT/UTC±[00:00:00 - 15:59:59], GMT/UTC±[0-16] | ||
| // Unsupported time zone strings :- GMT/UTC±16:00 to upper, GMT/UTC±6.5 |
There was a problem hiding this comment.
Sorry for thinking 15:59 was the min/max, but turns out postgres allows up to 167:59:00...
otan=# set time zone 'UTC+168';
ERROR: invalid value for parameter "TimeZone": "UTC+168"
otan=# set time zone 'UTC+167:59';
SET
otan=# set time zone 'UTC+167:59:59';
ERROR: time zone "UTC+167:59:59" appears to use leap seconds
DETAIL: PostgreSQL does not support leap seconds.
otan=# set time zone 'UTC+167:59:00';
SET
otan=# set time zone 'UTC+167:59:00';
SET
otan=# set time zone 'UTC+167:59:01';
ERROR: time zone "UTC+167:59:01" appears to use leap seconds
DETAIL: PostgreSQL does not support leap seconds.
otan=# set time zone 'UTC-167:59:00';
SET
otan=# set time zone 'UTC-168';
ERROR: invalid value for parameter "TimeZone": "UTC-168"
otan=# set time zone 'UTC+168';
ERROR: invalid value for parameter "TimeZone": "UTC+168"
There was a problem hiding this comment.
Good to know. This will be resolved once the above changes are made :)
pkg/util/timeutil/time_zone_util.go
Outdated
| @@ -1,4 +1,4 @@ | |||
| // Copyright 2017 The Cockroach Authors. | |||
| // Copyright 2019 The Cockroach Authors. | |||
There was a problem hiding this comment.
nit: we don't have to change this :)
There was a problem hiding this comment.
I read that part of the wiki after this commit. :)
sql: added an extra string to time zone converter to check strings like
GMT-08:00there was no check in place to convert the strings to timezone. Added a regex to check for valid string and convert it to offset in timeutil package
Fix for #41776
Release Notes:
timezone="GMT-08:00",UST+11:00,-03:00.@rafiss