-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Hydrolix dates 1 #835
Fix Hydrolix dates 1 #835
Conversation
bcf4099
to
233b989
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great direction?
How did you find this and concluded it is Hydrolix related? I looked into log lines and just hit this error in dev env by me and Rafal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We likely need more simple tests for this date logic.
if dateTime, ok := v.(string); ok { | ||
// if it's a date, we need to parse it to Clickhouse's DateTime format | ||
// how to check if it does not contain date math expression? | ||
if _, err := iso8601.ParseString(dateTime); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not using iso8601
, we should drop it from dependencies.
Hmm, I just knew that it always used to work fine in our new version of Clickhouse, and in Hydrolix it didn't, so I:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use int64 instead of float64. IDeally use:
fromUnixTimestamp64Milli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied my changes to use int64 instead of float64.
This eliminates an error we encountered with Hydrolix.
It's possible to compare dates and floats in Clickhouse from 2022 year, but in 2021 it wasn't possible, and Hydrolix must use some earlier version, so I created a solution which works in both scenarios
I move date parsing from Clickhouse to Quesma, removing usage of Clickhouse's
parseDateTimeBestEffort()
. Now we parse date ourselves, and usetoDateTime64()
to compare this date with date field (checked, it works fine also for fields of basicDateTime
type, notDateTime64
, even in Clickhouse 4 years ago). Elastic has like 50 different date formats available, so I doubt all of them are available in Clickhouse or other databases, so it's a move in the right direction, I guess.After:
Timestamps are fine: there's
"gte": 1727858503270
in the request, which isWed Oct 02 2024 08:41:43 GMT+0000
, so histogram starts fine from8:41:30
: