You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/17 23:02:59 UTC
[GitHub] [arrow] sanjibansg opened a new pull request, #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
sanjibansg opened a new pull request, #13181:
URL: https://github.com/apache/arrow/pull/13181
This PR adds a convenience function to Substrait consumer for the `DeserializePlan` method, which creates ExecPlan for single root relations.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13181:
URL: https://github.com/apache/arrow/pull/13181#issuecomment-1129401741
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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
[GitHub] [arrow] westonpace commented on a diff in pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13181:
URL: https://github.com/apache/arrow/pull/13181#discussion_r877207840
##########
cpp/src/arrow/engine/substrait/serde.cc:
##########
@@ -86,6 +86,20 @@ Result<std::vector<compute::Declaration>> DeserializePlan(
return sink_decls;
}
+Result<compute::ExecPlan> DeserializePlan(const Buffer& buf,
+ const ConsumerFactory& consumer_factory,
+ ExtensionSet* ext_set_out) {
+ ARROW_ASSIGN_OR_RAISE(auto declarations,
+ DeserializePlans(buf, consumer_factory, ext_set_out));
+ if (declarations.size() > 1) {
+ return Status::Invalid("DeserializePlan does not support root relations");
Review Comment:
```suggestion
return Status::Invalid("DeserializePlan does not support multiple root relations");
```
--
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
[GitHub] [arrow] sanjibansg commented on a diff in pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
sanjibansg commented on code in PR #13181:
URL: https://github.com/apache/arrow/pull/13181#discussion_r877169545
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -48,9 +48,12 @@ using ConsumerFactory = std::function<std::shared_ptr<compute::SinkNodeConsumer>
/// returned here.
/// \return a vector of ExecNode declarations, one for each toplevel relation in the
/// Substrait Plan
-ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlan(
- const Buffer& buf, const ConsumerFactory& consumer_factory,
- ExtensionSet* ext_set = NULLPTR);
+ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlans(
+ const Buffer& buf, const ConsumerFactory& consumer_factory, ExtensionSet* ext_set);
Review Comment:
Made the change.
--
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
[GitHub] [arrow] sanjibansg commented on a diff in pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
sanjibansg commented on code in PR #13181:
URL: https://github.com/apache/arrow/pull/13181#discussion_r877169926
##########
cpp/src/arrow/engine/substrait/serde.cc:
##########
@@ -86,6 +86,20 @@ Result<std::vector<compute::Declaration>> DeserializePlan(
return sink_decls;
}
+Result<compute::ExecPlan> DeserializePlan(const Buffer& buf,
+ const ConsumerFactory& consumer_factory,
+ ExtensionSet* ext_set_out) {
Review Comment:
Renamed ext_set to ext_set_out.
--
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
[GitHub] [arrow] sanjibansg commented on a diff in pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
sanjibansg commented on code in PR #13181:
URL: https://github.com/apache/arrow/pull/13181#discussion_r877169926
##########
cpp/src/arrow/engine/substrait/serde.cc:
##########
@@ -86,6 +86,20 @@ Result<std::vector<compute::Declaration>> DeserializePlan(
return sink_decls;
}
+Result<compute::ExecPlan> DeserializePlan(const Buffer& buf,
+ const ConsumerFactory& consumer_factory,
+ ExtensionSet* ext_set_out) {
Review Comment:
Renamed `ext_set` to `ext_set_out`.
--
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
[GitHub] [arrow] sanjibansg commented on a diff in pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
sanjibansg commented on code in PR #13181:
URL: https://github.com/apache/arrow/pull/13181#discussion_r877218762
##########
cpp/src/arrow/engine/substrait/serde.cc:
##########
@@ -86,6 +86,20 @@ Result<std::vector<compute::Declaration>> DeserializePlan(
return sink_decls;
}
+Result<compute::ExecPlan> DeserializePlan(const Buffer& buf,
+ const ConsumerFactory& consumer_factory,
+ ExtensionSet* ext_set_out) {
+ ARROW_ASSIGN_OR_RAISE(auto declarations,
+ DeserializePlans(buf, consumer_factory, ext_set_out));
+ if (declarations.size() > 1) {
+ return Status::Invalid("DeserializePlan does not support root relations");
Review Comment:
Corrected that, thanks!
--
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
[GitHub] [arrow] westonpace closed pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
westonpace closed pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
URL: https://github.com/apache/arrow/pull/13181
--
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
[GitHub] [arrow] ursabot commented on pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13181:
URL: https://github.com/apache/arrow/pull/13181#issuecomment-1133398870
Benchmark runs are scheduled for baseline = b7507c34b71a200c9f08597b83e935d1639dd85c and contender = cc2265a358717f0039e428b4c8b217153dee54f0. cc2265a358717f0039e428b4c8b217153dee54f0 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/140c678f28cd4fa081ce3c4c4d4a495b...20166efbdc284bf1b013e0872c55b9ee/)
[Failed :arrow_down:0.97% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/32575ffbcd62405fa9532b03fc81091f...a2279731108646f99be8d8b5fef27ced/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f4ea1aed7cfc4fbe8822c814f45bc1ac...2b7aeda108c248cabf93affeffe821ce/)
[Finished :arrow_down:0.2% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/e1161aca9aeb4425af3704674c38692e...b4012d14d621454780e7e47267007e94/)
Buildkite builds:
[Finished] [`cc2265a3` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/805)
[Failed] [`cc2265a3` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/802)
[Failed] [`cc2265a3` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/792)
[Finished] [`cc2265a3` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/808)
[Finished] [`b7507c34` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/804)
[Failed] [`b7507c34` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/801)
[Failed] [`b7507c34` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/791)
[Finished] [`b7507c34` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/807)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] sanjibansg commented on a diff in pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
sanjibansg commented on code in PR #13181:
URL: https://github.com/apache/arrow/pull/13181#discussion_r877169225
##########
cpp/src/arrow/engine/substrait/serde.cc:
##########
@@ -86,6 +86,20 @@ Result<std::vector<compute::Declaration>> DeserializePlan(
return sink_decls;
}
+Result<compute::ExecPlan> DeserializePlan(const Buffer& buf,
+ const ConsumerFactory& consumer_factory,
+ ExtensionSet* ext_set_out) {
+ ARROW_ASSIGN_OR_RAISE(auto declarations,
+ DeserializePlans(buf, consumer_factory, ext_set_out));
+ if (declarations.size() > 1) {
+ return Status::Invalid("Substrait plan cannot have multiple root relations");
Review Comment:
Yes, right, made the change, thanks!
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13181:
URL: https://github.com/apache/arrow/pull/13181#issuecomment-1129401726
https://issues.apache.org/jira/browse/ARROW-15534
--
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
[GitHub] [arrow] westonpace commented on a diff in pull request #13181: ARROW-15534: [C++] Add convenience function to substrait consumer to create plan instead of declaration
Posted by GitBox <gi...@apache.org>.
westonpace commented on code in PR #13181:
URL: https://github.com/apache/arrow/pull/13181#discussion_r877107802
##########
cpp/src/arrow/engine/substrait/serde.cc:
##########
@@ -86,6 +86,20 @@ Result<std::vector<compute::Declaration>> DeserializePlan(
return sink_decls;
}
+Result<compute::ExecPlan> DeserializePlan(const Buffer& buf,
+ const ConsumerFactory& consumer_factory,
+ ExtensionSet* ext_set_out) {
+ ARROW_ASSIGN_OR_RAISE(auto declarations,
+ DeserializePlans(buf, consumer_factory, ext_set_out));
+ if (declarations.size() > 1) {
+ return Status::Invalid("Substrait plan cannot have multiple root relations");
Review Comment:
```suggestion
return Status::Invalid("DeserializePlan does not support root relations");
```
Technically a Substrait plan can have multiple root relations. We just don't support those plans in this method.
##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -48,9 +48,12 @@ using ConsumerFactory = std::function<std::shared_ptr<compute::SinkNodeConsumer>
/// returned here.
/// \return a vector of ExecNode declarations, one for each toplevel relation in the
/// Substrait Plan
-ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlan(
- const Buffer& buf, const ConsumerFactory& consumer_factory,
- ExtensionSet* ext_set = NULLPTR);
+ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlans(
+ const Buffer& buf, const ConsumerFactory& consumer_factory, ExtensionSet* ext_set);
Review Comment:
```suggestion
const Buffer& buf, const ConsumerFactory& consumer_factory, ExtensionSet* ext_set = NULLPTR);
```
I think we still need the default value here.
##########
cpp/src/arrow/engine/substrait/serde.cc:
##########
@@ -86,6 +86,20 @@ Result<std::vector<compute::Declaration>> DeserializePlan(
return sink_decls;
}
+Result<compute::ExecPlan> DeserializePlan(const Buffer& buf,
+ const ConsumerFactory& consumer_factory,
+ ExtensionSet* ext_set_out) {
Review Comment:
I do like the name `ext_set_out` more than `ext_set`. Could you update the name in the header as well so they match?
--
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