Implement the CONVERT_TZ() SQL function #532

VinaiRachakonda · 2021-08-20T23:43:06Z

No description provided.

zachmu

What's here is good, but this needs to work on datetime columns as wel

zachmu · 2021-08-23T16:58:10Z

enginetest/queries.go

@@ -5106,6 +5106,18 @@ var QueryTests = []QueryTest{
 			},
 		},
 	},
+	{
+		Query: `SELECT CONVERT_TZ("2004-01-01 4:00:00", "GMT", "MET")`,


Need an engine test that select a column as well, not just a literal.

There's a table with datetime values for this purpose

zachmu · 2021-08-23T17:02:05Z

sql/expression/function/convert_tz.go

+		return nil, err
+	}
+
+	datetimeStr, ok := datetime.(string)


This isn't right. This value can be anything that can be converted to a datetime, not just a string

Need a test for this

VinaiRachakonda · 2021-08-23T20:10:54Z

Great catch

VinaiRachakonda · 2021-08-24T03:14:29Z

sql/datetimetype.go

@@ -205,7 +205,7 @@ func (t datetimeType) ConvertWithoutRangeCheck(v interface{}) (time.Time, error)
 			return zeroTime, ErrConvertingToTime.New(v)
 		}
 	case time.Time:
-		res = value.UTC()
+		res = value


@zachmu Pretty confident that we need this change. For datetime objects (time.Time) it's essential we return the object with the correct timezone loaded in. Converting to UTC consistently does not seem correct

@Hydrocharged do you have any thoughts about this? It doesn't seem to break any tests but I'm a little wary of changing the semantics here

I actually don't think this will change anything for any tests that hit Dolt. All throughout Dolt we call time.UTC() to make sure that we're standardizing them. There's probably other places in gms where we do this time.UTC() call so I'm not sure if this is even changing anything. From a conceptual level though, this particular change affects both TIMESTAMP and DATETIME. TIMESTAMP is definitely supposed to convert to UTC, and DATETIME does not.

For the purposes of CONVERT_TZ, this change shouldn't be necessary. When you call time.UTC(), Go is simply setting the internal location to UTC. It's easy to simply set it back in your CONVERT_TZ function (which you're going to have to do anyway). It doesn't change the internal unix timestamp at all. In fact, all locations/timezones are just for display purposes as far as Go is concerned, so converting it to UTC, finding the timezone delta, and applying that to the time gives the exact same result as far as what is exposed from MySQL.

We shouldn't be comparing times using ==, but if we are, having everything as UTC at least ensures that all of those will evaluate correctly. We could treat UTC as "no timezone", and any "timezone modifications" would just offset the actual timestamp by some amount. There are probably more changes that would need to be made to accommodate this, but it's far easier if we can ensure all times are set to UTC to begin with.

TL;DR: this change isn't necessary and we can leave it as UTC() for the sake of consistency, as multiple places reference this. If this change is required to make your code work, then I'm not sure that code is correct.

The problem isn't with the internal timestamp it's more so with how we return the final outputted value to the user. For example, say I want to return a datetime with the following call SELECT CONVERT_TZ("2004-01-01 4:00:00", "UTC", "EST"), I want my output to produce the output form in the correct timezone (EST). When the .UTC() function is enabled on every returned datetime object, GMS will produce an output SQL string of the UTC time. From the user perspective, I am getting my time told in UTC when I want it in EST!

TLDR: My problem is with the returned SQL string that the datetime type creates by always doing .UTC on ConvertWithRangeCheck. It's forcing a functionality that the user is not looking for.

No need to do a TL;DR when it's half the size of the paragraph lol. It's only for stuff that's "too long" lol

So SELECT CONVERT_TZ("2004-01-01 4:00:00", "UTC", "EST") should give an output of 2003-12-31 23:00:00, which does not have any location information. We return 2003-12-31 23:00:00 +0000 UTC which is incorrect. In the first scenario it doesn't matter whether the time.Time has a specific location, all we need to return is the date and time portion. If all of our times have the same location internally, then we can treat them all as though none of them have any timezone information. The user wants 2003-12-31 23:00:00, which is applying the offset from UTC to EST to our initial 2004-01-01 4:00:00.

MySQL internally does not carry any timezone information with its time constructs. More than likely, for TIMESTAMP it applies an offset between the connection timezone and UTC and stores just the date and time portion internally. Then it does the reverse upon retrieval. We can't throw away timezone information by using time.Time, so setting them all to UTC is effectively the same.

zachmu

Other changes look fine, want Daylon's opinion on the UTC semantics change

zachmu · 2021-08-24T18:58:26Z

sql/datetimetype.go

@@ -205,7 +205,7 @@ func (t datetimeType) ConvertWithoutRangeCheck(v interface{}) (time.Time, error)
 			return zeroTime, ErrConvertingToTime.New(v)
 		}
 	case time.Time:
