-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add utility to create kdb types #10
Conversation
yznima
commented
Dec 1, 2018
•
edited
Loading
edited
- Added kdb builders for most of the missing types.
- Instead of Time being a type, it is now a function to be uniform with other types
@sv Ready for review |
arr := arr.([]int32) | ||
var timearr = make([]Time, veclen) | ||
for i := 0; i < int(veclen); i++ { | ||
timearr[i] = Time(qEpoch.Add(time.Duration(arr[i]) * time.Millisecond)) |
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.
Since time is just the time since the beginning of the day, there is no need to add the epoch. 10:30AM 1970 is equivalent to 10:30AM 2000. Since most systems out there have epoch 1970, I suggest we just return the time since 1970. Thoughts @sv ?
// Time hh:mm:ss.SSS | ||
type Time time.Time | ||
|
||
func (t Time) String() string { |
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.
time has specific formatting requirement. Did you find it not useful?
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 actually did. However, I wanted to introduce the Time function that returns the K object and is consistent with all other types. I think it would be simple to implement the String logic on the K object uniformly for all types
@sv addressed comments |
@sv ready for review |
@sv I was wondering if you are still considering this PR. |
@yznima yes. I will merge some version of this to v1beta. Need to be careful with how time types handled - i.e. x.UTC() might not necessarily mean correct kdb time? |