Skip to content
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

Merged

Conversation

nankolena
Copy link
Contributor

@nankolena nankolena commented Dec 10, 2024

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 as GMT-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:

image

Please check if the PR fulfills these requirements

  • Include reference to internal ticket and/or GitHub issue "Fixes #NNNN" (if applicable)
  • Relevant tests for the changes have been added
  • Relevant docs have been added / updated

@nankolena nankolena marked this pull request as ready for review December 11, 2024 17:53
@nankolena nankolena requested a review from a team as a code owner December 11, 2024 17:53
from kolena._utils.pydantic_v1.dataclasses import dataclass
from kolena._utils.validators import ValidatorConfig

os.putenv("TZ", "GMT")
Copy link
Contributor

@yifanwu-kolena yifanwu-kolena Dec 16, 2024

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that was my initial approach, but it turned out that .replace() will strip out the timezone info in the input string and lead to incorrect epoch time. In the following, this call basically just ignores the input timezone:

image image

Copy link
Contributor Author

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

Copy link
Contributor

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

@nankolena nankolena merged commit d4504d2 into trunk Dec 16, 2024
57 checks passed
@nankolena nankolena deleted the nan/kol-3547-sdk-handle-timestamp-classes-in-json-serde branch December 16, 2024 21:06
Comment on lines +72 to +73
if not self.format:
raise ValueError("format needs to be specified for string timestamp")
Copy link
Contributor

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?

Copy link
Contributor Author

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.

nankolena added a commit that referenced this pull request Dec 17, 2024
nankolena added a commit that referenced this pull request Dec 17, 2024
nankolena added a commit that referenced this pull request Dec 17, 2024
munkyshi pushed a commit that referenced this pull request Jan 7, 2025
* Revert "Revert "feat: add special datatype for timestamp (#730)" (#732)"

This reverts commit 5e3863c.

* support iso format by default, refactor tz localization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants