Skip to content

Commit

Permalink
builtins: fix incorrect timezone conversions for extract and `date_…
Browse files Browse the repository at this point in the history
…trunc`

`extract`: Due to actually taking `loc` into account for
`MakeDTimestampTZFromDate` (as opposed to ignoring the argument before) in
`827c97e04041e280a4413984d80cee99520c1d27`, this exposed other bugs.

`date_trunc`: `timestamp` was incorrect because it tried to adapt to the
timezone, even though it is returning a timestamp without time zone
type. Furthermore, the `date` needed an additional `offset` for a given
timezone as the given `date` is in UTC, but we need to truncate it in
the correct timezone. This meant that my fix in the previous PR only
fixed positive UTC offsets, but not negative UTC offsets.

This PR fixes the immediate issue of `extract` and `date_trunc`,
and I will make an audit of all other incorrect usages in an upcoming PR.

There will be an upcoming PR to fix more other bugs found in the Plus
and Minus expressions with the MakeDTimestampTZFromDate change.

Release note (bug fix): Previously, `<date>:date` when context
local timestamp is set would result in the previous day
(`<date-1>::date>`) if the timezone is less than UTC+00:00 due to a
recently introduced bug. This change fixes this.

Release note (bug fix): Previously, date_trunc for timestamp were
incorrect if in a local timezone was set. This change fixes this.

Release note (bug fix): Previously, date_trunc for date types were
incorrect with a negative timezone offset in a local timezone. This
change fixes this.
  • Loading branch information
otan committed Nov 7, 2019
1 parent e9dceea commit d89c232
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 6 deletions.
46 changes: 43 additions & 3 deletions pkg/sql/logictest/testdata/logic_test/datetime
Original file line number Diff line number Diff line change
Expand Up @@ -1377,18 +1377,44 @@ select date_trunc('day', '2011-01-01 22:30:00+01:00'::timestamptz);
2011-01-01 00:00:00 +0000 UTC

statement ok
SET TIME ZONE 'Africa/Nairobi';
SET TIME ZONE 'Africa/Nairobi'

query T
select date_trunc('day', '2011-01-01 22:30:00'::date);
select date_trunc('day', '2011-01-01 22:30:00'::date)
----
2011-01-01 00:00:00 +0300 +0300

query T
select date_trunc('day', '2011-01-01 22:30:00+01:00'::timestamptz);
select date_trunc('day', '2011-01-02 01:30:00'::timestamp)
----
2011-01-02 00:00:00 +0000 +0000

query T
select date_trunc('day', '2011-01-01 22:30:00+01:00'::timestamptz)
----
2011-01-02 00:00:00 +0300 +0300

statement ok
SET TIME ZONE -5

query TT
select date_trunc('day', '2011-01-02 01:30:00'::date), pg_typeof(date_trunc('day', '2011-01-02 01:30:00'::date))
----
2011-01-02 00:00:00 -0500 -0500 timestamptz

query TT
select date_trunc('day', '2011-01-02 01:30:00'::timestamp), pg_typeof(date_trunc('day', '2011-01-02 01:30:00'::timestamp))
----
2011-01-02 00:00:00 +0000 +0000 timestamp

query TT
select date_trunc('day', '2011-01-02 01:30:00+00:00'::timestamptz), pg_typeof(date_trunc('day', '2011-01-02 01:30:00+00:00'::timestamptz))
----
2011-01-01 00:00:00 -0500 -0500 timestamptz

statement ok
SET TIME ZONE 0

# Test negative years to ensure they can round-trip through the parser.
# Also ensure that we don't trigger any of the "convenience" rules.
# Update: dates now have a much more limited range such that the original
Expand Down Expand Up @@ -1482,3 +1508,17 @@ query T
SET timezone = 'utc'; SHOW timezone
----
UTC

subtest regression_42244

statement ok
SET TIME ZONE -5

# Check date is still configured correctly from day.
query I
select extract(day from '2019-01-15'::date) as final
----
15

statement ok
SET TIME ZONE 0
15 changes: 12 additions & 3 deletions pkg/sql/sem/builtins/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -1788,7 +1788,7 @@ may increase either contention or retry errors, or both.`,
Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
timeSpan := strings.ToLower(string(tree.MustBeDString(args[0])))
date := args[1].(*tree.DDate)
fromTSTZ, err := tree.MakeDTimestampTZFromDate(ctx.GetLocation(), date)
fromTSTZ, err := tree.MakeDTimestampTZFromDate(time.UTC, date)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1908,7 +1908,7 @@ may increase either contention or retry errors, or both.`,
if err != nil {
return nil, err
}
return tree.MakeDTimestamp(tsTZ.Time.In(ctx.GetLocation()), time.Microsecond), nil
return tree.MakeDTimestamp(tsTZ.Time, time.Microsecond), nil
},
Info: "Truncates `input` to precision `element`. Sets all fields that are less\n" +
"significant than `element` to zero (or one, for day and month)\n\n" +
Expand All @@ -1921,11 +1921,20 @@ may increase either contention or retry errors, or both.`,
Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) {
timeSpan := strings.ToLower(string(tree.MustBeDString(args[0])))
date := args[1].(*tree.DDate)
// Localize the timestamp into the given location.
fromTSTZ, err := tree.MakeDTimestampTZFromDate(ctx.GetLocation(), date)
if err != nil {
return nil, err
}
return truncateTimestamp(ctx, fromTSTZ.Time, timeSpan)
// From the given time in the context location, we also have to subtract
// the offset.
// This is because we expect to truncate in the given timezone,
// but the date argument assumed no timezone, meaning converting it into
// the location's timestamp with the date library converts it into the
// wrong time locally.
_, offset := fromTSTZ.Time.Zone()
fromTSTZTime := fromTSTZ.Time.Add(time.Duration(-offset) * time.Second)
return truncateTimestamp(ctx, fromTSTZTime, timeSpan)
},
Info: "Truncates `input` to precision `element`. Sets all fields that are less\n" +
"significant than `element` to zero (or one, for day and month)\n\n" +
Expand Down

0 comments on commit d89c232

Please sign in to comment.