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

Python: Add adlfs support (Azure DataLake FileSystem) #6392

Merged
merged 11 commits into from
Dec 19, 2022

Conversation

cccs-eric
Copy link
Contributor

Adds support for adlfs storage in Python API.

@github-actions github-actions bot added the python label Dec 9, 2022
Copy link
Contributor

@rdblue rdblue left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. I just have one comment about the FileNotFoundError. I'll defer to @Fokko to also review since he catches a lot more than I do in Python.

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

One minor comment about the credential, but apart from that this is looks great 👏🏻

python/Makefile Show resolved Hide resolved
python/pyiceberg/io/fsspec.py Show resolved Hide resolved
python/tests/conftest.py Show resolved Hide resolved
@cccs-eric cccs-eric marked this pull request as ready for review December 15, 2022 13:00
@Fokko
Copy link
Contributor

Fokko commented Dec 15, 2022

@cccs-eric it looks like we have to add pyparsing to the pyproject.toml. I don't know why it was working before, but it should have been added in #6259

@Fokko
Copy link
Contributor

Fokko commented Dec 15, 2022

@cccs-eric I forgot one thing, could you also add the adlfs option to the docs in python/mkdocs/?

@cccs-eric
Copy link
Contributor Author

Yes, I'll do that. And I've also realized that I'm not calling the tests for ADLFS. Should I simply add this to python-ci.yml?

    - name: Tests S3
      working-directory: ./python
      run: make test-s3
    - name: Tests Adlfs
      working-directory: ./python
      run: make test-adlfs

@Fokko
Copy link
Contributor

Fokko commented Dec 15, 2022

@cccs-eric Yes, please do!

@github-actions github-actions bot added the INFRA label Dec 15, 2022
@cccs-eric
Copy link
Contributor Author

@cccs-eric it looks like we have to add pyparsing to the pyproject.toml. I don't know why it was working before, but it should have been added in #6259

@Fokko I don't understand what is going on. pyparsing is part of pyproject.toml and when I run make test-s3, it works on my machine ™️

@rdblue
Copy link
Contributor

rdblue commented Dec 16, 2022

@cccs-eric, I just merged $6439 that fixes the pyparsing problem (my fault) so you should be able to rebase and get tests working. Sorry about that!

@cccs-eric
Copy link
Contributor Author

@cccs-eric, I just merged $6439 that fixes the pyparsing problem (my fault) so you should be able to rebase and get tests working. Sorry about that!

Thanks @rdblue , build is now passing 🥳

@cccs-eric
Copy link
Contributor Author

@Fokko One last thing that I haven't done is modifying verify-release.md for the integration tests. Should I add test-adlfs in there?
https://github.com/apache/iceberg/blob/master/python/mkdocs/docs/verify-release.md?plain=1#L86-L90

@rdblue
Copy link
Contributor

rdblue commented Dec 18, 2022

@cccs-eric, looks like this was out of date. I tried to fix conflicts, but it is failing tests. Can you fix and then we'll merge?

@cccs-eric
Copy link
Contributor Author

@rdblue All clear now

@Fokko
Copy link
Contributor

Fokko commented Dec 19, 2022

Awesome, thanks @cccs-eric for working on this 🚀

@Fokko Fokko merged commit 3ce88df into apache:master Dec 19, 2022
@cccs-eric cccs-eric deleted the adlfs-support branch December 19, 2022 12:21
@rdblue
Copy link
Contributor

rdblue commented Dec 19, 2022

Thanks, @cccs-eric!

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

Successfully merging this pull request may close these issues.

3 participants