feat: add FFI support for user defined functions #1145
Conversation
timsaucer
changed the title
| result = [r.column(0) for r in result] | ||
| expected = [ | ||
| pa.array([3], type=pa.int64()), | ||
| pa.array([3], type=pa.int64()), |
There was a problem hiding this comment.
Completely unrelated to this PR, but why isn't this null instead of 3?
> select 1 + null;
+-----------------+
| Int64(1) + NULL |
+-----------------+
| |
+-----------------+
1 row(s) fetched.
Elapsed 0.004 seconds.
I'm more than open to the possibility that the group by has a subtly different behavior than normal addition, but it just caught me by surprise because I expected to see null there. Poking a bit, none of the other aggregates I thought of off the top of my head return null so I guess its at least consistent.
Anywho, just a curiosity while I was reviewing.
There was a problem hiding this comment.
I'm just reusing the Sum aggregate function, not writing my own. And by default Sum will skip null values in an aggregation. Sum and addition are actually different in this way!
There was a problem hiding this comment.
Oh, absolutely! I was just wearing my review hat while reading the test so was predicting that it’d be null and then got surprised so I noted it.
And, “aha!” on sum skipping null values, so thanks for that! I just have the “math on nulls returns nulls” in my brain so knowing that behavior isn’t consistent is a TIL.
|
Ok, that's twice now so I assume that's a config thing on this repo. |
There was a problem hiding this comment.
Thank you @timsaucer and @davisp for the review
FYI @kosiew in case you have any time to review this code
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "ffe060b978f74ab446be722adb8a274e052e005bf6dfd171caadc3abaad10080" | ||
| version = "48.0.0" | ||
| source = "git+https://github.com/apache/datafusion?rev=33a32d4382bee7e3c705d0f55d05c24a115a2f98#33a32d4382bee7e3c705d0f55d05c24a115a2f98" |
There was a problem hiding this comment.
I am curious why this lock file entry contains a revision commit point.
There was a problem hiding this comment.
Maybe regenerate the lock file to avoid curious questions like mine 😄 ?
There was a problem hiding this comment.
Good call! Thank you!
Which issue does this PR close?
Closes #1017
Rationale for this change
Now that we have user defined scalar, aggregate, and window functions in the upstream
datafusion48.0.0, we can add support indatafusion-python. This allows for greater code reuse and extends the available options for Rust implementation of functions to be exposed to python.What changes are included in this PR?
For all three types of functions, add pycapsule support in the datafusion-python libraries and add the supporting methods to initialize these as UDFs to register with the session context.
Integration tests are included.
Are there any user-facing changes?
This is only addition. All previous code is not impacted.