[GitHub] [arrow] rtpsw commented on a diff in pull request #13613: ARROW-15582: [C++] Add support for registering standard Substrait functions

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

rtpsw commented on code in PR #13613:
URL: https://github.com/apache/arrow/pull/13613#discussion_r923491522


##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -111,7 +113,7 @@ ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlans(
 /// Plan is returned here.
 /// \return a vector of ExecNode declarations, one for each toplevel relation in the
 /// Substrait Plan
-ARROW_ENGINE_EXPORT Result<compute::ExecPlan> DeserializePlan(
+ARROW_ENGINE_EXPORT Result<std::shared_ptr<compute::ExecPlan>> DeserializePlan(

Review Comment:
   The number of arguments of this function is rapidly rising, and I think it would be better to place them in an options struct, though that would impact PyArrow's code. I'll leave it to you to decide whether to do it in this PR.



##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -703,6 +703,11 @@ TEST(Substrait, ExtensionSetFromPlan) {
         "extension_uri_anchor": 7,
         "uri": ")" + substrait::default_extension_types_uri() +
                                R"("
+      },
+      {
+        "extension_uri_anchor": 18,
+        "uri": ")" + std::string(kSubstraitArithmeticFunctionsUri) +

Review Comment:
   I think it would be useful, at least for PyArrow's sake, to expose the string value of this constant (and similar ones) like `default_extension_types_uri()` does.



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