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

Add support for Time64 #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

remilapeyre
Copy link

Signed-off-by: Rémi Lapeyre [email protected]

Signed-off-by: Rémi Lapeyre <[email protected]>
@zilder
Copy link
Contributor

zilder commented Jul 23, 2020

Hi @remilapeyre,

thanks for your contribution! At first glance looks good. I'll do a more thorough review soon and will merge it then.

@zilder zilder self-requested a review July 23, 2020 09:39
@remilapeyre
Copy link
Author

Thanks @zilder, I tried to look carefully to find whether there was something special to do to handle timezones and time resolution (in the Parquet file or the PostgreSQL database) and after reading PostgreSQL source code I don't think it needs special treatment but I'm not sure about that.

@zilder
Copy link
Contributor

zilder commented Aug 3, 2020

Hi @remilapeyre,

As i understood parquet's time and timestamp types do not contain information on the timezone. They have isAdjustedToUTC boolean parameter, but without actual timezone information there is not much can be done about it.

One thing that wasn't taken into account in this pull request is time precision. Proposed patch implies that time is in microseconds. Though time64 type may also represent nanoseconds. Other than that the patch looks good.

@remilapeyre
Copy link
Author

Hi thanks for the review, I had not the time to look at the nanoseconds yet but I will try to do it in the coming days.

@mkgrgis
Copy link

mkgrgis commented Jan 21, 2023

@remilapeyre , any activity here?

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