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/10/23 07:20:06 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
rtpsw opened a new pull request, #14480:
URL: https://github.com/apache/arrow/pull/14480
See https://issues.apache.org/jira/browse/ARROW-18135
--
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 #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14480:
URL: https://github.com/apache/arrow/pull/14480#issuecomment-1288034727
https://issues.apache.org/jira/browse/ARROW-18135
--
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] rtpsw commented on a diff in pull request #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #14480:
URL: https://github.com/apache/arrow/pull/14480#discussion_r1004481381
##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -602,7 +602,9 @@ Future<std::vector<ExecBatch>> DeclarationToExecBatchesAsync(Declaration declara
.Then([collected_fut, exec_plan]() -> Result<std::vector<ExecBatch>> {
ARROW_ASSIGN_OR_RAISE(auto collected, collected_fut.result());
return ::arrow::internal::MapVector(
- [](std::optional<ExecBatch> batch) { return std::move(*batch); },
+ [](std::optional<ExecBatch> batch) {
+ return ARROW_PREDICT_TRUE(batch) ? std::move(*batch) : ExecBatch();
Review Comment:
Does `value_or` use something like `ARROW_PREDICT_TRUE` underneath? Granted, the code changes here are not in performance-critical code, though they such code may follow the example set here. Bottom line, if you want the conciseness change, I'd be OK doing it.
--
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] pitrou commented on a diff in pull request #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14480:
URL: https://github.com/apache/arrow/pull/14480#discussion_r1004535775
##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -602,7 +602,9 @@ Future<std::vector<ExecBatch>> DeclarationToExecBatchesAsync(Declaration declara
.Then([collected_fut, exec_plan]() -> Result<std::vector<ExecBatch>> {
ARROW_ASSIGN_OR_RAISE(auto collected, collected_fut.result());
return ::arrow::internal::MapVector(
- [](std::optional<ExecBatch> batch) { return std::move(*batch); },
+ [](std::optional<ExecBatch> batch) {
+ return ARROW_PREDICT_TRUE(batch) ? std::move(*batch) : ExecBatch();
Review Comment:
> Does value_or use something like ARROW_PREDICT_TRUE underneath?
Probably not, but that probably doesn't matter either.
--
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] rtpsw commented on pull request #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #14480:
URL: https://github.com/apache/arrow/pull/14480#issuecomment-1289311266
@lidavidm, would you be able to review this, once the release work is done?
--
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] pitrou merged pull request #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
Posted by GitBox <gi...@apache.org>.
pitrou merged PR #14480:
URL: https://github.com/apache/arrow/pull/14480
--
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] pitrou commented on a diff in pull request #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14480:
URL: https://github.com/apache/arrow/pull/14480#discussion_r1004119438
##########
cpp/src/arrow/compute/exec/exec_plan.cc:
##########
@@ -602,7 +602,9 @@ Future<std::vector<ExecBatch>> DeclarationToExecBatchesAsync(Declaration declara
.Then([collected_fut, exec_plan]() -> Result<std::vector<ExecBatch>> {
ARROW_ASSIGN_OR_RAISE(auto collected, collected_fut.result());
return ::arrow::internal::MapVector(
- [](std::optional<ExecBatch> batch) { return std::move(*batch); },
+ [](std::optional<ExecBatch> batch) {
+ return ARROW_PREDICT_TRUE(batch) ? std::move(*batch) : ExecBatch();
Review Comment:
This is fine with me, but could also use https://en.cppreference.com/w/cpp/utility/optional/value_or for concision.
--
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 #14480: ARROW-18135: [C++] Avoid warnings that ExecBatch::length may be uninitialized
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14480:
URL: https://github.com/apache/arrow/pull/14480#issuecomment-1295064155
Benchmark runs are scheduled for baseline = ede0b594468b997212f599455620649925eab8fd and contender = 53023b3b8634f22f5ec8fd7b7035d98ed54f315e. 53023b3b8634f22f5ec8fd7b7035d98ed54f315e 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/95b2f0d82df6491f98799c7034c19a90...e95aafef15ed49b9a3c272e96e6d2d76/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/efc7ec36e2f84cf495e26c7d8fba271a...a4caeb72a9b442999ad644cc4287b6fd/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/6e557d94d5884802a941f7f779bd0b1b...cdae57d0e81e48d68d7c7622e3f246d0/)
[Finished :arrow_down:0.61% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/141a089661e84d32940c13d85c1a46fe...aabe63f542634dea877f79c7d91e3fa6/)
Buildkite builds:
[Finished] [`53023b3b` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1766)
[Failed] [`53023b3b` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1787)
[Finished] [`53023b3b` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1753)
[Finished] [`53023b3b` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1779)
[Finished] [`ede0b594` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1765)
[Failed] [`ede0b594` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1786)
[Finished] [`ede0b594` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1752)
[Finished] [`ede0b594` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1778)
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