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 ballista python module #58

Closed
wants to merge 5 commits into from
Closed

Add ballista python module #58

wants to merge 5 commits into from

Conversation

nl5887
Copy link
Contributor

@nl5887 nl5887 commented Jun 5, 2022

Rationale for this change

This will introduce the ballista python module, similar to the datafusion python module.

@andygrove
Copy link
Member

Thank you for the contribution @nl5887. Having a Python repl as part of Ballista will make it much more accessible.

Because this builds on the datafusion-python module which was originally part of DataFusion but is now part of the datafusion-contrib GitHub org (outside of Apache Arrow governance) I need to figure out what the IP clearance situation is for this contribution.

One option may be for us to restore the original datafusion-python code here from before it was removed from this repo and then rebase your PR so it just has the additions on top of that. Another option will be for us to treat it like a regular donation which will require getting all contributors to sign an ICLA (or remove their contributions from the donation).

@andygrove
Copy link
Member

@thinkharderdev
Copy link
Contributor

Nice! This is a great contribution.

export RUSTFLAGS='-C target-cpu=skylake'
docker run --rm -v $(pwd):/io \
--workdir /io \
konstin2/maturin:v0.12.16 \
Copy link
Member

Choose a reason for hiding this comment

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

this won't work - it has only 0.12.10 pushed to dockerhub

@andygrove
Copy link
Member

@nl5887 Now that #61 is merged, could you rebase this PR on top of that, with the code going into the python directory rather than ballista-python. This will allow us to see the specific changes that are being contributed.

* Revert "remove python (#1518)"

This reverts commit bac97fa.

* fix cargo.toml lint issues and exclude from workspace

* use original datafusion dependency

* fix build

* lint
@github-actions github-actions bot added the python label Jun 8, 2022
@nl5887
Copy link
Contributor Author

nl5887 commented Jun 8, 2022

@andygrove rebased this PR on top of #61. The conflicts shows how datafusion-contrib and this python converged.

@andygrove
Copy link
Member

Hi @jimexist. Now that we have restored the original DataFusion Python bindings in this repo, would you be willing to create a PR here to submit the improvements that you have made in the datafusion-contrib repo? That will reduce the size of this PR further. We need to be careful not to include changes from contributors that have not yet signed an ICLA but as far as I can see there are just two such commits and they could easily be excluded. If you don't have time to do this would you be ok with me creating the PR with your changes? Thanks.

@andygrove
Copy link
Member

Hi @jimexist just in case you missed the previous message. Let me know what your thoughts are,

@andygrove
Copy link
Member

Hi @nl5887 Sorry for the delay on this but there have been discussions on the mailing list about this and it turns out that we can just go ahead and PR these changes after all. Would you mind rebasing this PR and I can go ahead and review. Thanks for your patience.

@andygrove
Copy link
Member

It's been just over a month without activity so I will close this in favor of #157

Thanks for the help with this @nl5887 - I used the ballista context code from this PR

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

Successfully merging this pull request may close these issues.

4 participants