-
Notifications
You must be signed in to change notification settings - Fork 39
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
Better support for parsing date-related data types from ClickHouse #6863
Conversation
- Support parsing timezones out of `DateTime` and `DateTime64` types, and add support for `Date` type. - Switch to using `nom` for more robust parsing of these types. - Fix bug in block concatenation, where we concatenated rows but didn't update the contained row count. Add regression test. - Add a new USDT probe for data packets specifically, with size and column names / types
oximeter/db/src/native/io/column.rs
Outdated
dst.reserve(values.len() * std::mem::size_of::<u16>()); | ||
for value in values { | ||
let days = value.signed_duration_since(EPOCH).num_days(); | ||
dst.put_u16_le(u16::try_from(days).unwrap()); |
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 shouldn't unwrap here. We can do that when deserializing values from ClickHouse, because the range must have already been validated to fit in a u16
for ClickHouse to have stored it in the first place. These values are from us, and start as chrono::NaiveDate
s, which have a wider range than ClickHouse supports. We need to check that and fail this call if they're out of range.
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.
Made this and the related pieces fallible in ff0af17
Sorry to request a review and then push more commits. This is indeed ready for a review! |
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.
I have no experience with nom, but lgtm.
DateTime
andDateTime64
types, and add support forDate
type.nom
for more robust parsing of these types.