ClickHouse / metabase-clickhouse-driver

ClickHouse database driver for the Metabase business intelligence front-end
Apache License 2.0
476 stars 92 forks source link

Add fully fledged time zones support #200

Open slvrtrn opened 11 months ago

slvrtrn commented 11 months ago

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: https://github.com/ClickHouse/metabase-clickhouse-driver/blob/b19928448adc5961bd37a8080b271f57756f67e1/src/metabase/driver/clickhouse_qp.clj#L320-L327

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:

Related Metabase issue to remove the hardcoded parts: https://github.com/metabase/metabase/issues/26807

mattdavenport commented 6 months ago

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 commented 6 months ago

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:

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 commented 6 months ago

@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 commented 5 months ago

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

dimitriosp commented 4 months ago

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.

image image image

slvrtrn commented 4 months ago

It is related.

dimitriosp commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

slvrtrn commented 3 months ago

1.50.1 adds support for both timezone-related features. Please try it out and create a new issue if you find any bugs.

mattdavenport commented 3 months ago

Thank you @slvrtrn!

adityapatadia commented 1 month ago

Is this version released on Cloud? We still don't see reports in correct timezone.

paoliniluis commented 1 month ago

@adityapatadia hi, please contact Metabase support

dimitriosp commented 1 month ago

1.50.1 adds support for both timezone-related features. Please try it out and create a new issue if you find any bugs.

Not sure if this is now related, however, when I use the simple sql prompt: select current_date()

i get 13.08.2024, but today is 14.08.2024

doing the same in postgres, i get the correct date

slvrtrn commented 1 month ago

The UI values are indeed incorrect despite the Metabase test suite for that functionality (general time zone support, time zone conversion, etc.) being green.

Currently, oddly, the workaround is to use data types with explicit timezones in the type definition (e.g., DateTime('UTC') instead of DateTime), similarly to the functions such as now.

SELECT now('UTC'); // or other timezone

Then, the report timezone is applied correctly.

We will be investigating how to fix it properly (and why all the tests are green while the UI values are incorrect).

adityapatadia commented 1 month ago

We have checked with the explicit timezone definition column DateTime('UTC') and it still does not show the correct date on the frontend.