You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/27 09:07:39 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #13252: ARROW-16677: [C++] Support nesting of function registries
rtpsw opened a new pull request, #13252:
URL: https://github.com/apache/arrow/pull/13252
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1144909608
@westonpace - could you review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1146006722
Looks like the failed jobs are not due to the changes here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1150927804
> 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.
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1139429542
https://issues.apache.org/jira/browse/ARROW-16677
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1139431222
This proposed feature, along with the one in https://github.com/apache/arrow/pull/13232 , would enable scoping for all registries used for UDFs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13252:
URL: https://github.com/apache/arrow/pull/13252#discussion_r888518121
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,7 +34,20 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ virtual ~FunctionRegistryImpl() {}
+
+ private:
Review Comment:
Instead of switching back and forth between public and private can we group the public and private functions together?
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -48,23 +61,56 @@ class FunctionRegistry::FunctionRegistryImpl {
if (it != name_to_function_.end() && !allow_overwrite) {
return Status::KeyError("Already have a function registered with name: ", name);
}
- name_to_function_[name] = std::move(function);
+ add(name, std::move(function));
Review Comment:
```suggestion
if (do_add) {
name_to_function_[name] = std::move(function);
}
```
If we change `add` to `bool do_add` I think this is a bit easier to follow. If performance is a concern (I don't think it would be as this isn't really in a critical section) you could use a template...
```
template <bool kDoAdd>
Status DoAddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
...
if (kDoAdd) {
name_to_function_[name] = std::move(function);
}
}
virtual Status CanAddFunction(std::shared_ptr<Function> function,
bool allow_overwrite) {
return DoAddFunction<false>(function, allow_overwrite);
}
...
```
##########
cpp/src/arrow/compute/registry_test.cc:
##########
@@ -27,37 +27,44 @@
#include "arrow/status.h"
#include "arrow/testing/gtest_util.h"
#include "arrow/util/macros.h"
+#include "arrow/util/make_unique.h"
namespace arrow {
namespace compute {
-class TestRegistry : public ::testing::Test {
- public:
- void SetUp() { registry_ = FunctionRegistry::Make(); }
+using MakeFunctionRegistry = std::function<std::unique_ptr<FunctionRegistry>()>;
+using GetNumFunctions = std::function<int()>;
+using GetFunctionNames = std::function<std::vector<std::string>()>;
+using TestRegistryParams =
+ std::tuple<MakeFunctionRegistry, GetNumFunctions, GetFunctionNames, std::string>;
- protected:
- std::unique_ptr<FunctionRegistry> registry_;
-};
+struct TestRegistry : public ::testing::TestWithParam<TestRegistryParams> {};
-TEST_F(TestRegistry, CreateBuiltInRegistry) {
+TEST(TestRegistry, CreateBuiltInRegistry) {
Review Comment:
TEST_P? (I might be wrong here. I can't remember if it's ok to mix parameterized and non-parameterized tests)
##########
cpp/src/arrow/compute/registry_test.cc:
##########
@@ -27,37 +27,44 @@
#include "arrow/status.h"
#include "arrow/testing/gtest_util.h"
#include "arrow/util/macros.h"
+#include "arrow/util/make_unique.h"
namespace arrow {
namespace compute {
-class TestRegistry : public ::testing::Test {
- public:
- void SetUp() { registry_ = FunctionRegistry::Make(); }
+using MakeFunctionRegistry = std::function<std::unique_ptr<FunctionRegistry>()>;
+using GetNumFunctions = std::function<int()>;
+using GetFunctionNames = std::function<std::vector<std::string>()>;
+using TestRegistryParams =
+ std::tuple<MakeFunctionRegistry, GetNumFunctions, GetFunctionNames, std::string>;
- protected:
- std::unique_ptr<FunctionRegistry> registry_;
-};
+struct TestRegistry : public ::testing::TestWithParam<TestRegistryParams> {};
-TEST_F(TestRegistry, CreateBuiltInRegistry) {
+TEST(TestRegistry, CreateBuiltInRegistry) {
// This does DCHECK_OK internally for now so this will fail in debug builds
// if there is a problem initializing the global function registry
FunctionRegistry* registry = GetFunctionRegistry();
ARROW_UNUSED(registry);
}
-TEST_F(TestRegistry, Basics) {
- ASSERT_EQ(0, registry_->num_functions());
+TEST_P(TestRegistry, Basics) {
+ auto registry_factory = std::get<0>(GetParam());
+ auto registry_ = registry_factory();
+ auto get_num_funcs = std::get<1>(GetParam());
+ int n_funcs = get_num_funcs();
+ auto get_func_names = std::get<2>(GetParam());
+ std::vector<std::string> func_names = get_func_names();
+ ASSERT_EQ(n_funcs + 0, registry_->num_functions());
Review Comment:
```suggestion
ASSERT_EQ(n_funcs, registry_->num_functions());
```
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -84,6 +106,11 @@ class ARROW_EXPORT FunctionRegistry {
// Use PIMPL pattern to not have std::unordered_map here
class FunctionRegistryImpl;
std::unique_ptr<FunctionRegistryImpl> impl_;
+
+ explicit FunctionRegistry(FunctionRegistryImpl* impl);
+
+ class NestedFunctionRegistryImpl;
+ friend class NestedFunctionRegistryImpl;
Review Comment:
I'm not sure this friend declaration is needed.
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -48,23 +61,56 @@ class FunctionRegistry::FunctionRegistryImpl {
if (it != name_to_function_.end() && !allow_overwrite) {
return Status::KeyError("Already have a function registered with name: ", name);
}
- name_to_function_[name] = std::move(function);
+ add(name, std::move(function));
return Status::OK();
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
+ public:
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return DoAddFunction(function, allow_overwrite, kFuncAddNoOp);
+ }
+
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return DoAddFunction(function, allow_overwrite, kFuncAddDo);
+ }
+
+ private:
+ Status DoAddAlias(const std::string& target_name, const std::string& source_name,
+ FuncAdd add) {
std::lock_guard<std::mutex> mutation_guard(lock_);
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
+ auto func_res = GetFunction(source_name); // must not acquire the mutex
+ if (!func_res.ok()) {
return Status::KeyError("No function registered with name: ", source_name);
}
- name_to_function_[target_name] = it->second;
+ add(target_name, func_res.ValueOrDie());
return Status::OK();
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
+ public:
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ return DoAddAlias(target_name, source_name, kFuncAddNoOp);
+ }
+
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ return DoAddAlias(target_name, source_name, kFuncAddDo);
+ }
+
+ private:
+ using FuncOptTypeAdd = std::function<void(const FunctionOptionsType* options_type)>;
+
+ const FuncOptTypeAdd kFuncOptTypeAddNoOp = [](const FunctionOptionsType* options_type) {
+ };
+ const FuncOptTypeAdd kFuncOptTypeAddDo =
+ [this](const FunctionOptionsType* options_type) {
+ name_to_options_type_[options_type->type_name()] = options_type;
+ };
Review Comment:
Same thing here. Replace with a boolean.
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -48,23 +61,56 @@ class FunctionRegistry::FunctionRegistryImpl {
if (it != name_to_function_.end() && !allow_overwrite) {
return Status::KeyError("Already have a function registered with name: ", name);
}
- name_to_function_[name] = std::move(function);
+ add(name, std::move(function));
return Status::OK();
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
+ public:
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return DoAddFunction(function, allow_overwrite, kFuncAddNoOp);
+ }
+
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return DoAddFunction(function, allow_overwrite, kFuncAddDo);
+ }
+
+ private:
+ Status DoAddAlias(const std::string& target_name, const std::string& source_name,
+ FuncAdd add) {
std::lock_guard<std::mutex> mutation_guard(lock_);
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
+ auto func_res = GetFunction(source_name); // must not acquire the mutex
+ if (!func_res.ok()) {
return Status::KeyError("No function registered with name: ", source_name);
}
Review Comment:
```suggestion
ARROW_ASSIGN_OR_RAISE(auto func_res, GetFunction(source_name));
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -103,32 +160,140 @@ class FunctionRegistry::FunctionRegistryImpl {
return it->second;
}
- int num_functions() const { return static_cast<int>(name_to_function_.size()); }
+ virtual int num_functions() const { return static_cast<int>(name_to_function_.size()); }
private:
std::mutex lock_;
std::unordered_map<std::string, std::shared_ptr<Function>> name_to_function_;
std::unordered_map<std::string, const FunctionOptionsType*> name_to_options_type_;
};
+class FunctionRegistry::NestedFunctionRegistryImpl
+ : public FunctionRegistry::FunctionRegistryImpl {
Review Comment:
It's a little bit odd to have a pimpl pattern combined with inheritance. If we want this inheritance chain we might be better off converting `FunctionRegistry` to a pure virtual class like we have with `ExtensionIdRegistry`.
On the other hand, would it make sense to simple change the base `FunctionRegistryImpl` to always have a parent pointer that is sometimes `nullptr` (i.e. no parent == `nullptr`)?
Then the lookup can just be...
```
Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const override {
auto it = name_to_function_.find(name);
if (it == name_to_function_.end()) {
if (parent_ == nullptr) {
return Status::KeyError("No function registered with name: ", name);
}
return parent_->GetFunction(name);
}
return it->second;
}
```
##########
cpp/src/arrow/compute/registry_test.cc:
##########
@@ -85,5 +95,137 @@ TEST_F(TestRegistry, Basics) {
ASSERT_EQ(func, f2);
}
+INSTANTIATE_TEST_SUITE_P(
+ TestRegistry, TestRegistry,
+ testing::Values(
+ std::make_tuple(
+ static_cast<MakeFunctionRegistry>([]() { return FunctionRegistry::Make(); }),
+ []() { return 0; }, []() { return std::vector<std::string>{}; }, "default"),
+ std::make_tuple(
+ static_cast<MakeFunctionRegistry>([]() {
+ return FunctionRegistry::Make(GetFunctionRegistry());
+ }),
+ []() { return GetFunctionRegistry()->num_functions(); },
+ []() { return GetFunctionRegistry()->GetFunctionNames(); }, "nested")));
+
+TEST(TestRegistry, RegisterTempFunctions) {
+ auto default_registry = GetFunctionRegistry();
+ constexpr int rounds = 3;
+ for (int i = 0; i < rounds; i++) {
+ auto registry = FunctionRegistry::Make(default_registry);
+ for (std::string func_name : {"f1", "f2"}) {
+ std::shared_ptr<Function> func = std::make_shared<ScalarFunction>(
+ func_name, Arity::Unary(), /*doc=*/FunctionDoc::Empty());
+ ASSERT_OK(registry->CanAddFunction(func));
+ ASSERT_OK(registry->AddFunction(func));
+ ASSERT_RAISES(KeyError, registry->CanAddFunction(func));
+ ASSERT_RAISES(KeyError, registry->AddFunction(func));
+ ASSERT_OK(default_registry->CanAddFunction(func));
+ }
+ }
+}
+
+TEST(TestRegistry, RegisterTempAliases) {
+ auto default_registry = GetFunctionRegistry();
+ std::vector<std::string> func_names = default_registry->GetFunctionNames();
+ constexpr int rounds = 3;
+ for (int i = 0; i < rounds; i++) {
+ auto registry = FunctionRegistry::Make(default_registry);
+ for (std::string func_name : func_names) {
+ std::string alias_name = "alias_of_" + func_name;
+ std::shared_ptr<Function> func = std::make_shared<ScalarFunction>(
+ func_name, Arity::Unary(), /*doc=*/FunctionDoc::Empty());
+ ASSERT_RAISES(KeyError, registry->GetFunction(alias_name));
+ ASSERT_OK(registry->CanAddAlias(alias_name, func_name));
+ ASSERT_OK(registry->AddAlias(alias_name, func_name));
+ ASSERT_OK(registry->GetFunction(alias_name));
+ ASSERT_OK(default_registry->GetFunction(func_name));
+ ASSERT_RAISES(KeyError, default_registry->GetFunction(alias_name));
+ }
+ }
+}
+
+template <int N>
Review Comment:
Instead of `N` can we call this `kTypeNameSuffix`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #13252:
URL: https://github.com/apache/arrow/pull/13252#discussion_r893307324
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
Review Comment:
Almost right. The first point should be: check if source_name exists in this or parent and fail otherwise. An alias can be to the a function existing only in the parent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #13252:
URL: https://github.com/apache/arrow/pull/13252#discussion_r888658433
##########
cpp/src/arrow/compute/registry_test.cc:
##########
@@ -85,5 +95,137 @@ TEST_F(TestRegistry, Basics) {
ASSERT_EQ(func, f2);
}
+INSTANTIATE_TEST_SUITE_P(
+ TestRegistry, TestRegistry,
+ testing::Values(
+ std::make_tuple(
+ static_cast<MakeFunctionRegistry>([]() { return FunctionRegistry::Make(); }),
+ []() { return 0; }, []() { return std::vector<std::string>{}; }, "default"),
+ std::make_tuple(
+ static_cast<MakeFunctionRegistry>([]() {
+ return FunctionRegistry::Make(GetFunctionRegistry());
+ }),
+ []() { return GetFunctionRegistry()->num_functions(); },
+ []() { return GetFunctionRegistry()->GetFunctionNames(); }, "nested")));
+
+TEST(TestRegistry, RegisterTempFunctions) {
+ auto default_registry = GetFunctionRegistry();
+ constexpr int rounds = 3;
+ for (int i = 0; i < rounds; i++) {
+ auto registry = FunctionRegistry::Make(default_registry);
+ for (std::string func_name : {"f1", "f2"}) {
+ std::shared_ptr<Function> func = std::make_shared<ScalarFunction>(
+ func_name, Arity::Unary(), /*doc=*/FunctionDoc::Empty());
+ ASSERT_OK(registry->CanAddFunction(func));
+ ASSERT_OK(registry->AddFunction(func));
+ ASSERT_RAISES(KeyError, registry->CanAddFunction(func));
+ ASSERT_RAISES(KeyError, registry->AddFunction(func));
+ ASSERT_OK(default_registry->CanAddFunction(func));
+ }
+ }
+}
+
+TEST(TestRegistry, RegisterTempAliases) {
+ auto default_registry = GetFunctionRegistry();
+ std::vector<std::string> func_names = default_registry->GetFunctionNames();
+ constexpr int rounds = 3;
+ for (int i = 0; i < rounds; i++) {
+ auto registry = FunctionRegistry::Make(default_registry);
+ for (std::string func_name : func_names) {
+ std::string alias_name = "alias_of_" + func_name;
+ std::shared_ptr<Function> func = std::make_shared<ScalarFunction>(
+ func_name, Arity::Unary(), /*doc=*/FunctionDoc::Empty());
+ ASSERT_RAISES(KeyError, registry->GetFunction(alias_name));
+ ASSERT_OK(registry->CanAddAlias(alias_name, func_name));
+ ASSERT_OK(registry->AddAlias(alias_name, func_name));
+ ASSERT_OK(registry->GetFunction(alias_name));
+ ASSERT_OK(default_registry->GetFunction(func_name));
+ ASSERT_RAISES(KeyError, default_registry->GetFunction(alias_name));
+ }
+ }
+}
+
+template <int N>
Review Comment:
I'll use `kExampleSeqNum`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #13252:
URL: https://github.com/apache/arrow/pull/13252#discussion_r893315827
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -115,20 +181,47 @@ std::unique_ptr<FunctionRegistry> FunctionRegistry::Make() {
return std::unique_ptr<FunctionRegistry>(new FunctionRegistry());
}
-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_)));
Review 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()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace merged pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
westonpace merged PR #13252:
URL: https://github.com/apache/arrow/pull/13252
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #13252:
URL: https://github.com/apache/arrow/pull/13252#discussion_r888669276
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -84,6 +106,11 @@ class ARROW_EXPORT FunctionRegistry {
// Use PIMPL pattern to not have std::unordered_map here
class FunctionRegistryImpl;
std::unique_ptr<FunctionRegistryImpl> impl_;
+
+ explicit FunctionRegistry(FunctionRegistryImpl* impl);
+
+ class NestedFunctionRegistryImpl;
+ friend class NestedFunctionRegistryImpl;
Review Comment:
Right, it was a leftover.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1159215617
This can get pushed and I'll handle the merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on a diff in pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #13252:
URL: https://github.com/apache/arrow/pull/13252#discussion_r888612056
##########
cpp/src/arrow/compute/registry_test.cc:
##########
@@ -27,37 +27,44 @@
#include "arrow/status.h"
#include "arrow/testing/gtest_util.h"
#include "arrow/util/macros.h"
+#include "arrow/util/make_unique.h"
namespace arrow {
namespace compute {
-class TestRegistry : public ::testing::Test {
- public:
- void SetUp() { registry_ = FunctionRegistry::Make(); }
+using MakeFunctionRegistry = std::function<std::unique_ptr<FunctionRegistry>()>;
+using GetNumFunctions = std::function<int()>;
+using GetFunctionNames = std::function<std::vector<std::string>()>;
+using TestRegistryParams =
+ std::tuple<MakeFunctionRegistry, GetNumFunctions, GetFunctionNames, std::string>;
- protected:
- std::unique_ptr<FunctionRegistry> registry_;
-};
+struct TestRegistry : public ::testing::TestWithParam<TestRegistryParams> {};
-TEST_F(TestRegistry, CreateBuiltInRegistry) {
+TEST(TestRegistry, CreateBuiltInRegistry) {
Review Comment:
I observed that gtest generates separate test-name patterns for `TEST` and `TEST_P`, so there is no conflict.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1139429563
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1148553454
@westonpace, is this good to go?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a diff in pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13252:
URL: https://github.com/apache/arrow/pull/13252#discussion_r891490267
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -45,20 +45,42 @@ class FunctionOptionsType;
/// lower-level function execution.
class ARROW_EXPORT FunctionRegistry {
public:
- ~FunctionRegistry();
+ virtual ~FunctionRegistry();
Review Comment:
A destructor should not be the only virtual method. If no methods are virtual then the destructor does not need to be virtual.
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -45,20 +45,42 @@ class FunctionOptionsType;
/// lower-level function execution.
class ARROW_EXPORT FunctionRegistry {
public:
- ~FunctionRegistry();
+ virtual ~FunctionRegistry();
/// \brief Construct a new registry. Most users only need to use the global
/// registry
static std::unique_ptr<FunctionRegistry> Make();
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(std::unique_ptr<FunctionRegistry> parent);
Review Comment:
A function that accepts a unique_ptr should take ownership of that unique pointer. That does not seem to be what we are doing here. In this case I think we should probably get rid of this overload.
There is a "maybe owned" pattern we use in a few places where we have a constructor that takes a raw pointer and one that takes a shared_ptr but in that case we have two member variables (e.g. https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/caching.cc#L147)
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
Review Comment:
```suggestion
if (parent_ != nullptr) {
RETURN_NOT_OK(parent_->CanAddFunction(function, allow_overwrite)));
}
return DoAddFunction(function, allow_overwrite, /*add=*/false);
```
Now that it seems we won't be able to use `&&` we should probably move away from this more compact pattern and more explicitly short-circuit.
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -45,20 +45,42 @@ class FunctionOptionsType;
/// lower-level function execution.
class ARROW_EXPORT FunctionRegistry {
public:
- ~FunctionRegistry();
+ virtual ~FunctionRegistry();
/// \brief Construct a new registry. Most users only need to use the global
/// registry
static std::unique_ptr<FunctionRegistry> Make();
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(std::unique_ptr<FunctionRegistry> parent);
+
+ /// \brief Checks whether a new function can be added to the registry. Returns
+ /// Status::KeyError if a function with the same name is already registered
+ Status CanAddFunction(std::shared_ptr<Function> function, bool allow_overwrite = false);
+
/// \brief Add a new function to the registry. Returns Status::KeyError if a
/// function with the same name is already registered
Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite = false);
- /// \brief Add aliases for the given function name. Returns Status::KeyError if the
+ /// \brief Checks whether an alias can be added for the given function name. Returns
+ /// Status::KeyError if the function with the given name is not registered
+ Status CanAddAlias(const std::string& target_name, const std::string& source_name);
+
+ /// \brief Add alias for the given function name. Returns Status::KeyError if the
/// function with the given name is not registered
Status AddAlias(const std::string& target_name, const std::string& source_name);
+ /// \brief Checks whether a new function options type can be added to the registry.
+ /// Returns Status::KeyError if a function options type with the same name is already
+ /// registered
Review Comment:
```suggestion
/// \brief Check whether a new function options type can be added to the registry.
/// \return Status::KeyError if a function options type with the same name is already
/// registered
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
Review Comment:
```suggestion
Status AddAlias(const std::string& target_name,
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
Review Comment:
```suggestion
Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
+ }
- const std::string name = options_type->type_name();
- auto it = name_to_options_type_.find(name);
- if (it != name_to_options_type_.end() && !allow_overwrite) {
- return Status::KeyError(
- "Already have a function options type registered with name: ", name);
- }
- name_to_options_type_[name] = options_type;
- return Status::OK();
+ virtual Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/true);
Review Comment:
```suggestion
if (parent_ != nullptr) {
RETURN_NOT_OK(parent_->CanAddFunctionOptionsType(options_type, allow_overwrite));
}
return DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/true);
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -115,20 +181,47 @@ std::unique_ptr<FunctionRegistry> FunctionRegistry::Make() {
return std::unique_ptr<FunctionRegistry>(new FunctionRegistry());
}
-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_)));
Review Comment:
```suggestion
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.
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -45,20 +45,42 @@ class FunctionOptionsType;
/// lower-level function execution.
class ARROW_EXPORT FunctionRegistry {
public:
- ~FunctionRegistry();
+ virtual ~FunctionRegistry();
/// \brief Construct a new registry. Most users only need to use the global
/// registry
static std::unique_ptr<FunctionRegistry> Make();
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
Review Comment:
```suggestion
/// \brief Construct a new nested registry with the given parent.
///
/// Most users only need to use the global registry
```
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -45,20 +45,42 @@ class FunctionOptionsType;
/// lower-level function execution.
class ARROW_EXPORT FunctionRegistry {
public:
- ~FunctionRegistry();
+ virtual ~FunctionRegistry();
/// \brief Construct a new registry. Most users only need to use the global
/// registry
static std::unique_ptr<FunctionRegistry> Make();
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(std::unique_ptr<FunctionRegistry> parent);
+
+ /// \brief Checks whether a new function can be added to the registry. Returns
+ /// Status::KeyError if a function with the same name is already registered
+ Status CanAddFunction(std::shared_ptr<Function> function, bool allow_overwrite = false);
+
/// \brief Add a new function to the registry. Returns Status::KeyError if a
/// function with the same name is already registered
Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite = false);
- /// \brief Add aliases for the given function name. Returns Status::KeyError if the
+ /// \brief Checks whether an alias can be added for the given function name. Returns
+ /// Status::KeyError if the function with the given name is not registered
Review Comment:
```suggestion
/// \brief Check whether an alias can be added for the given function name.
/// \return Status::KeyError if a function with the
/// given source_name is not registered
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
+ }
- const std::string name = options_type->type_name();
- auto it = name_to_options_type_.find(name);
- if (it != name_to_options_type_.end() && !allow_overwrite) {
- return Status::KeyError(
- "Already have a function options type registered with name: ", name);
- }
- name_to_options_type_[name] = options_type;
- return Status::OK();
+ virtual Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/true);
}
- Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
+ virtual Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
auto it = name_to_function_.find(name);
if (it == name_to_function_.end()) {
+ if (parent_ != nullptr) {
+ return parent_->GetFunction(name);
+ }
return Status::KeyError("No function registered with name: ", name);
}
return it->second;
}
- std::vector<std::string> GetFunctionNames() const {
+ virtual std::vector<std::string> GetFunctionNames() const {
std::vector<std::string> results;
+ if (parent_ != nullptr) {
+ results = parent_->GetFunctionNames();
+ }
for (auto it : name_to_function_) {
results.push_back(it.first);
}
std::sort(results.begin(), results.end());
return results;
}
- Result<const FunctionOptionsType*> GetFunctionOptionsType(
+ virtual Result<const FunctionOptionsType*> GetFunctionOptionsType(
const std::string& name) const {
auto it = name_to_options_type_.find(name);
if (it == name_to_options_type_.end()) {
+ if (parent_ != nullptr) {
+ return parent_->GetFunctionOptionsType(name);
+ }
return Status::KeyError("No function options type registered with name: ", name);
}
return it->second;
}
- int num_functions() const { return static_cast<int>(name_to_function_.size()); }
+ virtual int num_functions() const {
+ return (parent_ == nullptr ? 0 : parent_->num_functions()) +
+ static_cast<int>(name_to_function_.size());
+ }
private:
+ Status DoAddFunction(std::shared_ptr<Function> function, bool allow_overwrite,
+ bool add) {
+#ifndef NDEBUG
+ // This validates docstrings extensively, so don't waste time on it
+ // in release builds.
+ RETURN_NOT_OK(function->Validate());
+#endif
+
+ std::lock_guard<std::mutex> mutation_guard(lock_);
+
+ const std::string& name = function->name();
+ auto it = name_to_function_.find(name);
+ if (it != name_to_function_.end() && !allow_overwrite) {
+ return Status::KeyError("Already have a function registered with name: ", name);
+ }
+ if (add) {
+ name_to_function_[name] = std::move(function);
+ }
+ return Status::OK();
+ }
+
+ Status DoAddAlias(const std::string& target_name, const std::string& source_name,
+ bool add) {
+ std::lock_guard<std::mutex> mutation_guard(lock_);
+
+ // following invocation must not acquire the mutex
+ ARROW_ASSIGN_OR_RAISE(auto func, GetFunction(source_name));
+ if (add) {
Review Comment:
Should we be doing a check here to see if a function with `target_name` is already registered (would probably also necessitate `allow_overwrite`)?
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -45,20 +45,42 @@ class FunctionOptionsType;
/// lower-level function execution.
class ARROW_EXPORT FunctionRegistry {
public:
- ~FunctionRegistry();
+ virtual ~FunctionRegistry();
/// \brief Construct a new registry. Most users only need to use the global
/// registry
static std::unique_ptr<FunctionRegistry> Make();
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(std::unique_ptr<FunctionRegistry> parent);
+
+ /// \brief Checks whether a new function can be added to the registry. Returns
+ /// Status::KeyError if a function with the same name is already registered
Review Comment:
```suggestion
/// \brief Check whether a new function can be added to the registry.
/// \return Status::KeyError if a function with the same name is already registered
```
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -45,20 +45,42 @@ class FunctionOptionsType;
/// lower-level function execution.
class ARROW_EXPORT FunctionRegistry {
public:
- ~FunctionRegistry();
+ virtual ~FunctionRegistry();
/// \brief Construct a new registry. Most users only need to use the global
/// registry
static std::unique_ptr<FunctionRegistry> Make();
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Construct a new nested registry with the given parent. Most users only need
+ /// to use the global registry
+ static std::unique_ptr<FunctionRegistry> Make(std::unique_ptr<FunctionRegistry> parent);
+
+ /// \brief Checks whether a new function can be added to the registry. Returns
+ /// Status::KeyError if a function with the same name is already registered
+ Status CanAddFunction(std::shared_ptr<Function> function, bool allow_overwrite = false);
+
/// \brief Add a new function to the registry. Returns Status::KeyError if a
/// function with the same name is already registered
Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite = false);
- /// \brief Add aliases for the given function name. Returns Status::KeyError if the
+ /// \brief Checks whether an alias can be added for the given function name. Returns
+ /// Status::KeyError if the function with the given name is not registered
+ Status CanAddAlias(const std::string& target_name, const std::string& source_name);
+
+ /// \brief Add alias for the given function name. Returns Status::KeyError if the
/// function with the given name is not registered
Review Comment:
```suggestion
/// \brief Add alias for the given function name.
/// \return Status::KeyError if the
/// function with the given source_name is not registered
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
Review Comment:
```suggestion
if (parent_ != nullptr) {
RETURN_NOT_OK(parent_->CanAddFunction(function, allow_overwrite));
}
return DoAddFunction(function, allow_overwrite, /*add=*/true);
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
Review Comment:
```suggestion
Status CanAddFunction(std::shared_ptr<Function> function,
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
Review Comment:
```suggestion
RETURN_NOT_OK(DoAddAlias(target_name, source_name, /*add=*/false));
if (parent_ != nullptr) {
return parent_->CanAddAlias(target_name, source_name);
}
return Status::OK();
```
Hmm...I think we want to:
* Check if source_name exists in this and fail otherwise
* Check if target_name exists in parent_ and fail otherwise
* Actually try and add to this (may fail if target_name exists)
I might be confused though.
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
Review Comment:
```suggestion
Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
Review Comment:
```suggestion
Status CanAddAlias(const std::string& target_name,
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
Review Comment:
```suggestion
~FunctionRegistryImpl() {}
```
Now that we aren't using inheritance these do not need to be virtual.
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
Review Comment:
I'm a little confused here. Why are we adding the alias to `parent_` as well?
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
Review Comment:
```suggestion
if (parent_ != nullptr) {
RETURN_NOT_OK(parent_->CanAddFunctionOptionsType(options_type, allow_overwrite));
}
return DoAddFunctionsType(options_type, allow_overwrite, /*add=*/false);
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
+ }
- const std::string name = options_type->type_name();
- auto it = name_to_options_type_.find(name);
- if (it != name_to_options_type_.end() && !allow_overwrite) {
- return Status::KeyError(
- "Already have a function options type registered with name: ", name);
- }
- name_to_options_type_[name] = options_type;
- return Status::OK();
+ virtual Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/true);
}
- Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
+ virtual Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
Review Comment:
```suggestion
Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
+ }
- const std::string name = options_type->type_name();
- auto it = name_to_options_type_.find(name);
- if (it != name_to_options_type_.end() && !allow_overwrite) {
- return Status::KeyError(
- "Already have a function options type registered with name: ", name);
- }
- name_to_options_type_[name] = options_type;
- return Status::OK();
+ virtual Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
Review Comment:
```suggestion
Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -115,20 +181,47 @@ std::unique_ptr<FunctionRegistry> FunctionRegistry::Make() {
return std::unique_ptr<FunctionRegistry>(new FunctionRegistry());
}
-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_)));
+}
+
+std::unique_ptr<FunctionRegistry> FunctionRegistry::Make(
+ std::unique_ptr<FunctionRegistry> parent) {
+ return FunctionRegistry::Make(&*parent);
+}
+
Review Comment:
```suggestion
```
As explained in the header file.
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
+ }
- const std::string name = options_type->type_name();
- auto it = name_to_options_type_.find(name);
- if (it != name_to_options_type_.end() && !allow_overwrite) {
- return Status::KeyError(
- "Already have a function options type registered with name: ", name);
- }
- name_to_options_type_[name] = options_type;
- return Status::OK();
+ virtual Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/true);
}
- Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
+ virtual Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
auto it = name_to_function_.find(name);
if (it == name_to_function_.end()) {
+ if (parent_ != nullptr) {
+ return parent_->GetFunction(name);
+ }
return Status::KeyError("No function registered with name: ", name);
}
return it->second;
}
- std::vector<std::string> GetFunctionNames() const {
+ virtual std::vector<std::string> GetFunctionNames() const {
std::vector<std::string> results;
+ if (parent_ != nullptr) {
+ results = parent_->GetFunctionNames();
+ }
for (auto it : name_to_function_) {
results.push_back(it.first);
}
std::sort(results.begin(), results.end());
return results;
}
- Result<const FunctionOptionsType*> GetFunctionOptionsType(
+ virtual Result<const FunctionOptionsType*> GetFunctionOptionsType(
Review Comment:
```suggestion
Result<const FunctionOptionsType*> GetFunctionOptionsType(
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
+ }
- const std::string name = options_type->type_name();
- auto it = name_to_options_type_.find(name);
- if (it != name_to_options_type_.end() && !allow_overwrite) {
- return Status::KeyError(
- "Already have a function options type registered with name: ", name);
- }
- name_to_options_type_[name] = options_type;
- return Status::OK();
+ virtual Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/true);
}
- Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
+ virtual Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
auto it = name_to_function_.find(name);
if (it == name_to_function_.end()) {
+ if (parent_ != nullptr) {
+ return parent_->GetFunction(name);
+ }
return Status::KeyError("No function registered with name: ", name);
}
return it->second;
}
- std::vector<std::string> GetFunctionNames() const {
+ virtual std::vector<std::string> GetFunctionNames() const {
std::vector<std::string> results;
+ if (parent_ != nullptr) {
+ results = parent_->GetFunctionNames();
+ }
for (auto it : name_to_function_) {
results.push_back(it.first);
}
std::sort(results.begin(), results.end());
return results;
}
- Result<const FunctionOptionsType*> GetFunctionOptionsType(
+ virtual Result<const FunctionOptionsType*> GetFunctionOptionsType(
const std::string& name) const {
auto it = name_to_options_type_.find(name);
if (it == name_to_options_type_.end()) {
+ if (parent_ != nullptr) {
+ return parent_->GetFunctionOptionsType(name);
+ }
return Status::KeyError("No function options type registered with name: ", name);
}
return it->second;
}
- int num_functions() const { return static_cast<int>(name_to_function_.size()); }
+ virtual int num_functions() const {
Review Comment:
```suggestion
int num_functions() const {
```
##########
cpp/src/arrow/compute/registry.cc:
##########
@@ -34,78 +34,144 @@ namespace compute {
class FunctionRegistry::FunctionRegistryImpl {
public:
- Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
-#ifndef NDEBUG
- // This validates docstrings extensively, so don't waste time on it
- // in release builds.
- RETURN_NOT_OK(function->Validate());
-#endif
-
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ explicit FunctionRegistryImpl(FunctionRegistryImpl* parent = NULLPTR)
+ : parent_(parent) {}
+ virtual ~FunctionRegistryImpl() {}
+
+ virtual Status CanAddFunction(std::shared_ptr<Function> function,
+ bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/false);
+ }
- const std::string& name = function->name();
- auto it = name_to_function_.find(name);
- if (it != name_to_function_.end() && !allow_overwrite) {
- return Status::KeyError("Already have a function registered with name: ", name);
- }
- name_to_function_[name] = std::move(function);
- return Status::OK();
+ virtual Status AddFunction(std::shared_ptr<Function> function, bool allow_overwrite) {
+ return (parent_ == nullptr ? Status::OK()
+ : parent_->CanAddFunction(function, allow_overwrite)) &
+ DoAddFunction(function, allow_overwrite, /*add=*/true);
}
- Status AddAlias(const std::string& target_name, const std::string& source_name) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/false);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->CanAddAlias(target_name, source_name);
+ }
- auto it = name_to_function_.find(source_name);
- if (it == name_to_function_.end()) {
- return Status::KeyError("No function registered with name: ", source_name);
- }
- name_to_function_[target_name] = it->second;
- return Status::OK();
+ virtual Status AddAlias(const std::string& target_name,
+ const std::string& source_name) {
+ Status st = DoAddAlias(target_name, source_name, /*add=*/true);
+ return st.ok() || parent_ == nullptr ? st
+ : parent_->AddAlias(target_name, source_name);
}
- Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
- bool allow_overwrite = false) {
- std::lock_guard<std::mutex> mutation_guard(lock_);
+ virtual Status CanAddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/false);
+ }
- const std::string name = options_type->type_name();
- auto it = name_to_options_type_.find(name);
- if (it != name_to_options_type_.end() && !allow_overwrite) {
- return Status::KeyError(
- "Already have a function options type registered with name: ", name);
- }
- name_to_options_type_[name] = options_type;
- return Status::OK();
+ virtual Status AddFunctionOptionsType(const FunctionOptionsType* options_type,
+ bool allow_overwrite = false) {
+ return (parent_ == nullptr
+ ? Status::OK()
+ : parent_->CanAddFunctionOptionsType(options_type, allow_overwrite)) &
+ DoAddFunctionOptionsType(options_type, allow_overwrite, /*add=*/true);
}
- Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
+ virtual Result<std::shared_ptr<Function>> GetFunction(const std::string& name) const {
auto it = name_to_function_.find(name);
if (it == name_to_function_.end()) {
+ if (parent_ != nullptr) {
+ return parent_->GetFunction(name);
+ }
return Status::KeyError("No function registered with name: ", name);
}
return it->second;
}
- std::vector<std::string> GetFunctionNames() const {
+ virtual std::vector<std::string> GetFunctionNames() const {
Review Comment:
```suggestion
std::vector<std::string> GetFunctionNames() const {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rtpsw commented on pull request #13252: ARROW-16677: [C++] Support nesting of function registries
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13252:
URL: https://github.com/apache/arrow/pull/13252#issuecomment-1139429871
cc @icexelloss
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org