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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
