ClickHouse / metabase-clickhouse-driver

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

Unnecessary CAST toDate for custom variables with time #196

Closed ADovgalyuk closed 7 months ago

ADovgalyuk commented 9 months ago

While using variables with type "Field Filter", widget type "Date Filter" error occurred due to CAST to Date type. In release notes for new driver i see "Removed superfluous CAST calls from generated queries that use Date* columns and/or intervals." which looks similar, but issue is still reproduced on last version.

Steps to reproduce

  1. New Sql Query
  2. Use variable with filter type "Date Filter"
  3. Choose one of options including smth related to Time, not only Date.
  4. Error is shown like (also presented on screenshot below):
    Code: 53. DB::Exception: Cannot convert string 2023-10-03 18:05:00 to type Date: while executing 'FUNCTION greaterOrEquals(CAST(date_time, 'date') : 4, '2023-10-03 18:05:00' : 2) -> greaterOrEquals(CAST(date_time, 'date'), '2023-10-03 18:05:00') UInt8 : 5'. (TYPE_MISMATCH) (version 22.8.16.32 (official build)) , server ClickHouseNode [uri=http://192.168.109.237:8123/statistics, options={use_server_time_zone_for_dates=true,use_no_proxy=false,product_name=metabase/1.2.2}]@-1605831072

Expected behaviour

No errors, data on specified time interval is shown

Configuration

Environment

ClickHouse server

query_preview main_window

slvrtrn commented 9 months ago

Thanks for the report.

In release notes for new driver i see "Removed superfluous CAST calls from generated queries that use Date* columns and/or intervals." which looks similar

Yes, this is correct, but for non-variable use cases. I am still to figure out where we can actually override the incorrect variables replacement.

I will look into it again early next week.

slvrtrn commented 9 months ago

It looks like this is the same bug: https://github.com/metabase/metabase/issues/21133 But, instead of losing performance like in that Postgres scenario, we cannot execute the query at all, as DateTime -> Date implicit upcast on the right side does not work. I am investigating whether it will be feasible to introduce some hack around it, as it looks like a substantial amount of copy-paste from Metabase variables substitution sources is required, as almost everything is private methods in the related module.

slvrtrn commented 9 months ago

After discussing my findings with the Metabase team, we agreed that this is blocked by Metabase currently. We'd also like to avoid a massive copy-paste, especially from the project with a different license, and I will try my best to prepare a Metabase patch that makes that particular bit more driver-override-friendly, so we can resolve that unnecessary upcast in a more canonical way. Stay tuned.

slvrtrn commented 7 months ago

@ADovgalyuk, it is fixed in 1.3.0. You could try it with Metabase 0.48.0-RC1 now.