lidavidm commented on code in PR #13375:
URL: https://github.com/apache/arrow/pull/13375#discussion_r896742518
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -47,35 +47,64 @@ class ARROW_EXPORT FunctionRegistry {
public:
~FunctionRegistry();
- /// \brief Construct a new registry. Most users only need to use the global
- /// registry
+ /// \brief Construct a new registry.
+ ///
+ /// Most users only need to use the global registry.
static std::unique_ptr<FunctionRegistry> Make();
- /// \brief Add a new function to the registry. Returns Status::KeyError if a
- /// function with the same name is already registered
+ /// \brief Construct a new nested registry with the given parent.
+ ///
+ /// Most users only need to use the global registry. The returned registry
never changes
+ /// its parent, even when an operation allows overwritting.
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Check 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
- /// function with the given name is not registered
+ /// \brief Check 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);
Review Comment:
Should this return `bool`?
##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -116,7 +119,7 @@ Result<compute::Declaration> FromProto(const
substrait::Rel& rel,
} else {
return Status::NotImplemented(
"substrait::ReadRel::LocalFiles::FileOrFiles::format "
- "other than FILE_FORMAT_PARQUET");
+ "other than FILE_FORMAT_PARQUET and not recognized");
Review Comment:
```suggestion
"other than FILE_FORMAT_PARQUET is not recognized");
```
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -47,35 +47,64 @@ class ARROW_EXPORT FunctionRegistry {
public:
~FunctionRegistry();
- /// \brief Construct a new registry. Most users only need to use the global
- /// registry
+ /// \brief Construct a new registry.
+ ///
+ /// Most users only need to use the global registry.
static std::unique_ptr<FunctionRegistry> Make();
- /// \brief Add a new function to the registry. Returns Status::KeyError if a
- /// function with the same name is already registered
+ /// \brief Construct a new nested registry with the given parent.
+ ///
+ /// Most users only need to use the global registry. The returned registry
never changes
+ /// its parent, even when an operation allows overwritting.
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Check 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);
Review Comment:
nit: take `const std::shared_ptr<Function>&` since this is a check? Also, I
would expect this to return `bool` and be declared `const`
##########
cpp/src/arrow/compute/registry.h:
##########
@@ -47,35 +47,64 @@ class ARROW_EXPORT FunctionRegistry {
public:
~FunctionRegistry();
- /// \brief Construct a new registry. Most users only need to use the global
- /// registry
+ /// \brief Construct a new registry.
+ ///
+ /// Most users only need to use the global registry.
static std::unique_ptr<FunctionRegistry> Make();
- /// \brief Add a new function to the registry. Returns Status::KeyError if a
- /// function with the same name is already registered
+ /// \brief Construct a new nested registry with the given parent.
+ ///
+ /// Most users only need to use the global registry. The returned registry
never changes
+ /// its parent, even when an operation allows overwritting.
+ static std::unique_ptr<FunctionRegistry> Make(FunctionRegistry* parent);
+
+ /// \brief Check 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
- /// function with the given name is not registered
+ /// \brief Check 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 Add a new function options type to the registry. Returns
Status::KeyError if
- /// a function options type with the same name is already registered
+ /// \brief Check 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
Review Comment:
Note it's `\return` in Doxygen
##########
cpp/src/arrow/engine/substrait/util.h:
##########
@@ -30,17 +31,37 @@ namespace substrait {
/// \brief Retrieve a RecordBatchReader from a Substrait plan.
ARROW_ENGINE_EXPORT Result<std::shared_ptr<RecordBatchReader>>
ExecuteSerializedPlan(
- const Buffer& substrait_buffer);
+ const Buffer& substrait_buffer, const ExtensionIdRegistry* registry =
NULLPTR,
+ compute::FunctionRegistry* func_registry = NULLPTR);
/// \brief Get a Serialized Plan from a Substrait JSON plan.
/// This is a helper method for Python tests.
ARROW_ENGINE_EXPORT Result<std::shared_ptr<Buffer>> SerializeJsonPlan(
const std::string& substrait_json);
+ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlans(
Review Comment:
nit: docstring?
##########
cpp/src/arrow/engine/substrait/util.cc:
##########
@@ -68,7 +68,7 @@ class SubstraitExecutor {
compute::ExecContext exec_context)
: plan_(std::move(plan)), exec_context_(exec_context) {}
- ~SubstraitExecutor() { ARROW_CHECK_OK(this->Close()); }
+ ~SubstraitExecutor() { ARROW_UNUSED(this->Close()); }
Review Comment:
Hmm, this should either `CHECK` or it should warn if not OK, but we
shouldn't swallow the error.
##########
cpp/src/arrow/compute/registry_test.cc:
##########
@@ -85,5 +96,139 @@ 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;
Review Comment:
nit: why is this test repeated?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
