Support for local timezone #118
Conversation
- add CLI option to take a connection string and use that to connect to a custom driver installation.
With this commit the driver will be able to use local timestamps towards the application and do the translation between that and ES/SQL (UTC) A connection string parameter controlls if this behavior is enabled; it is disabled by default. The timezone is read from the TZ environment variable, or the system if that isn't set. The driver will now also provide the SQL format of date/timestamp when a DATE/DATETIME value is converted to string.
- the commit also corrects some of the existing tests to respect the column size and/or decimal digits.
driver/convert.c
Outdated
| @@ -27,6 +28,17 @@ | |||
| (_tsp)->second = (_tmp)->tm_sec; \ | |||
| } while (0) | |||
|
|
|||
| #ifdef _WIN32 | |||
| # ifdef _WIN64 | |||
There was a problem hiding this comment.
mktime is an inline function that is equivalent to _mktime64, unless _USE_32BIT_TIME_T is defined
So the condition should be #ifndef _USE_32BIT_TIME_T. It doesn't matter whether the build is 32 bit or 64 bit but whether at compile time you asked for a 32 bit time_t. Modern 32 bit Windows still uses a 64 bit time_t by default.
(Note that this is different if you ever do a Linux version, as on 32 bit Linux time_t is 32 bit.)
There was a problem hiding this comment.
Oh, right! Good catch, I've made a wrong "automatic" assumption. Thanks, fixed it.
| @@ -243,6 +257,13 @@ void convert_init() | |||
| sql = SQL_GUID; | |||
| csql = SQL_C_GUID; | |||
| compat_matrix[ESSQL_TYPE_IDX(sql)][ESSQL_C_TYPE_IDX(csql)] = TRUE; | |||
|
|
|||
| /* TZ conversions */ | |||
| tzset(); | |||
There was a problem hiding this comment.
My experience a few years ago with how Microsoft's C runtime library handles TZ and tzset() was that it's pretty poor. My experience was that it worked well with the default settings for timezone inherited from the OS, but as soon as you asked the C runtime to deviate from the OS via TZ it switched to a horrible and highly non-standard implementation inside the C runtime itself rather than the Windows core DLLs.
Maybe they've resolved this in recent versions of the C runtime. I will try to dig out some references from the ML code on Monday.
Also, I wonder how useful being able to set this via an environment variable really is on Windows? Most users aren't aware of environment variables - they're not a natural thing to use like on *nix. It might be better to just offer the choice of UTC or the system timezone set via control panel.
There was a problem hiding this comment.
I will try to dig out some references from the ML code on Monday.
Sure, that'd be great. With the tests I've made it worked out as expected. There are a couple of unit tests added as well.
It might be better to just offer the choice of UTC or the system timezone set via control panel.
That's pretty much how it works: if ApplyTZ connection string is set, the time zone will be read from TZ, if defined and the system's, if it isn't. if ApplyTZ is left to default false value, the UTC times are used.
There was a problem hiding this comment.
Some references from the ML C++ code are:
and:
Those comments relate to the C-runtime libraries that shipped with Visual Studio 2010 and Visual Studio 2013. (Remember that before Visual Studio 2015 each Visual Studio had a completely separate C-runtime library. Starting in Visual Studio 2015 half the C-runtime library is part of Windows - the "universal" C-runtime, or UCRT for short - and the other half is still part of the Visual Studio distribution.) It is conceivable that those comments might now be out-of-date for the UCRT. So it might all work perfectly in Visual Studio 2017. We haven't revisited this after upgrading to Visual Studio 2017 for the ML C++ code because timezone conversions are done in Java for ML, so that code is only used in tests.
There is an independent report of the same limitations in one Microsoft C-runtime library's handling of the TZ environment variable in https://caml.inria.fr/mantis/view.php?id=7489
Java also completely reimplemented the timezone database rather than relying on the OS implementation being usable. Again, however, this may be due to the situation many years in the past and the current situation may be better.
Therefore I would recommend doing more testing of what happens when TZ is set and the times you're dealing with are on a day where the daylight savings rules are different between the US and other parts of the world. For example, I believe that in the Germany summer time ends on the last Sunday in October, but in the US it's the first Sunday in November. So try testing with TZ=Europe/Berlin and a midweek day in between these two Sundays. If you find that doesn't work then it's probably best to explicitly check that TZ is not set. But also before 2007 US daylight savings ended on the last Sunday in October, so that could mask the difference and it might be worth testing with locations in Australia that are completely different.
Even if that test does work I would err towards not documenting anything about use of TZ. The docs can just talk about ApplyTZ using local time but leave TZ as an internal secret for people who look at the source code to use at their own risk.
Finally, I think it would be worth talking through the name of the setting with product management if you haven't already. Given the difficulties of relying on environment variables for ordinary Windows users, maybe UseLocalTime would be a better name. Or even flip it around and call the option UseUTC. Do most other ODBC drivers default to UTC or local time?
There was a problem hiding this comment.
Thanks for the explanations and links.
The comments in the Inria link are still valid (and in sync with my experience).
My initial intention was to add support for local time - since SQL/ES API translates any DATE (or DATETIME in SQL translation) to UTC - and along with that the TZ comes for free and must be dealt with one way or the other (i.e. either allow use of it, or fail if set).
TBH I don't expect that typical Windows users of the driver would make use of the TZ (or environment variables, in general), but for the few others:
- I think it would be preferable to expose this functionality, than not; in support of that, I'd expect that the environment variable won't be set by mistake; and even if it is, the driver logs it (i.e. if it comes down to an issue report, this can be quickly identified).
- I'd expect to know what they are doing; most specifically, its format; in that respect
TZ=Europe/Berlinwill not work, but true maybe worst, will not have the expected result and a validation of its value is not trivial.
Even if that test does work I would err towards not documenting anything about use of TZ.
Not documenting it would seem reasonable, though and I'll keep it away (at least until there would a specifically asked for feature).
it would be worth talking through the name of the setting
My intention is to extend the GUI in a separate PR and expose a setting to allow times conversions to UTC only to local time only. And specifically not have a drop-down with timezones to choose from (since that would require setting a permanent user/system-wide configuration with the env var, which is disruptive). The connection string settings would normally not be used by end-users.
Do most other ODBC drivers default to UTC or local time?
I haven't done an analysis, TBH. But DBs don't typically impose a timezone or time normalisation (like ES/SQL does).
There was a problem hiding this comment.
My intention is to extend the GUI in a separate PR and expose a setting to allow times conversions to UTC only
The default for ApplyTZ is no, so UTC is the default now. I guess it has to be like this for BWC, as UTC has been the only way up to now.
When this is added to the UI would it be better to have the option the same way in the UI as the connection string? So UI option is "Use local time?" and connection string option can stay as ApplyTZ. Or else UI option is "Use UTC?" and connection string option is UseUTC. It seems like it's creating complexity to have the options opposite ways around.
There was a problem hiding this comment.
The default for ApplyTZ is no, so UTC is the default now. I guess it has to be like this for BWC, as UTC has been the only way up to now.
Correct.
When this is added to the UI would it be better to have the option the same way in the UI as the connection string? So UI option is "Use local time?" and connection string option can stay as ApplyTZ.
Yes, indeed. I see Use local time? option (no by default) as the better choice since that's more in line with the rest of ES/SQL (which would serve UTC over CLI/Kibana/JDBC/Rest) so one would need to specifically "ask" for a change to get local times.
- _WIN64 -> !_USE_32BIT_TIME_T
I realise the above probably created confusion: the intention was to write |
* run integration tests with custom driver install - add CLI option to take a connection string and use that to connect to a custom driver installation. * add support for timestamps in local timezone With this commit the driver will be able to use local timestamps towards the application and do the translation between that and ES/SQL (UTC) A connection string parameter controlls if this behavior is enabled; it is disabled by default. The timezone is read from the TZ environment variable, or the system if that isn't set. The driver will now also provide the SQL format of date/timestamp when a DATE/DATETIME value is converted to string. * add the integration tests for the timezone support - the commit also corrects some of the existing tests to respect the column size and/or decimal digits. * fix define conditionals for mktime error message - _WIN64 -> !_USE_32BIT_TIME_T (cherry picked from commit 6eda98f)
bpintea
added
>feature
With this PR the driver will be able to use local timestamps towards
the application and do the translation between that and ES/SQL (UTC).
A connection string parameter controlls if this behavior is enabled; it
is disabled by default.
The timezone is read from the TZ environment variable, or the system if
that isn't set.
The driver will now also provide the SQL format of date/timestamp when a
DATE/DATETIME value is converted to string.