ClickHouse / metabase-clickhouse-driver

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

current week returns result, but "past month" returns nothing #164

Closed taylorchu closed 1 year ago

taylorchu commented 1 year ago

Describe the bug

current week returns result, but "past 1 month" returns nothing. it seems the result is inconsistent across timestamp filter.

Steps to reproduce

Screenshot 2023-05-19 at 6 39 41 PM Screenshot 2023-05-19 at 6 38 42 PM
  1. choose "timestamp" to filter to "this" week. it shows N rows
  2. choose "timestamp" to filter last N months including this week shows no rows

Expected behaviour

filtering by time works.

Error log

Configuration

Environment

ClickHouse server

taylorchu commented 1 year ago

also

  1. current week works but current months return nothing
  2. it seems to correlate to time units. minute, hour, and week work, but day, month, quarter and year do not work.
taylorchu commented 1 year ago

@slvrtrn could you take a look? it seems like you have a pr, but not sure if it is related.

slvrtrn commented 1 year ago

@taylorchu, I will have a look once I am back (next week).

slvrtrn commented 1 year ago

@taylorchu, I checked it with 1.1.6, and it seems to be working as expected.

image

Note that there is an option to include the current month in the result set, and it generates two different queries based on that:

Without "include this month" (May will be excluded):

SELECT `default`.`past_test`.`i` AS `i`, `default`.`past_test`.`when` AS `when` 
FROM `default`.`past_test` 
WHERE (`default`.`past_test`.`when` >= toStartOfMonth(toDateTime((CAST(now() AS timestamp) + INTERVAL -1 month))) 
AND `default`.`past_test`.`when` < toStartOfMonth(toDateTime(now()))) 
LIMIT 2000

With "include this month" (May will be included):

SELECT `default`.`past_test`.`i` AS `i`, `default`.`past_test`.`when` AS `when` 
FROM `default`.`past_test` 
WHERE (`default`.`past_test`.`when` >= toStartOfMonth(toDateTime((CAST(now() AS timestamp) + INTERVAL -1 month))) 
AND `default`.`past_test`.`when` < toStartOfMonth(toDateTime((CAST(now() AS timestamp) + INTERVAL 1 month)))) 
LIMIT 2000

^ see the difference in the upper bound

`when` < toStartOfMonth(toDateTime(now()))

-- less than the 1st of May

vs

`when` < toStartOfMonth(toDateTime((CAST(now() AS timestamp) + INTERVAL 1 month)))

-- less than the 1st of June

My dataset was defined like the following:

CREATE TABLE past_test (i UInt64, when DATETIME) 
ENGINE MergeTree 
ORDER BY i;

INSERT INTO past_test 
SELECT number, now() - toIntervalMinute(randCanonical() * 1000000) 
FROM system.numbers LIMIT 100000;

if the issue is still reproducible on your end using 1.1.6, even with "include this month" enabled, can you please share the minimal dataset that can help me investigate this issue?

taylorchu commented 1 year ago

yes. this is still reproduced in 1.1.6.

My timestamp column is timestamp DateTime64(3).

CREATE DATABASE IF NOT EXISTS app;
CREATE TABLE IF NOT EXISTS app.logs
(
  `timestamp` DateTime64(3),
  `host` LowCardinality(String),
  `namespace_name` LowCardinality(String),
  `pod_name` LowCardinality(String),
  `container_name` LowCardinality(String),
  `stream` LowCardinality(String),
  `labels_app` LowCardinality(String),
  `labels_role` LowCardinality(String),
  `message` String
)
ENGINE = MergeTree
PARTITION BY toStartOfHour(timestamp)
ORDER BY (labels_app, pod_name, container_name, timestamp)
TTL toDateTime(timestamp) + INTERVAL 31 DAY;
slvrtrn commented 1 year ago

@taylorchu, after some investigation, it turned out this is due to the issue related to ClickHouse itself: https://github.com/ClickHouse/ClickHouse/issues/50353

There is a workaround available, I will check if it's possible to squeeze it into the :month method (that's where the toStartOfMonth call is coming from), cause it's the only one for all the Date* types.

slvrtrn commented 1 year ago

@taylorchu, there is a PR that fixes this odd behavior on the CH server side: https://github.com/ClickHouse/ClickHouse/pull/50280.

taylorchu commented 1 year ago

Thanks! it seems like a critical bug.

taylorchu commented 1 year ago

After updating to 23.5, "quarter" and "year" still do not work, but the others do.

taylorchu commented 1 year ago

Actually that is because I did not include "current quarter and year". This is now fixed.