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 implementation for load_with_datetime in Python package. #411

Merged
merged 7 commits into from
Aug 27, 2021

Conversation

zijie0
Copy link
Contributor

@zijie0 zijie0 commented Aug 24, 2021

Description

We would like to use load_with_datetime method implemented in Rust binding in our Python project. Add the implementation in Python package.

@zijie0
Copy link
Contributor Author

zijie0 commented Aug 24, 2021

Hmm... the test failed. Let me check it later.

@zijie0 zijie0 marked this pull request as draft August 24, 2021 15:24
@zijie0
Copy link
Contributor Author

zijie0 commented Aug 25, 2021

It's weird, the tests passed on my local machine:

In [1]: %load test.py

In [2]: # %load test.py
   ...: from deltalake import DeltaTable
   ...:
   ...:
   ...: table_path = "./rust/tests/data/simple_table"
   ...: dt = DeltaTable(table_path)
   ...: dt.load_with_datetime("2020-05-01T00:47:31-07:00")
   ...: assert dt.version() == 0
   ...: dt.load_with_datetime("2020-05-02T22:47:31-07:00")
   ...: assert dt.version() == 1
   ...: dt.load_with_datetime("2020-05-25T22:47:31-07:00")
   ...: assert dt.version() == 4
   ...:
   ...:

In [3]: dt.version()
Out[3]: 4

In [4]: dt.load_with_datetime("2020-05-04T22:47:31-07:00")

In [5]: dt.version()
Out[5]: 3

I'm not sure why it's failing in CI. Could @fvaleye @houqp take a look please?

@zijie0 zijie0 marked this pull request as ready for review August 25, 2021 06:31
@zijie0
Copy link
Contributor Author

zijie0 commented Aug 25, 2021

Learned from time_travel_by_ds test case, we should set delta log mtime before running the test. Problem solved.

python/deltalake/table.py Outdated Show resolved Hide resolved
python/deltalake/table.py Outdated Show resolved Hide resolved
python/deltalake/table.py Outdated Show resolved Hide resolved
@fvaleye fvaleye added the binding/python Issues for the Python package label Aug 26, 2021
@fvaleye
Copy link
Collaborator

fvaleye commented Aug 26, 2021

@zijie0 Thank you for your PR, I added some comments!

@zijie0
Copy link
Contributor Author

zijie0 commented Aug 27, 2021

Thanks @fvaleye ! I will go through all the comments today.

Add tests for bad datetime string format.
@zijie0
Copy link
Contributor Author

zijie0 commented Aug 27, 2021

Hi @fvaleye , when I run make install it seems that the old version of deltalake is not removed and pip will give some message like: deltalake is already installed with the same version as the provided wheel. Use --force-reinstall to force an installation of the wheel.

So I added an uninstall step in Makefile, is that ok?

@houqp
Copy link
Member

houqp commented Aug 27, 2021

I think it makes sense to add a new uninstall makefile target. We could also update the install target to use --force-reinstall by default if that helps. Once PyO3/maturin#604 is resolved, we should probably switch to maturin develop for the install target so we won't need to manually pick up code change for local development.

houqp
houqp previously approved these changes Aug 27, 2021
Copy link
Member

@houqp houqp left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you @zijie0 !

python/Makefile Show resolved Hide resolved
@houqp houqp requested a review from fvaleye August 27, 2021 03:23
python/deltalake/table.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@fvaleye fvaleye left a comment

Choose a reason for hiding this comment

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

Perfect👍
Thanks @zijie0

@fvaleye fvaleye merged commit db5c23e into delta-io:main Aug 27, 2021
fvaleye pushed a commit to fvaleye/delta-rs that referenced this pull request Aug 28, 2021
…-io#411)

* Add implementation for `load_with_datetime` in Python package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/python Issues for the Python package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants