[GitHub] [arrow] westonpace commented on a diff in pull request #13214: ARROW-15635: [C++] Support nested extension-id-registry

[GitBox <gi...@apache.org>]

westonpace commented on code in PR #13214:
URL: https://github.com/apache/arrow/pull/13214#discussion_r881370183


##########
cpp/src/arrow/engine/substrait/extension_set.h:
##########
@@ -224,7 +240,7 @@ class ARROW_ENGINE_EXPORT ExtensionSet {
   std::size_t num_functions() const { return functions_.size(); }
 
  private:
-  ExtensionIdRegistry* registry_;
+  const ExtensionIdRegistry* registry_;

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



##########
cpp/src/arrow/engine/substrait/extension_set.h:
##########
@@ -84,6 +86,7 @@ class ARROW_ENGINE_EXPORT ExtensionIdRegistry {
   virtual util::optional<FunctionRecord> GetFunction(Id) const = 0;
   virtual util::optional<FunctionRecord> GetFunction(
       util::string_view arrow_function_name) const = 0;
+  virtual Status CanRegisterFunction(Id, std::string arrow_function_name) const = 0;

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



##########
cpp/src/arrow/engine/substrait/extension_set.h:
##########
@@ -63,6 +63,8 @@ class ARROW_ENGINE_EXPORT ExtensionIdRegistry {
   };
   virtual util::optional<TypeRecord> GetType(const DataType&) const = 0;
   virtual util::optional<TypeRecord> GetType(Id, bool is_variation) const = 0;
+  virtual Status CanRegisterType(Id, std::shared_ptr<DataType> type,

Review Comment:
   ```suggestion
     virtual Status CanRegisterType(Id, const std::shared_ptr<DataType>& type,
   ```



##########
cpp/src/arrow/engine/substrait/util.h:
##########
@@ -37,6 +37,8 @@ ARROW_ENGINE_EXPORT Result<std::shared_ptr<RecordBatchReader>> ExecuteSerialized
 ARROW_ENGINE_EXPORT Result<std::shared_ptr<Buffer>> SerializeJsonPlan(
     const std::string& substrait_json);
 
+ARROW_ENGINE_EXPORT std::shared_ptr<ExtensionIdRegistry> MakeExtensionIdRegistry();

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



-- 
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