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

minute and second temporal kernels do not respect timezone #500

Closed
alamb opened this issue Jun 24, 2021 · 4 comments · Fixed by #771 or #824
Closed

minute and second temporal kernels do not respect timezone #500

alamb opened this issue Jun 24, 2021 · 4 comments · Fixed by #771 or #824
Labels

Comments

@alamb
Copy link
Contributor

alamb commented Jun 24, 2021

Describe the bug
As pointed out by @jorgecarleitao https://github.com/apache/arrow-rs/pull/493/files#r656987963 the minute and second temporal kernels do not respect timezone

To Reproduce
Extract seconds from a timestamp column that has an associated timezone (aka is not None or Utc) will always be interpreted as a timestamp in UTC rather than the specified timezone.

Expected behavior
Seconds to be interpreted in terms of the the timezone or raise an error about not being supported

@sum12
Copy link
Contributor

sum12 commented Sep 8, 2021

I would like to work on this.

split in two parts

  • if tz is of the form [+-][0-9]{2}:[0-9]{2} then construct a chrono::offset::FixedOffset object from the provided tz string and add that at offset to the NaiveDateTime (NaiveDateTIme implements Add<FixedOffset>)
  • else if it is a string which cannot be parsed as above we use th chrono-tz crate to generate an offset

sounds okay ?

@jorgecarleitao
Copy link
Member

makes sense to me. I have done this recently in arrow2 around here

@alamb
Copy link
Contributor Author

alamb commented Sep 9, 2021

@sum12 this also sounds reasonable to me.

As you have probably seen the spec for what can be in the timezone string is here: https://github.com/apache/arrow/blob/master/format/Schema.fbs#L341-L351 and I believe your proposal in #500 (comment) is consistent with that

Regarding chrono-tz, please make that an optional dependency (via feature flag) of arrow-rs -- we have many users who use arrow in a variety of ways that are sensitive to additional dependencies.

@sum12
Copy link
Contributor

sum12 commented Sep 13, 2021

Thanks for the feedback.

@alamb, created a PR for now, does not really follow the proposal. I have tried to reuse the parsing capabilities of the chrono itself to parse the timezone. Not sure if it is okay to go down that route. Although happy to back to proposal if the PR is not going in the correct direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants