ARROW-16677: [C++] Support nesting of function registries #13252
Conversation
|
|
|
cc @icexelloss |
|
This proposed feature, along with the one in #13232 , would enable scoping for all registries used for UDFs. |
|
@westonpace - could you review? |
|
Looks like the failed jobs are not due to the changes here. |
|
@westonpace, is this good to go? |
There was a problem hiding this comment.
Most of these changes are minor but we do need to clean up the constructors.
I realized as I was doing the review that some of these method comments (e.g. \brief) were following existing patterns in this file. However, I think the suggestions move us more in line with the rest of the code base (e.g. \brief should be a one line description. The things that follow are a note) and we can clean up the others later.
I find the nested ternary (i.e. (a ? b : c) & d a little too compact to follow. Now that Status && Status is unavailable I think we should probably avoid & when short-circuit is desired when using Status.
It might be worth noting somewhere (assuming my understanding is correct here) that even if allow_overwrite is true it a registry will never modify its parent.
Lastly, I'm not sure why we didn't check for existing function names in AddAlias. I think we should probably correct that here rather than persist the status quo. But it's possible I'm not understanding some nuance.
cpp/src/arrow/compute/registry.cc
Outdated
| FunctionRegistry::FunctionRegistry() { impl_.reset(new FunctionRegistryImpl()); } | ||
| std::unique_ptr<FunctionRegistry> FunctionRegistry::Make(FunctionRegistry* parent) { | ||
| return std::unique_ptr<FunctionRegistry>( | ||
| new FunctionRegistry(new FunctionRegistry::FunctionRegistryImpl(&*parent->impl_))); |
There was a problem hiding this comment.
| new FunctionRegistry(new FunctionRegistry::FunctionRegistryImpl(&*parent->impl_))); | |
| new FunctionRegistry(parent->impl_.get()); |
The way it is currently will create a copy of the parent. This pointer leaks but, more importantly, if you did something like...
auto parent = GetFunctionRegistry();
auto nested = FunctionRegistry::Make(parent);
// The function added here (using parent) will not be visible by nested
// and, if I'm understanding the purpose correctly, we would want that
parent->AddFunction(...);
Instead we should just take a non-owning "view" pointer to the parent. If the parent is deleted before the nested registry is deleted then bad things will happen but that is to be expected.
There was a problem hiding this comment.
This change leads to a failed test:
/mnt/user1/tscontract/github/rtpsw/arrow/cpp/src/arrow/compute/registry_test.cc:124: Failure
Failed
'default_registry->CanAddFunction(func)' failed with Key error: Already have a function registered with name: f1
[ FAILED ] TestRegistry.RegisterTempFunctions (0 ms)
The existing code makes a new FunctionRegistryImpl with a parent of the same type, which is what we want for scoping. Though I did fix this code to use .get().
I agree with almost everything. I think your comments about logic are not minor :) I ended up fixing and cleaning up the code quite a bit following them. Thanks for catching. |
|
This can get pushed and I'll handle the merge. |
Currently, only a default function-registry is supported. Modifying this registry has global effects, which is often undesirable. Support for nesting function-registries will provide scoping for such modifications.