-
Notifications
You must be signed in to change notification settings - Fork 53
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: Allow partition value timestamp to be ISO8601 formatted string #622
fix: Allow partition value timestamp to be ISO8601 formatted string #622
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #622 +/- ##
==========================================
+ Coverage 83.45% 83.49% +0.04%
==========================================
Files 74 74
Lines 16877 16919 +42
Branches 16877 16919 +42
==========================================
+ Hits 14084 14127 +43
Misses 2135 2135
+ Partials 658 657 -1 ☔ View full report in Codecov by Sentry. |
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.
Approach LGTM!
@zachschuermann The integration test creates a directory |
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.
lgtm with one nit, thanks!
let mut timestamp = NaiveDateTime::parse_from_str(raw, "%Y-%m-%d %H:%M:%S%.f"); | ||
|
||
if timestamp.is_err() && *self == Timestamp { | ||
timestamp = NaiveDateTime::parse_from_str(raw, "%+"); |
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.
Can you add a comment that %+
means "ISO 8601 / RFC 3339 format"
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.
lgtm!
What changes are proposed in this pull request?
The delta protocol states that partition value timestamps may be encoded as ISO 8601 formatted strings in addition to the existing format of
{year}-{month}-{day} {hour}:{minute}:{second}
. Previously, we did not cover the ISO8601 case.This PR allows kernel to read tables to be partitioned on timestamps with this format.
How was this change tested?
New unit tests are introduces to check the following:
{year}-{month}-{day} {hour}:{minute}:{second}
with and without microsecondsA new integration test is also introduced to check a table with timestamp partition values is correctly read.