ARROW-15635: [C++] Support nested extension-id-registry #13214

rtpsw · 2022-05-22T10:59:25Z

See this post.

github-actions · 2022-05-22T10:59:46Z

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

github-actions · 2022-05-22T10:59:47Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

rtpsw · 2022-05-22T11:04:16Z

cc @icexelloss

icexelloss · 2022-05-23T15:59:56Z

registering Python UDFs to an extension registry instance that (1) is specific to the Python interpreter and (2) is linked to the default global one (so it can find both UDF and normal functions). This Python-specific registry would then be passed to be used by the execution engine. I think this way (only) the Python-specific registry would naturally get cleaned up on finalization of the Python interpreter.

I think high level this makes sense as @westonpace previously mentioned in email discussion about an local/temporary registry for a specific substrait plan execution. However, I am not expert on this matter so curious to hear about how @westonpace and @vibhatha think.

rtpsw · 2022-05-23T16:46:23Z

If this makes sense to other people, I could also try to make a similar separate solution for the function registry.

icexelloss · 2022-05-24T19:53:01Z

@westonpace I wonder what's your thoughts about the changes here? Is this on the right track?

vibhatha · 2022-05-24T23:40:59Z

@rtpsw Is the PR associated with the correct JIRA?

westonpace

I think the concept is a good idea. I think @sanjibansg is close to having a PR for ARROW-15582 which changes the extension id registry a bit. However, these two PRs are addressing orthogonal concerns so it should be pretty straightforward to rebase whichever one ends up being second.

What would you envision the lifetime of the custom extension id registry being? Based on some of the comments it sounds like you might be thinking per-plan.

For a plan-specific "embedded UDF" (i.e. a UDF that has been pickled and serialized as part of the plan) I though the idea was that the embedded UDF would have some special way of being inserted into an expression (admittedly, the protobuf is lacking for this part at the moment) and so it wouldn't need to be a part of the extension set.

For UDFs that are not embedded in the plan itself it seems the user would probably want to just register those once so I think the lifetime of this nested set would still be process-scoped.

The proposed change works fine for process-scoped but I want to make sure I understand the intended usage.

Lastly, we should have some unit tests in place before we merge this.

westonpace · 2022-05-25T08:26:33Z

cpp/src/arrow/engine/substrait/extension_set.h

  };
  virtual util::optional<TypeRecord> GetType(const DataType&) const = 0;
  virtual util::optional<TypeRecord> GetType(Id, bool is_variation) const = 0;
