Rework the python bindings using conversion traits from arrow-rs #873

kszucs · 2021-08-13T15:31:34Z

Alternative to #856
Inspired by #856 (comment)
Depends on apache/arrow-rs#691

The CI will probably fail since I hardcoded arrow-rs local dependency.

datafusion/src/lib.rs

datafusion/Cargo.toml

python/Cargo.toml

datafusion/Cargo.toml

cpcloud · 2021-08-13T16:52:13Z

Any chance of rewording the commits to be something other than single characters?

kszucs · 2021-08-13T17:56:34Z

Any chance of rewording the commits to be something other than single characters?

Squashed.

kszucs · 2021-08-30T16:21:50Z

@jorgecarleitao @andygrove we should roll up apache/arrow-rs#691 and this PR so we can proceed with the ibis datafusion backend.

houqp · 2021-09-07T04:51:28Z

The simplification looks great, thanks @kszucs! Looks like there are some minor rust build errors that need to be addressed.

ballista/rust/core/src/serde/logical_plan/to_proto.rs

kszucs · 2021-09-08T13:43:55Z

The simplification looks great, thanks @kszucs! Looks like there are some minor rust build errors that need to be addressed.

I had to bump the arrow-rs dependency everywhere so we need to address a couple more issues.

@houqp there is an async sort order issue which is rather hard to comprehend, any ideas what could have caused this error?

jorgecarleitao

Looks great so far! It is a awesome simplification!

I am curious about the naming equivalents but I think we should merge this regardless.

An observation: from here on we are bound to use the same pyo3 version used by arrow-rs.

I suggest an extra reviewer (e.g. @alamb, @houqp) -- I focused mostly on the Python part.

jorgecarleitao · 2021-09-08T13:46:56Z

python/src/catalog.rs

+    database: Arc<dyn SchemaProvider>,
+}
+
+#[pyclass(name = "Table", subclass)]


is this related to pa.Table? It is the Python-equivalent of the TableProvider, right?

Nope, it was an attempt to expose the catalog information to ibis, but I may defer the catalog.rs to a follow-up PR instead.

jorgecarleitao · 2021-09-08T13:47:13Z

python/src/catalog.rs

+    catalog: Arc<dyn CatalogProvider>,
+}
+
+#[pyclass(name = "Database", subclass)]


same here; this is SchemaProvider in DataFusion, right?

Right. I'm going to remove this module from the PR to avoid confusion.

datafusion/src/pyarrow.rs

kszucs · 2021-09-08T14:14:49Z

I need to cover the newly exposed python API with unittests.

houqp · 2021-09-09T05:12:08Z

@kszucs I took a quick look at the physical_plan::sort_preserving_merge::tests::test_async error, it is caused by the arrow-rs upgrade. I haven't had the time to trace down on the offending arrow-rs commit yet. Will probably take a closer look at it today or tomorrow.

alamb · 2021-09-09T15:10:55Z

@kszucs I took a quick look at the physical_plan::sort_preserving_merge::tests::test_async error, it is caused by the arrow-rs upgrade. I haven't had the time to trace down on the offending arrow-rs commit yet. Will probably take a closer look at it today or tomorrow.

I am pretty sure I saw a failure in the sort_preserving_merge tests related to apache/arrow-rs#656

kszucs · 2021-09-09T16:10:03Z

@kszucs I took a quick look at the physical_plan::sort_preserving_merge::tests::test_async error, it is caused by the arrow-rs upgrade. I haven't had the time to trace down on the offending arrow-rs commit yet. Will probably take a closer look at it today or tomorrow.

I am pretty sure I saw a failure in the sort_preserving_merge tests related to apache/arrow-rs#656

Thanks Andrew for the heads up! I wonder, wouldn't it better if we would assert on record batch equality rather than its string representation?

alamb · 2021-09-09T16:26:26Z

Thanks Andrew for the heads up! I wonder, wouldn't it better if we would assert on record batch equality rather than its string representation?

@kszucs -- I agree that doing so would avoid such "small floating point changes require many test changes" type problem. I think the downside is that (in my opinion) the tests are then harder to read and update.

Let me prepare a draft PR for updating datafusion to the latest arrow-rs so we can at least decouple the "changes needed for just arrow-rs upgrade" from the other changes in this PR

kszucs · 2021-09-09T16:32:57Z

@kszucs -- I agree that doing so would avoid such "small floating point changes require many test changes" type problem. I think the downside is that (in my opinion) the tests are then harder to read and update.

I understand, though the test actually tests that the two representations are equal rather than that the results are equal.
Aren't there other assertion crates providing better checks, perhaps with optional additional context (as a possible future improvement)?

Let me prepare a draft PR for updating datafusion to the latest arrow-rs so we can at least decouple the "changes needed for just arrow-rs upgrade" from the other changes in this PR

Great, thank You!

alamb · 2021-09-09T16:46:46Z

I understand, though the test actually tests that the two representations are equal rather than that the results are equal.
Aren't there other assertion crates providing better checks, perhaps with optional additional context (as a possible future improvement)?

🤔 Yes , you are correct. There may be something deeper going on here and perhaps related to the change to use unstable sort in apache/arrow-rs#552. All the more reason to debug it now. I will work on that

alamb · 2021-09-09T16:51:11Z

#984 is the PR to update to arrow-rs master. The sort_preserving_merge test is indeed failing and I will continue to debug on that PR.

houqp · 2021-09-10T08:41:08Z

@kszucs @jorgecarleitao quick question, do we want to release this change as part of 0.3.0 or delay to 0.4.0?

kszucs · 2021-11-02T14:26:57Z

@houqp rebased

houqp · 2021-11-02T16:25:08Z

Thank you @kszucs ! Great simplification to the python binding :)

