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 TableProvider::insert_into into FFI Bindings #14391

Merged
merged 3 commits into from
Feb 1, 2025

Conversation

davisp
Copy link
Member

@davisp davisp commented Jan 31, 2025

This method was missing from the FFI bindings for use in datafusion-python extensions.

Which issue does this PR close?

Closes #14390.

Rationale for this change

Add missing method wrapper for TableProvider::insert_into in the FFI bindings.

What changes are included in this PR?

A wrapper for TableProvider::insert_into is added.

Are these changes tested?

Its tested as thoroughly as TableProvider::scan.

Are there any user-facing changes?

Previously wrapped TableProvider instances will now support insert_into which I guess would be user-facing?

This method was missing from the FFI bindings for use in
datafusion-python extensions.
@davisp
Copy link
Member Author

davisp commented Jan 31, 2025

@timsaucer Apparently I can't request my own reviewers, but I figured you'd want a heads up on this.

@timsaucer timsaucer self-requested a review January 31, 2025 17:45
@alamb alamb changed the title Wrap TableProvider::insert_into Add TableProvider::insert_into into FFI Bindings Feb 1, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @davisp this looks good to me

As we are about to do a release for 45 I plan to merge this shortly to get it into the release. FYI @timsaucer

@alamb
Copy link
Contributor

alamb commented Feb 1, 2025

I merged up from main to make sure CI is running on the latest commit. Once tests pass I plan to merge

@alamb
Copy link
Contributor

alamb commented Feb 1, 2025

The CI failure https://github.com/apache/datafusion/actions/runs/13088375990/job/36522221854?pr=14391

Seems to be due to a logical conflict with
#13937

I am investigating

Copy link
Contributor

@timsaucer timsaucer left a comment

Choose a reason for hiding this comment

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

Thank you! This looks like a very clean addition and includes unit tests.

@alamb
Copy link
Contributor

alamb commented Feb 1, 2025

I have to go do something else now -- @timsaucer any chance you can figure out the CI failure?

@timsaucer
Copy link
Contributor

I have to go do something else now -- @timsaucer any chance you can figure out the CI failure?

Yes, I can do this right now

… of the current async function to the foreign execution plan on the insert_into operation
@alamb
Copy link
Contributor

alamb commented Feb 1, 2025

Woohoo! 🚀

@alamb alamb merged commit 7f9a8c0 into apache:main Feb 1, 2025
25 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 1, 2025

Thanks @davisp and @timsaucer

cj-zhukov pushed a commit to cj-zhukov/datafusion that referenced this pull request Feb 3, 2025
* Wrap TableProvider::insert_into

This method was missing from the FFI bindings for use in
datafusion-python extensions.

* Switch from passing the runtime around to it's handle, add the handle of the current async function to the foreign execution plan on the insert_into operation

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Tim Saucer <timsaucer@gmail.com>
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.

Add FFI wrappers for TableProvider::insert_into
3 participants