Skip to content

ARROW-2122: [Python] Pyarrow fails to serialize dataframe with timestamp. - #1707

Closed
adshieh wants to merge 3 commits into
apache:masterfrom
adshieh:ARROW-2122
Closed

ARROW-2122: [Python] Pyarrow fails to serialize dataframe with timestamp.#1707
adshieh wants to merge 3 commits into
apache:masterfrom
adshieh:ARROW-2122

Conversation

@adshieh

@adshieh adshieh commented Mar 6, 2018

Copy link
Copy Markdown
Contributor

Fixes ARROW-2122.

Comment thread python/pyarrow/types.pxi Outdated
return 'ns'


FIXED_OFFSET_PREFIX = '+'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@wesm suggestions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We probably want the offset to be encoded as [+-]HH:MM.
See https://github.com/apache/arrow/blob/master/format/Schema.fbs#L162-L166

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@wesm

wesm commented Mar 13, 2018

Copy link
Copy Markdown
Member

This needs a little more scrutiny (cc @jreback @cpcloud @pitrou) before we commit to something for 0.9.0 we might have to break later. I'm going to move this issue off 0.9.0 so we aren't blocking

Comment thread python/pyarrow/types.pxi
FIXED_OFFSET_PREFIX = '+'


def tzinfo_to_string(tz):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These two functions would deserve a docstring, and appropriate unit tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

assert pa.Array.from_pandas(expected).equals(result)


def test_fixed_offset_timezone():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please put this under the class above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me. Perhaps a pandas or timezone expert wants to comment, otherwise I think it's good to go.

Comment thread python/pyarrow/types.pxi Outdated
return 'ns'


_FIXED_OFFSET_RE = re.compile(r'([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this needs a $ at the end to avoid ignoring any trailing text.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

@xhochy xhochy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM

CI failures are unrelated.

@xhochy xhochy closed this in fff992a Apr 3, 2018
@adshieh
adshieh deleted the ARROW-2122 branch April 3, 2018 14:58
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.

4 participants