github-actions bot added datafusion Changes in the datafusion crate python sql SQL Planner labels Aug 13, 2021

cpcloud reviewed Aug 13, 2021

View reviewed changes

datafusion/src/lib.rs Outdated Show resolved Hide resolved

datafusion/Cargo.toml Outdated Show resolved Hide resolved

python/Cargo.toml Show resolved Hide resolved

python/Cargo.toml Outdated Show resolved Hide resolved

datafusion/Cargo.toml Outdated Show resolved Hide resolved

kszucs force-pushed the python-rework-2 branch from fbea0ed to 89bbaea Compare August 13, 2021 17:56

kszucs force-pushed the python-rework-2 branch from 89bbaea to df9b924 Compare August 30, 2021 15:54

kszucs mentioned this pull request Aug 30, 2021

feat(datafusion): initial implementation for Arrow Datafusion backend ibis-project/ibis#2918

Merged

kszucs force-pushed the python-rework-2 branch from df9b924 to 99a8ebf Compare August 31, 2021 13:13

kszucs mentioned this pull request Sep 1, 2021

Rework the python bindings [WIP] #856

Closed

kszucs force-pushed the python-rework-2 branch from d35ed23 to fccd0f1 Compare September 1, 2021 10:47

8bit-pixies mentioned this pull request Sep 5, 2021

Adding some support for PyArrow Date and Datetimes to Rust #969

Closed

kszucs changed the title ~~Rework the python bindings using conversion traits from arrow-rs [WIP]~~ Rework the python bindings using conversion traits from arrow-rs Sep 8, 2021

kszucs force-pushed the python-rework-2 branch from d54858c to d47311b Compare September 8, 2021 09:32

github-actions bot added the ballista label Sep 8, 2021

kszucs commented Sep 8, 2021

View reviewed changes

ballista/rust/core/src/serde/logical_plan/to_proto.rs Outdated Show resolved Hide resolved

jorgecarleitao approved these changes Sep 8, 2021

View reviewed changes

kszucs added 19 commits November 2, 2021 13:45

Resolve post-rebase issues

85f7be5

Resolve post-rebase issues

fd21e6f

Resolve post-rebase issues

51a2727

Address review comments

436e63a

Reuse wait_for_future from dataframe.rs

48a4779

Improve module organization

b96e871

Reorganize tests

e512147

Support Binary and LargeBinary arrays for ScalarValue::try_from_array

ad184e8

Cargo format

ae52d17

Define column and literal functions in python

84e7fc0

Apply isort

5831536

Add test for importing from datafusion.functions

69e257d

Rename internals to _internal

0e1901d

Make classes inheritable; add tests for imports; set module

6bedd79

Remove PyVolatility

a2ef207

Move ScalarUdf to udf.rs

c7f384a

Factor out PyScalarUDF and PyAggregateUDF

837c1ba

Refactor UDF and UDAF construction

b804772

Set public as the default database for the catalog

35ad333

kszucs force-pushed the python-rework-2 branch from 830a5ac to 35ad333 Compare November 2, 2021 12:47

houqp approved these changes Nov 2, 2021

View reviewed changes

houqp merged commit 1c351ec into apache:master Nov 2, 2021

houqp added enhancement New feature or request and removed datafusion Changes in the datafusion crate ballista sql SQL Planner labels Nov 4, 2021

jimexist mentioned this pull request Nov 18, 2021

function module in python binding is broken in 6.0 #1328

Closed

alamb mentioned this pull request Jan 21, 2022

(Left Over?) pyarrow does not compile #1633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework the python bindings using conversion traits from arrow-rs #873

Rework the python bindings using conversion traits from arrow-rs #873

kszucs commented Aug 13, 2021

cpcloud commented Aug 13, 2021

kszucs commented Aug 13, 2021

kszucs commented Aug 30, 2021

houqp commented Sep 7, 2021

kszucs commented Sep 8, 2021

jorgecarleitao left a comment

jorgecarleitao Sep 8, 2021

kszucs Sep 8, 2021

jorgecarleitao Sep 8, 2021

kszucs Sep 8, 2021

kszucs commented Sep 8, 2021

houqp commented Sep 9, 2021 •

edited

Loading

alamb commented Sep 9, 2021

kszucs commented Sep 9, 2021

alamb commented Sep 9, 2021

kszucs commented Sep 9, 2021

alamb commented Sep 9, 2021

alamb commented Sep 9, 2021

houqp commented Sep 10, 2021

kszucs commented Nov 2, 2021

houqp commented Nov 2, 2021

Rework the python bindings using conversion traits from arrow-rs #873

Rework the python bindings using conversion traits from arrow-rs #873

Conversation

kszucs commented Aug 13, 2021

cpcloud commented Aug 13, 2021

kszucs commented Aug 13, 2021

kszucs commented Aug 30, 2021

houqp commented Sep 7, 2021

kszucs commented Sep 8, 2021

jorgecarleitao left a comment

Choose a reason for hiding this comment

jorgecarleitao Sep 8, 2021

Choose a reason for hiding this comment

kszucs Sep 8, 2021

Choose a reason for hiding this comment

jorgecarleitao Sep 8, 2021

Choose a reason for hiding this comment

kszucs Sep 8, 2021

Choose a reason for hiding this comment

kszucs commented Sep 8, 2021

houqp commented Sep 9, 2021 • edited Loading

alamb commented Sep 9, 2021

kszucs commented Sep 9, 2021

alamb commented Sep 9, 2021

kszucs commented Sep 9, 2021

alamb commented Sep 9, 2021

alamb commented Sep 9, 2021

houqp commented Sep 10, 2021

kszucs commented Nov 2, 2021

houqp commented Nov 2, 2021

houqp commented Sep 9, 2021 •

edited

Loading