-
Notifications
You must be signed in to change notification settings - Fork 102
lightning: fix parquet parser for decimal type #1272
Conversation
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.
rest LGTM
if dotIndex == 0 { | ||
res.WriteByte('0') | ||
} else { | ||
res.Write([]byte(val[:dotIndex])) |
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.
res.Write([]byte(val[:dotIndex])) | |
res.WriteString(val[:dotIndex]) |
} | ||
if scale > 0 { | ||
res.WriteByte('.') | ||
res.Write([]byte(val[dotIndex:])) |
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.
res.Write([]byte(val[dotIndex:])) | |
res.WriteString(val[dotIndex:]) |
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.
rest LGTM
sec = v / 1e9 | ||
nsec = v % 1e9 | ||
} | ||
// TODO: how to deal with TimeZone |
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.
Is there more we need to do about time zones?
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.
The time/timestamp value read from parquet is either UTC or Local timezone (based on the IsAdjustedToUTC
setting). Since currently lightning don't support set timezone (always set to target cluster's timezone). Not sure how should we deal with the unknown timezone offset if IsAdjustedToUTC = false
(at least the default is true). @kennytm PTAL
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.
Make the produced string 2006-01-02 15:04:05.999999Z
when isAdjustedToUTC is true, and 2006-01-02 15:04:05.999999
when it is false.
(why do we only support 3 fractional digits instead of 6?)
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.
done
@glorv It's better file an issue to record this bug. |
@gozssky: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: f871acc
|
In response to a cherrypick label: new pull request created: #1275. |
In response to a cherrypick label: new pull request created: #1276. |
In response to a cherrypick label: new pull request created: #1277. |
What problem does this PR solve?
Fix parse parquet file when the corresponding type is decimal with
FIXED_LEN_BYTE_ARRAY
orBINARY
What is changed and how it works?
Check List
Tests
Code changes
Side effects
Related changes
Release note