-		res = value.UTC()
+		res = value


@Hydrocharged do you have any thoughts about this? It doesn't seem to break any tests but I'm a little wary of changing the semantics here

Hydrocharged

Looks good! A couple of things and you're free to merge!

Hydrocharged · 2021-08-27T23:27:01Z

sql/expression/function/convert_tz.go

+	// Get the time in UTC.
+	datetime = time.Date(datetime.Year(), datetime.Month(), datetime.Day(), datetime.Hour(), datetime.Minute(), datetime.Second(), datetime.Nanosecond(), time.UTC)


If we remove this, does anything change? ConvertWithoutRangeCheck should already be returning a UTC time, so this seems like it shouldn't change anything.

Hydrocharged · 2021-08-27T23:31:09Z

sql/expression/function/convert_tz.go

+	getCopy := func(t time.Time, loc *time.Location) time.Time {
+		return time.Date(t.Year(), t.Month(), t.Day(), t.Hour(), t.Minute(), t.Second(), t.Nanosecond(), loc).UTC()
+	}


I'd make this a function in the module namespace rather than one created inside of this function. I'm assuming this was done to avoid cluttering the namespace, so you could always make it a function of *ConvertTz, e.g.:

func (c *ConvertTz) getCopy(t time.Time, loc *time.Location) time.Time { return time.Date(t.Year(), t.Month(), t.Day(), t.Hour(), t.Minute(), t.Second(), t.Nanosecond(), loc).UTC() }

Not sure if it needs to be a method on ConvertTz. I'll take this out of the other function though

Hydrocharged · 2021-08-27T23:34:58Z

sql/expression/function/convert_tz.go

+	}
+
+	converted := convertTimeZone(datetime, fromStr, toStr)
+	if !converted.IsZero() {


If converted is the zero time then we attempt to parse offsets. What if the conversation is supposed to result in the zero time? Is that an error or is that valid? Perhaps a success bool being returned if it is valid.

VinaiRachakonda added 5 commits August 20, 2021 13:16

basic conversion

9b1fe1e

deltas

50b8903

add more tests

27fc639

fix flaw in functionality

3be2326

nitsd before review

bdd4aa1

VinaiRachakonda changed the title ~~[wip] Vinai/convert tz~~ Add functionality for CONVERT_TZ() Aug 21, 2021

VinaiRachakonda requested a review from zachmu August 21, 2021 00:19

small nit

18dfcbb

VinaiRachakonda removed the request for review from zachmu August 23, 2021 16:39

small engine tests

a7ef377

VinaiRachakonda requested a review from zachmu August 23, 2021 16:51

zachmu reviewed Aug 23, 2021

View reviewed changes

VinaiRachakonda added 3 commits August 23, 2021 15:12

fix timezone and datetime return

b6652b0

Refactor signicantly by changing how timezone parsing is done

86ddcb7

do my -

3634562

VinaiRachakonda commented Aug 24, 2021

View reviewed changes

add another test case

4f3addd

zachmu reviewed Aug 24, 2021

View reviewed changes

VinaiRachakonda changed the title ~~Add functionality for CONVERT_TZ()~~ Implement the CONVERT_TZ() SQL function Aug 24, 2021

switch to a diff method to preserve utc

b14fe9d

VinaiRachakonda requested a review from Hydrocharged August 27, 2021 02:22

Hydrocharged approved these changes Aug 27, 2021

View reviewed changes

cleanup stuff

be4ff3c

VinaiRachakonda merged commit 13e1f53 into master Aug 30, 2021

Hydrocharged deleted the vinai/convert_tz branch December 8, 2021 07:08

		// Get the time in UTC.
		datetime = time.Date(datetime.Year(), datetime.Month(), datetime.Day(), datetime.Hour(), datetime.Minute(), datetime.Second(), datetime.Nanosecond(), time.UTC)

Uh oh!

Implement the CONVERT_TZ() SQL function #532

Implement the CONVERT_TZ() SQL function #532

Uh oh!

Conversation

VinaiRachakonda commented Aug 20, 2021

Uh oh!

zachmu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VinaiRachakonda commented Aug 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VinaiRachakonda Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hydrocharged Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hydrocharged left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VinaiRachakonda Aug 25, 2021 •

edited

Loading

Hydrocharged Aug 25, 2021 •

edited

Loading