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

Handle pandas timestamps #958

Merged
merged 15 commits into from
Dec 1, 2022
Merged

Conversation

hayesgb
Copy link
Contributor

@hayesgb hayesgb commented Nov 26, 2022

Description

As described in #686 some pandas datatypes are not converted to a format that is compatible with delta lake. This handles the instance of timestamps, which are stored with ns resolution in Pandas. Here, if is a schema is not provided, we specify converting the timestamps to us resolution.

We also update python/tests/test_writer.py::test_write_pandas to reflect this change.

Related Issue(s)

#685

Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Have two requests on it.

python/deltalake/writer.py Outdated Show resolved Hide resolved
Comment on lines 140 to 143
_data = pa.Table.from_pandas(data)
_schema = _data.schema
schema_out = []
for _field in _schema:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you be willing to pull this out into a helper function (_delta_arrow_schema_from_pandas)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really like this idea. Factored this out into a helper function, but I'd prefer it to be public rather than private. Would like to leverage this helper function elsewhere. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but if you make it public, then I'd suggest putting it in schema.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@hayesgb
Copy link
Contributor Author

hayesgb commented Nov 27, 2022

Not sure why mypy is complaining.

@houqp
Copy link
Member

houqp commented Nov 27, 2022

@hayesgb you might need to add timestamp to https://github.com/delta-io/delta-rs/blob/main/python/stubs/pandas/__init__.pyi in order to fix the mypy error

python/deltalake/schema.py Outdated Show resolved Hide resolved
python/deltalake/schema.py Outdated Show resolved Hide resolved
python/deltalake/schema.py Outdated Show resolved Hide resolved
hayesgb and others added 3 commits November 30, 2022 08:34
Co-authored-by: Will Jones <willjones127@gmail.com>
Co-authored-by: Will Jones <willjones127@gmail.com>
Co-authored-by: Will Jones <willjones127@gmail.com>
hayesgb and others added 2 commits November 30, 2022 18:50
Co-authored-by: Will Jones <willjones127@gmail.com>
Co-authored-by: Will Jones <willjones127@gmail.com>
python/deltalake/schema.py Outdated Show resolved Hide resolved
python/deltalake/schema.py Outdated Show resolved Hide resolved
@wjones127 wjones127 enabled auto-merge (squash) December 1, 2022 03:40
Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

The CI test is being flaky. I tested locally tested and all tests pass.

Thank you @hayesgb!

@wjones127 wjones127 merged commit 4efc67c into delta-io:main Dec 1, 2022
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