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 2021/11/08 14:07:51 UTC
[GitHub] [arrow] lidavidm opened a new pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
lidavidm opened a new pull request #11640:
URL: https://github.com/apache/arrow/pull/11640
This fixes two issues:
- GroupByNode would try to finish its future twice
- GroupByNode wouldn't set the length of the key column batch, which broke down when using a scalar column as the key
--
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 change in pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#discussion_r745043272
##########
File path: cpp/src/arrow/compute/exec/aggregate_node.cc
##########
@@ -525,9 +525,8 @@ class GroupByNode : public ExecNode {
void StopProducing(ExecNode* output) override {
DCHECK_EQ(output, outputs_[0]);
- if (input_counter_.Cancel()) {
- finished_.MarkFinished();
- } else if (output_counter_.Cancel()) {
+ ARROW_UNUSED(input_counter_.Cancel());
Review comment:
Looks good. I don't know if you even need to call `input_counter_.Cancel()` here but I also suppose it can't hurt and maybe it will save some extra work.
--
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 edited a comment on pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#issuecomment-963568846
Benchmark runs are scheduled for baseline = 187211dedc83ac378c124b0cc7ed916872cc8174 and contender = fa6b0aab40eb910514db78cd1d641af1fa3bab35. fa6b0aab40eb910514db78cd1d641af1fa3bab35 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/27b57c800c0a4f92adbf69290d0c1f56...8553b34e1a954dce8ace157e8036d7dc/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/eebdbca358e8406cb44f801b8afbc94a...27ba597f3d754825b76429abbd7b32f7/)
[Finished :arrow_down:0.45% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8f1191d42d8c4b578d1e14463c095c18...a350cf3949e24f648d22792df837bb8b/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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 edited a comment on pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#issuecomment-963568846
Benchmark runs are scheduled for baseline = 187211dedc83ac378c124b0cc7ed916872cc8174 and contender = fa6b0aab40eb910514db78cd1d641af1fa3bab35. fa6b0aab40eb910514db78cd1d641af1fa3bab35 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/27b57c800c0a4f92adbf69290d0c1f56...8553b34e1a954dce8ace157e8036d7dc/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/eebdbca358e8406cb44f801b8afbc94a...27ba597f3d754825b76429abbd7b32f7/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8f1191d42d8c4b578d1e14463c095c18...a350cf3949e24f648d22792df837bb8b/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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 edited a comment on pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#issuecomment-963568846
Benchmark runs are scheduled for baseline = 187211dedc83ac378c124b0cc7ed916872cc8174 and contender = fa6b0aab40eb910514db78cd1d641af1fa3bab35. fa6b0aab40eb910514db78cd1d641af1fa3bab35 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/27b57c800c0a4f92adbf69290d0c1f56...8553b34e1a954dce8ace157e8036d7dc/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/eebdbca358e8406cb44f801b8afbc94a...27ba597f3d754825b76429abbd7b32f7/)
[Finished :arrow_down:0.45% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8f1191d42d8c4b578d1e14463c095c18...a350cf3949e24f648d22792df837bb8b/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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 change in pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#discussion_r744793707
##########
File path: cpp/src/arrow/compute/exec/plan_test.cc
##########
@@ -607,6 +607,42 @@ TEST(ExecPlanExecution, StressSourceOrderBy) {
}
}
+TEST(ExecPlanExecution, StressSourceGroupedSumStop) {
+ auto input_schema = schema({field("a", int32()), field("b", boolean())});
+ for (bool slow : {false, true}) {
+ SCOPED_TRACE(slow ? "slowed" : "unslowed");
+
+ for (bool parallel : {false, true}) {
+ SCOPED_TRACE(parallel ? "parallel" : "single threaded");
+
+ int num_batches = slow && !parallel ? 30 : 300;
Review comment:
Nit: add parentheses around `slow && !parallel`?
--
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] lidavidm closed pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #11640:
URL: https://github.com/apache/arrow/pull/11640
--
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] lidavidm commented on a change in pull request #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#discussion_r744799827
##########
File path: cpp/src/arrow/compute/exec/plan_test.cc
##########
@@ -607,6 +607,42 @@ TEST(ExecPlanExecution, StressSourceOrderBy) {
}
}
+TEST(ExecPlanExecution, StressSourceGroupedSumStop) {
+ auto input_schema = schema({field("a", int32()), field("b", boolean())});
+ for (bool slow : {false, true}) {
+ SCOPED_TRACE(slow ? "slowed" : "unslowed");
+
+ for (bool parallel : {false, true}) {
+ SCOPED_TRACE(parallel ? "parallel" : "single threaded");
+
+ int num_batches = slow && !parallel ? 30 : 300;
Review comment:
Done (also fixed it in other tests)
--
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 #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#issuecomment-963191272
--
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 #11640: ARROW-14630: [C++] Fix aggregation over scalar key columns
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11640:
URL: https://github.com/apache/arrow/pull/11640#issuecomment-963568846
Benchmark runs are scheduled for baseline = 187211dedc83ac378c124b0cc7ed916872cc8174 and contender = fa6b0aab40eb910514db78cd1d641af1fa3bab35. fa6b0aab40eb910514db78cd1d641af1fa3bab35 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/27b57c800c0a4f92adbf69290d0c1f56...8553b34e1a954dce8ace157e8036d7dc/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/eebdbca358e8406cb44f801b8afbc94a...27ba597f3d754825b76429abbd7b32f7/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8f1191d42d8c4b578d1e14463c095c18...a350cf3949e24f648d22792df837bb8b/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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