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

refactor(api): remove casting from timestamp to integer and integer to timestamp #9854

Closed
cpcloud opened this issue Aug 16, 2024 · 5 comments
Assignees

Comments

@cpcloud
Copy link
Member

cpcloud commented Aug 16, 2024

These two operations differ in our backends and IMO are confusing foot guns that make it hard to write portable Ibis code due to differing support for different units across backends. It also doesn't round trip, even within the same backend.

For the timestamp -> integer case, we can make an epoch(unit) API on TimestampValue.

For the integer -> timestamp case, we have the IntegerValue.to_timestamp method which allows specifying units. For that case I think we should also make the units required and not default to seconds.

@ncclementi
Copy link
Contributor

ncclementi commented Aug 20, 2024

For the timestamp -> integer case, we can make an epoch(unit) API on TimestampValue.

@jcrist you had some comments/opions about this part while we were on triage.

For the integer -> timestamp case, we have the IntegerValue.to_timestamp method which allows specifying units. For that case I think we should also make the units required and not default to seconds.

We can do this easily in the new IntergerValue.as_timestamp() API that we just changed. (PR coming up).

@cpcloud
Copy link
Member Author

cpcloud commented Aug 20, 2024

I think you mean Integer value in the last bit. IntervalValue.as_timestamp() isn't a meaningful operation in general.

@ncclementi
Copy link
Contributor

Yup, you are right!

@ncclementi
Copy link
Contributor

@jcrist I lost track of the other PR that was tackling part one, and I can't seem to find it, can you reference it here and close this one if it has been merged.

@jcrist
Copy link
Member

jcrist commented Aug 26, 2024

For the timestamp -> integer case, we can make an epoch(unit) API on TimestampValue.

This was resolved by #9856

@jcrist jcrist closed this as completed Aug 26, 2024
@github-project-automation github-project-automation bot moved this from backlog to done in Ibis planning and roadmap Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants