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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
