ARROW-16823: [C++] Arrow Substrait enhancements for UDF by rtpsw · Pull Request #13375 · apache/arrow

rtpsw · 2022-06-13T12:02:07Z

See https://issues.apache.org/jira/browse/ARROW-16823

github-actions · 2022-06-13T12:02:31Z

https://issues.apache.org/jira/browse/ARROW-16823

vibhatha · 2022-06-13T12:11:15Z

@rtpsw I did skim through the PR, interesting!.
What is required to get a functional test case to evaluate registering a UDF? I went through the JIRA, but it is not clear what is meant by

registering a function (with an Id) external to the plan

rtpsw · 2022-06-13T13:50:17Z

@rtpsw I did skim through the PR, interesting!. What is required to get a functional test case to evaluate registering a UDF? I went through the JIRA, but it is not clear what is meant by
registering a function (with an Id) external to the plan

You're right, this isn't trivial. The issue is that Id is defined with two members of type util::string_view, which point to strings that must be held elsewhere while the plan is in scope. This is satisfied for an Id that has these fields pointing to strings parsed from the Substrait plan itself. However, if a user registers a function with an Id whose field values are external to the plan, then the strings they point to should be held while the plan is in scope, so it makes sense to keep the strings on the plan; this is what AddExternalSymbol is meant for.

I intend to add test cases a bit later. This PR is an extraction from a larger project project I'm working on for end-to-end (Ibis/Ibis-Substrait/PyArrow) support for Python-UDFs.

rtpsw · 2022-06-14T11:41:06Z

@vibhatha, I think this PR is ready for review. Are you the one to review it?

vibhatha · 2022-06-14T11:55:22Z

@vibhatha, I think this PR is ready for review. Are you the one to review it?

@rtpsw I was reading it now. But I won't be a major reviewer. I will be closely reading and co-review certain parts.

cc @westonpace @lidavidm could you please take a look.

rtpsw · 2022-06-15T18:16:07Z

Right now I see only a minor change waiting for me to make. Let me know if you're still reviewing and I'll hold for your notification.

lidavidm · 2022-06-15T18:56:43Z

@vibhatha, I'm not up to date on Acero/Substrait progress anymore. Are the changes here reasonable?

rtpsw · 2022-06-15T20:18:45Z

@vibhatha, I'm not up to date on Acero/Substrait progress anymore. Are the changes here reasonable?

I have some explanation here, in case it helps. The TBD parts are expected in an upcoming PR (or two) I'll prepare.

rtpsw · 2022-06-15T20:36:29Z

For background on nested registries, see:

a completed PR on nested extension-id-registries
a pending PR on nested function-registries, which I ended up merging here.

vibhatha · 2022-06-15T23:42:42Z

@vibhatha, I'm not up to date on Acero/Substrait progress anymore. Are the changes here reasonable?

@lidavidm I am going to go through again. I will check with my knowlege on ACERO/substrait. But it would be better to have another review from @westonpace on this.

vibhatha · 2022-06-15T23:43:20Z

cc @westonpace could please take a look.

vibhatha · 2022-06-16T00:23:26Z

@vibhatha, I'm not up to date on Acero/Substrait progress anymore. Are the changes here reasonable?
I have some explanation here, in case it helps. The TBD parts are expected in an upcoming PR (or two) I'll prepare.

@rtpsw I added a comment to the JIRA. Appreciate your feedback to clarify the design and usage.

vibhatha · 2022-06-16T00:28:00Z

I think we have two important pieces discussed here. One is how Substrait-UDF usage is benefitted and the second is how the function registry usage must be modified. Since the function registry usage is an important piece for the first task, should we address it first and move for the second. Just a thought. We could test the usage of the temporary FR further.

westonpace · 2022-06-17T01:18:30Z

Sorry, I'm still catching up from being out earlier this week. I'll take a look at this tomorrow.

westonpace

I skipped over the changes to nested function registries since I already reviewed those (I think) in #13252 . I agree with these additions. It would be nice to move away from Substrait plans requiring users to use the consuming sink node and this is a good way to do that while keeping the convenience if desired.

This will enable custom non-embedded functions to be used in Substrait plans although I'd prefer it to be a bit more automatic (e.g. not requiring a second register call).

The ability to easily create plans for writing is a nice convenience as well.

rtpsw · 2022-06-17T20:49:18Z

I skipped over the changes to nested function registries since I already reviewed those (I think) in #13252 .

I think there are a few additions here, so I'll try to rebase to make the diff clear.

This will enable custom non-embedded functions to be used in Substrait plans although I'd prefer it to be a bit more automatic (e.g. not requiring a second register call).

I'll look into this.

rtpsw · 2022-06-19T09:19:35Z

Rebase done and pushed using git push --force-with-lease origin HEAD.

rtpsw · 2022-06-23T18:26:32Z

@westonpace, is this good to go?

westonpace

Just a few more thoughts.

westonpace

Were there any other changes you still needed to make here @rtpsw or is this good to go?

rtpsw · 2022-06-30T19:46:03Z

This is good to go from my point of view. I have an upcoming PyArrow UDF PR that will use the changes in this one.

See https://issues.apache.org/jira/browse/ARROW-16823 Authored-by: Yaron Gvili <rtpsw@hotmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

github-actionsBot added the Component: C++ label Jun 13, 2022

vibhatha reviewed Jun 13, 2022
View reviewed changes

Comment threadcpp/src/arrow/engine/substrait/plan_internal.cc Outdated

vibhatha reviewed Jun 13, 2022
View reviewed changes

Comment threadcpp/src/arrow/engine/substrait/serde.cc Outdated

vibhatha reviewed Jun 14, 2022
View reviewed changes