+  virtual Status CanRegisterType(Id, std::shared_ptr<DataType> type,


Suggested change

virtual Status CanRegisterType(Id, std::shared_ptr<DataType> type,

virtual Status CanRegisterType(Id, const std::shared_ptr<DataType>& type,

westonpace · 2022-05-25T08:26:51Z

cpp/src/arrow/engine/substrait/extension_set.h

  virtual util::optional<FunctionRecord> GetFunction(Id) const = 0;
  virtual util::optional<FunctionRecord> GetFunction(
      util::string_view arrow_function_name) const = 0;
+  virtual Status CanRegisterFunction(Id, std::string arrow_function_name) const = 0;


Suggested change

virtual Status CanRegisterFunction(Id, std::string arrow_function_name) const = 0;

virtual Status CanRegisterFunction(Id, const std::string& arrow_function_name) const = 0;

westonpace · 2022-05-25T08:27:48Z

cpp/src/arrow/engine/substrait/extension_set.h


 private:
-  ExtensionIdRegistry* registry_;
+  const ExtensionIdRegistry* registry_;


Is there any particular reason to change this to a const? I think it's fine but it seems unrelated to the change so I wanted to make sure I wasn't missing anything.

This PR is an extraction from a larger project I'm working on, and I just wanted the compiler to ensure that no unintended modification to the extension-id-registry occurs via this class. I don't mind so much keeping or removing the const modifier in this PR.

westonpace · 2022-05-25T08:30:10Z

cpp/src/arrow/engine/substrait/util.h

 ARROW_ENGINE_EXPORT Result<std::shared_ptr<Buffer>> SerializeJsonPlan(
    const std::string& substrait_json);

+ARROW_ENGINE_EXPORT std::shared_ptr<ExtensionIdRegistry> MakeExtensionIdRegistry();


We should probably start documenting the functions in this file. At least a brief reference to the nice docs you have in extension_set.h

rtpsw · 2022-05-25T11:48:02Z

@rtpsw Is the PR associated with the correct JIRA?

Yeah, this is by mistake. I'll move it soon.

rtpsw · 2022-05-25T12:07:02Z

What would you envision the lifetime of the custom extension id registry being? Based on some of the comments it sounds like you might be thinking per-plan.

Either or both per-process and per-plan. The scope can be controlled by the user (from Arrow or PyArrow):

For per-process scope, the user makes per_process_registry = nested_extension_id_registry(default_extension_id_registry()) and keeps it throughout the lifetime of the process.
For per-plan scope, the user recreates per_plan_registry = nested_extension_id_registry(default_extension_id_registry()) for each plan,
For both per-process and per-plan scope, the user makes per_process_registry = nested_extension_id_registry(default_extension_id_registry()) and recreates per_plan_registry = nested_extension_id_registry(per_process_registry) (note the double-nesting) for each plan.

For a plan-specific "embedded UDF" (i.e. a UDF that has been pickled and serialized as part of the plan) I though the idea was that the embedded UDF would have some special way of being inserted into an expression (admittedly, the protobuf is lacking for this part at the moment) and so it wouldn't need to be a part of the extension set.

Locally, I was able to get this to work for UDFs restricted to element-wise flat-scalar-valued (not analytic or reduction, not struct, not table or dataset).

For UDFs that are not embedded in the plan itself it seems the user would probably want to just register those once so I think the lifetime of this nested set would still be process-scoped.

My local solution involves an Ibis/Substrait/Arrow workflow, where all UDFs are registered once and get serialized into each Substrait plan they are used in.

The proposed change works fine for process-scoped but I want to make sure I understand the intended usage.

As explained above, I think it works more broadly.

Lastly, we should have some unit tests in place before we merge this.

Agreed. I'll move on to add them, now that this approach seems to be acceptable.

rtpsw · 2022-05-25T15:40:04Z

This PR is replaced by #13232

Replacing #13214 Lead-authored-by: Yaron Gvili <[email protected]> Co-authored-by: rtpsw <[email protected]> Signed-off-by: Weston Pace <[email protected]>

ARROW-15635: [C++] Support nested extension-id-registry

30a8dbf

github-actions bot added the Component: C++ label May 22, 2022

rtpsw added 9 commits May 22, 2022 07:17

lint

a8f1104

lint

e840693

lint

cc38b93

lint

50bc1aa

Add virtual dtors to extension-id-registry impls

2e05a98

Add override modifiers

fa9e6bd

lint

5eb852f

lint

ad42d45

lint

f3dde55

bug fix

d091807

westonpace requested changes May 25, 2022

View reviewed changes

rtpsw mentioned this pull request May 25, 2022

ARROW-16657: [C++] Support nesting of extension-id-registries #13232

Merged

westonpace closed this May 26, 2022

rtpsw deleted the ARROW-15635 branch June 1, 2022 09:42

rtpsw mentioned this pull request Jun 15, 2022

ARROW-16823: [C++] Arrow Substrait enhancements for UDF #13375

Merged

asfimport mentioned this pull request Oct 25, 2022

[C++][Python] UDF Integration #31094

Open

14 tasks

	virtual Status CanRegisterType(Id, std::shared_ptr<DataType> type,
	virtual Status CanRegisterType(Id, const std::shared_ptr<DataType>& type,

	virtual Status CanRegisterFunction(Id, std::string arrow_function_name) const = 0;
	virtual Status CanRegisterFunction(Id, const std::string& arrow_function_name) const = 0;

ARROW-15635: [C++] Support nested extension-id-registry #13214

ARROW-15635: [C++] Support nested extension-id-registry #13214

Uh oh!

Conversation

rtpsw commented May 22, 2022

Uh oh!

github-actions bot commented May 22, 2022

Uh oh!

github-actions bot commented May 22, 2022

Uh oh!

rtpsw commented May 22, 2022

Uh oh!

icexelloss commented May 23, 2022

Uh oh!

rtpsw commented May 23, 2022

Uh oh!

icexelloss commented May 24, 2022

Uh oh!

vibhatha commented May 24, 2022

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

westonpace May 25, 2022

Choose a reason for hiding this comment

Uh oh!

westonpace May 25, 2022

Choose a reason for hiding this comment

Uh oh!

westonpace May 25, 2022

Choose a reason for hiding this comment

Uh oh!

rtpsw May 25, 2022

Choose a reason for hiding this comment

Uh oh!

westonpace May 25, 2022

Choose a reason for hiding this comment

Uh oh!

rtpsw commented May 25, 2022

Uh oh!

rtpsw commented May 25, 2022

Uh oh!

rtpsw commented May 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants