-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: add special datatype for timestamp #730
feat: add special datatype for timestamp #730
Conversation
from kolena._utils.pydantic_v1.dataclasses import dataclass | ||
from kolena._utils.validators import ValidatorConfig | ||
|
||
os.putenv("TZ", "GMT") |
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.
in my understanding we only need this tz for line 80, could we set it at the func calling side like from datetime import timezone; datetime.strptime(self.value, self.format).replace(tzinfo=timezone.utc).timestamp()
for better locality?
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.
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.
Let me know what you think of this commit. I agree that manipulating os
is bad practice and at the same time did not want to introduce new dependencies for this alone. Looking at the python doc:
Changed in version 3.2: When the %z directive is provided to the strptime() method, an aware datetime object will be produced. The tzinfo of the result will be set to a timezone instance.
The change is to simply check for presence of %z
and append it to the string if not exists
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.
thanks for the detailed explanation, i like the changes
if not self.format: | ||
raise ValueError("format needs to be specified for string timestamp") |
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.
This feels a bit strict -- was there a reason we didn't do a best guess attempt at parsing with python-dateutil
+ just default to a display 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.
Agreed that this can be one of the next steps to improve the experience, and our focus so far has been to support timestamp across the platform. Before using something like python-dateutil
we'll want to validate across different use cases and make sure that it results in the correct epoch time.
This reverts commit d4504d2.
Linked issue(s)
Towards KOL-3547. Backend and frontend changes are WIP.
What change does this PR introduce and why?
Create a new data type class for timestamp, as
SPECIAL/TIMESTAMP
Users can either create a timestamp object using a float epoch time (
Timestamp(epoch_time=1735689600)
), or a string along with its format (Timestamp(value="12/31/2024, 00:00:00", format="%m/%d/%Y, %H:%M:%S"
). For the latter, the Kolena client would convert the timestamp into the epoch float time in order to be used to filter/sort on Kolena. Unless an offset such asGMT-05:00
is specified, these timestamps are assumed to be in GMT timezone.For now, leave the special data classes under
experimental
. Screenshot of the generated API reference is attached below:Please check if the PR fulfills these requirements