-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
ARROW-622: [Python] Add coerce_timestamps option to parquet.write_table, deprecate timestamps_to_ms argument #944
Conversation
Change-Id: I40e544677a78f0cd335b3c2a9aae4ace5c8def08
Change-Id: Id4fb68628b7f47a559907e1ac74e8f0241d0125f
Triggered build now that PARQUET-1078 is merged |
Travis CI build passed except for an idiosyncratic failure from the Travis CI cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things, otherwise looks good.
elif self.coerce_timestamps == 'us': | ||
props.coerce_timestamps(TimeUnit_MICRO) | ||
elif self.coerce_timestamps is not None: | ||
raise ValueError('Invalid value for coerce_timestamps: {0}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will bubble up unless you return an integer and set except <integer_error_value>
in the signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. if you have int return value and except -1
then I believe cython handles automatically returning -1 when you raise an exception
python/pyarrow/pandas_compat.py
Outdated
@@ -250,6 +250,10 @@ def maybe_coerce_datetime64(values, dtype, type_, timestamps_to_ms=False): | |||
|
|||
coerce_ms = timestamps_to_ms and values.dtype != 'datetime64[ms]' | |||
|
|||
if timestamps_to_ms: | |||
import warnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably just be at the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Change-Id: Iaccf8df44d30c9275d6d3c952a5b72dbcb943722
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
xref pandas-dev/pandas#17438 this was not fully resolved in apache#944 Author: Jeff Reback <[email protected]> Closes apache#1046 from jreback/warn and squashes the following commits: 382592f [Jeff Reback] deprecate timestamps_to_ms in .from_pandas()
Requires PARQUET-1078 apache/parquet-cpp#380
cc @xhochy @fjetter @cpcloud, could you have a look. This needs to go into 0.6.0