You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2025/03/05 11:50:25 UTC
[PR] GH-45676: [C++][Compute] Add skew and kurtosis functions [arrow]
pitrou opened a new pull request, #45677:
URL: https://github.com/apache/arrow/pull/45677
### Rationale for this change
### What changes are included in this PR?
### Are these changes tested?
### Are there any user-facing changes?
--
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
Re: [PR] GH-45676: [C++][Python][Compute] Add skew and kurtosis functions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45677:
URL: https://github.com/apache/arrow/pull/45677#discussion_r1981549131
##########
cpp/src/arrow/compute/kernels/hash_aggregate.cc:
##########
@@ -928,27 +969,34 @@ struct GroupedVarStdImpl : public GroupedAggregator {
means[i] = ToDouble(sums[i]) / counts[i];
}
+ double* m2s = state.m2s_mutable_data();
+ double* m3s = state.m3s_mutable_data();
+ double* m4s = state.m4s_mutable_data();
+ // Having distinct VisitGroupedValuesNonNull calls based on moments_level_
+ // would increase code generation for relatively little benefit.
VisitGroupedValuesNonNull<Type>(
batch, [&](uint32_t g, typename TypeTraits<Type>::CType value) {
- const double v = ToDouble(value);
- m2s[g] += (v - means[g]) * (v - means[g]);
+ const double d = ToDouble(value) - means[g];
+ const double d2 = d * d;
+ switch (moments_level_) {
+ case 4:
+ m4s[g] += d2 * d2;
+ [[fallthrough]];
+ case 3:
+ m3s[g] += d2 * d;
+ [[fallthrough]];
+ default:
+ m2s[g] += d2;
+ break;
+ }
});
- ARROW_ASSIGN_OR_RAISE(auto mapping,
- AllocateBuffer(num_groups_ * sizeof(uint32_t), pool_));
- for (uint32_t i = 0; static_cast<int64_t>(i) < num_groups_; i++) {
- mapping->template mutable_data_as<uint32_t>()[i] = i;
- }
- ArrayData group_id_mapping(uint32(), num_groups_, {nullptr, std::move(mapping)},
- /*null_count=*/0);
- return this->Merge(std::move(state), group_id_mapping);
Review Comment:
This was doing a pointless allocation of the identity mapping at every batch consumption stage.
--
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
Re: [PR] GH-45676: [C++][Python][Compute] Add skew and kurtosis functions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #45677:
URL: https://github.com/apache/arrow/pull/45677
--
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
Re: [PR] GH-45676: [C++][Compute] Add skew and kurtosis functions [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45677:
URL: https://github.com/apache/arrow/pull/45677#issuecomment-2700816031
Revision: 8234bdaf549cc122f584c8c2aa6a9514bdff6ac4
Submitted crossbow builds: [ursacomputing/crossbow @ actions-7c1b8ba665](https://github.com/ursacomputing/crossbow/branches/all?query=actions-7c1b8ba665)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157102/job/38237035178)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157957/job/38237039407)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/13676158060/job/38237038988)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157509/job/38237036624)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/13676158075/job/38237039246)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157514/job/38237036934)|
|test-conda-cpp-meson|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157560/job/38237036691)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157413/job/38237037438)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157505/job/38237037444)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157908/job/38237038924)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157558/job/38237037447)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157093/job/38237035224)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157463/job/38237036572)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157286/job/38237035999)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157546/job/38237037481)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157115/job/38237035190)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157960/job/38237039514)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157579/job/38237036936)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157933/job/38237039247)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157261/job/38237036227)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157726/job/38237038633)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157942/job/38237038941)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/13676157614/job/38237037808)|
--
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
Re: [PR] GH-45676: [C++][Python][Compute] Add skew and kurtosis functions [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #45677:
URL: https://github.com/apache/arrow/pull/45677#issuecomment-2707998852
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 13c898cbe9ef08daf1bc7bdf08280c5bfa16086d.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/38415351620) has more details. It also includes information about 37 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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
Re: [PR] GH-45676: [C++][Compute] Add skew and kurtosis functions [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45677:
URL: https://github.com/apache/arrow/pull/45677#issuecomment-2700889511
Revision: 21ae489a45b8eb40dbfd611150dc9412babf78bb
Submitted crossbow builds: [ursacomputing/crossbow @ actions-893ce5dfcd](https://github.com/ursacomputing/crossbow/branches/all?query=actions-893ce5dfcd)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745997/job/38238834794)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745618/job/38238831667)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745716/job/38238832223)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745858/job/38238833501)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/13676746165/job/38238835653)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745746/job/38238832804)|
|test-conda-cpp-meson|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745702/job/38238832281)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745719/job/38238832813)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745538/job/38238831464)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/13676746058/job/38238834816)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745856/job/38238833793)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745552/job/38238831650)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745846/job/38238833330)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/13676746006/job/38238835002)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745768/job/38238832856)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745775/job/38238833114)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745876/job/38238834417)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745774/job/38238833191)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745805/job/38238833235)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745665/job/38238832141)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/13676746034/job/38238835012)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745299/job/38238831103)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/13676745868/job/38238834423)|
--
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
Re: [PR] GH-45676: [C++][Python][Compute] Add skew and kurtosis functions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45677:
URL: https://github.com/apache/arrow/pull/45677#issuecomment-2705994463
@zanmato1984 Do you want to review this? Otherwise I may merge soon, given that there is already an approval.
--
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
Re: [PR] GH-45676: [C++][Compute] Add skew and kurtosis functions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45677:
URL: https://github.com/apache/arrow/pull/45677#issuecomment-2700810167
@github-actions crossbow submit -g cpp
--
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
Re: [PR] GH-45676: [C++][Compute] Add skew and kurtosis functions [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45677:
URL: https://github.com/apache/arrow/pull/45677#issuecomment-2700883168
@github-actions crossbow submit -g cpp
--
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
Re: [PR] GH-45676: [C++][Python][Compute] Add skew and kurtosis functions [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45677:
URL: https://github.com/apache/arrow/pull/45677#issuecomment-2706035788
Hi @pitrou , sorry I was planning looking at it these days but never got the time. Given the 1 lgtm, I think I'll just execute myself and let it merge as you wish.
--
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