Add fully fledged time zones support

[slvrtrn]

Despite ClickHouse itself featuring great time zone(s) support, these two driver features were never enabled:

:set-timezone
:convert-timezone

There was a draft attempt from me recently, with this snippet commented out and a few other parts missing:

metabase-clickhouse-driver/src/metabase/driver/clickhouse_qp.clj

Lines 320 to 327 in b199284

	;; FIXME: there are still many failing tests that prevent us from turning this feature on
	;; (defmethod sql.qp/->honeysql [:clickhouse :convert-timezone]
	;; [driver [_ arg target-timezone source-timezone]]
	;; (let [expr (sql.qp/->honeysql driver (cond-> arg (string? arg) u.date/parse))
	;; with-tz-info? (h2x/is-of-type? expr #"(?:nullable\(|lowcardinality\()?(datetime64\(\d, {0,1}'.*|datetime\(.*)")
	;; _ (sql.u/validate-convert-timezone-args with-tz-info? target-timezone source-timezone)
	;; inner (if (not with-tz-info?) [:'toTimeZone expr source-timezone] expr)]
	;; [:'toTimeZone inner target-timezone]))

However, a significant amount of Metabase timezone-related tests fail, most of the time because of hardcoded branches for specific drivers.

Examples of such tests:

• https://github.com/metabase/metabase/blob/master/test/metabase/query_processor_test/date_time_zone_functions_test.clj#L273-L304
• https://github.com/metabase/metabase/blob/master/test/metabase/query_processor_test/date_time_zone_functions_test.clj#L190-L198
• https://github.com/metabase/metabase/blob/master/test/metabase/query_processor_test/date_time_zone_functions_test.clj#L705-L722

Related Metabase issue to remove the hardcoded parts: metabase/metabase#26807

[mattdavenport]

Any updates on this one? I don't have enough clojure knowledge to pitch in unfortunately. I'm running into this issue where I have:

Clickhouse - UTC
Metabase - US/Eastern
JVM - US/Eastern

I have a value when queried in Clickhouse (UTC) is: 2020-03-22 22:08:28
I can do a SELECT toTimezone(created_at, 'US/Eastern') in Clickhouse to return: 2020-03-22 18:08:28

However in Metabase, when I select this same record, I'm showing the UTC time (March 22, 2020 10:08PM). I believe this is a related issue, but also wanting to check and see if there is another setting somewhere I'm missing.

Thanks!

[slvrtrn]

Hi @mattdavenport, no updates yet. However, as you are interested in this feature, we might reconsider our priorities.

Let me explain why this was postponed in more detail.

The driver runs the entire set of Metabase tests (5000+ tests, 39000+ assertions as of 0.49.x), aiming for 100% green CI run there, as it tests most of the things in and out, allowing us to mostly rely on these, writing specific tests for the driver only when it is necessary (this is still quite a lot, you can see it in the sources).

However, even though I think I have a working driver code for the fully-fledged timezones support (I briefly checked it from the UI, and it seemed to be working), there are a lot of timezone tests with hardcoded assertions for certain built-in drivers. Sometimes, the ClickHouse output, while being correct, is slightly different from the assertion there (it could be as minor as a few extra zeroes after the dot in the ISO formatted string or whatever else), and this causes the entire test to fail. It is also impossible to fix all of them, as one workaround could cause another test to fail (with another minor "cosmetic" discrepancy), and so on.

There are dozens of such test cases (you can see the code examples links in the OP), and we can disable them, but it will require rewriting comparable tests adjusted to the driver in this repo. This is very time-consuming, and that was the main reason why it was postponed. Additionally, this is the exact reason why the Metabase timezones feature was always disabled in this driver. Here's a link to the Metabase issue about these hardcoded tests.

Regarding your situation - yes, this is certainly a related issue. Unless everything (JVM, CH, Metabase) is forced into the same timezone, there might be discrepancies, just like the one you mentioned.

If it is impossible to switch to the same timezone everywhere, which will ensure proper UI results, we can think about a release where we could do the following:

• Disable Metabase timezones tests
• Write a few select test cases in the driver code covering the most dangerous parts
• Allow timezone features in the driver to be enabled via an environment variable or similar.

This has a chance of working properly; however, we cannot consider it stable until it 100% passes the entire Metabase test set.

Please let me know what you think.

CC @mshustov (we discussed that only recently).

[mattdavenport]

@slvrtrn Thanks for the detailed explanation! That makes a lot of sense. Perhaps Allow timezone features in the driver to be enabled via an environment variable or similar. could be a way to enable this "experimental" behavior that is documented unstable but usable at the moment. This could open up the new functionality without effectively changing the core behavior, but would likely need to remain untested until the referenced Metabase issue is resolved. It looks like there is some relatively recent activity on that ticket, so it may also be best to wait if your nudge to increase priority has any effect.

Thanks again for all of the work you all put in here and on Clickhouse. Very impressive stuff!

[adityapatadia]

This is a problem for us in production. Can we get this prioritised?

[dimitriosp]

Not sure if this is related, however when I try to set the REPORT TIMEZONE in metabase-cloud, it does not show the correct timezone. For the postgres(supabase) driver, this is working.
If not related, i will create a new ticket.

[slvrtrn]

It is related.

[dimitriosp]

It is related.

Is there any other solution for metabase-cloud? since we cant host it, we can't change the parameter in docker.

[slvrtrn]

Is there any other solution for metabase-cloud?

I'd ask Metabase support for that; maybe they can adjust the TZ for your particular instance.

[dimitriosp]

I'd ask Metabase support for that; maybe they can adjust the TZ for your particular instance.

I went back and forth with the support and there seems no other option, then adjusting the sql query to reflect the timezone, since it is not officially supported. see here