You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "js8544 (via GitHub)" <gi...@apache.org> on 2023/06/10 08:02:04 UTC
[GitHub] [arrow] js8544 opened a new pull request, #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
js8544 opened a new pull request, #36020:
URL: https://github.com/apache/arrow/pull/36020
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Implement cumulative prod, max and min compute functions
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
1. Add implementations, docs and tests for the three functions.
2. Refactor `CumulativeSumOptions` to `CumulativeOptions` for reusability.
3. Fix a bug where `GenericFromScalar(GenericToScalar(std::nullopt)) != std::nullopt`.
4. Remove an unnecessary Cast with the default start value.
I'll explain some of the changes in comments.
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
5. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes, in vector_accumulative_ops_test.cc and test_compute.py
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
No. The data members of `CumulativeSumOptions` are changed, but the member functions behave as before. And std::optional<T> also can be constructed directly from T. So users should not feel any difference.
--
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] bkietz commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1235619772
##########
cpp/src/arrow/compute/function_internal.h:
##########
@@ -382,6 +377,12 @@ static inline Result<std::shared_ptr<Scalar>> GenericToScalar(const Datum& value
}
}
+template <typename T>
+static inline Result<decltype(MakeScalar(std::declval<T>()))> GenericToScalar(
+ const std::optional<T>& value) {
+ return value.has_value() ? MakeScalar(value.value()) : std::make_shared<NullScalar>();
+}
Review Comment:
This might be more readable with a trailing return type. Also, do you think we should include an overload which accepts std::nullopt?
```suggestion
static inline Result<std::shared_ptr<Scalar>> GenericToScalar(std::nullopt_t) {
return std::make_shared<NullScalar>();
}
template <typename T>
static inline auto GenericToScalar(
const std::optional<T>& value) -> Result<decltype(MakeScalar(value.value()))> {
return value.has_value() ? MakeScalar(value.value()) : std::make_shared<NullScalar>();
}
```
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc:
##########
@@ -160,18 +169,52 @@ const FunctionDoc cumulative_sum_doc{
("`values` must be numeric. Return an array/chunked array which is the\n"
"cumulative sum computed over `values`. Results will wrap around on\n"
"integer overflow. Use function \"cumulative_sum_checked\" if you want\n"
- "overflow to return an error."),
+ "overflow to return an error. The default start is 0."),
{"values"},
- "CumulativeSumOptions"};
+ "CumulativeOptions"};
const FunctionDoc cumulative_sum_checked_doc{
"Compute the cumulative sum over a numeric input",
("`values` must be numeric. Return an array/chunked array which is the\n"
"cumulative sum computed over `values`. This function returns an error\n"
"on overflow. For a variant that doesn't fail on overflow, use\n"
- "function \"cumulative_sum\"."),
+ "function \"cumulative_sum\". The default start is 0."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_prod_doc{
+ "Compute the cumulative product over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative product computed over `values`. Results will wrap around on\n"
+ "integer overflow. Use function \"cumulative_prod_checked\" if you want\n"
+ "overflow to return an error. The default start is 1."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_prod_checked_doc{
+ "Compute the cumulative product over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative product computed over `values`. This function returns an error\n"
+ "on overflow. For a variant that doesn't fail on overflow, use\n"
+ "function \"cumulative_prod\". The default start is 1."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_max_doc{
+ "Compute the cumulative max over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative max computed over `values`. The default start is the minimum\n"
+ "value of input type."),
Review Comment:
I think it'd be worthwhile to be even more explicit here
```suggestion
"value of input type (so that any other value will replace the\n
"start as the new maximum)."),
```
(and similarly for cumulative min)
##########
cpp/src/arrow/compute/function_internal.h:
##########
@@ -510,6 +491,29 @@ static inline enable_if_same_result<T, Datum> GenericFromScalar(
return Status::Invalid("Cannot deserialize Datum from ", value->ToString());
}
+template <typename>
+constexpr bool is_optional_impl = false;
+template <typename T>
+constexpr bool is_optional_impl<std::optional<T>> = true;
+
+template <typename T>
+using is_optional =
+ std::integral_constant<bool, is_optional_impl<std::decay_t<T>> ||
+ std::is_same<T, std::nullopt_t>::value>;
+
+template <typename T, typename R = void>
+using enable_if_optional = enable_if_t<is_optional<T>::value, Result<T>>;
+
+template <typename T>
+static inline enable_if_optional<T> GenericFromScalar(
Review Comment:
I think this doesn't need the `integral_constant` or the custom `enable_if_optional`, would you mind simplifying it?
```suggestion
template <typename>
constexpr bool is_optional_v = false;
template <typename T>
constexpr bool is_optional_v<std::optional<T>> = true;
template <>
constexpr bool is_optional_v<std::nullopt_t> = true;
template <typename T>
static inline std::enable_if_t<is_optional_v<T>, Result<T>> GenericFromScalar(
```
##########
python/pyarrow/_compute.pyx:
##########
@@ -1928,31 +1928,37 @@ class PartitionNthOptions(_PartitionNthOptions):
self._set_options(pivot, null_placement)
-cdef class _CumulativeSumOptions(FunctionOptions):
+cdef class _CumulativeOptions(FunctionOptions):
def _set_options(self, start, skip_nulls):
- if not isinstance(start, Scalar):
+ if start is None:
+ self.wrapped.reset(new CCumulativeOptions(skip_nulls))
+ elif isinstance(start, Scalar):
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
+ else:
try:
start = lib.scalar(start)
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
except Exception:
_raise_invalid_function_option(
start, "`start` type for CumulativeSumOptions", TypeError)
- self.wrapped.reset(new CCumulativeSumOptions((<Scalar> start).unwrap(), skip_nulls))
-
-class CumulativeSumOptions(_CumulativeSumOptions):
+class CumulativeOptions(_CumulativeOptions):
"""
- Options for `cumulative_sum` function.
+ Options for `cumulative` functions.
Review Comment:
I think it'd be worthwhile to list these explicitly
```suggestion
Options for `cumulative_*` functions.
- cumulative_sum
- cumulative_sum_checked
...
```
##########
python/pyarrow/compute.py:
##########
@@ -33,7 +33,7 @@
AssumeTimezoneOptions,
CastOptions,
CountOptions,
- CumulativeSumOptions,
+ CumulativeOptions,
Review Comment:
@jorisvandenbossche Does this need an alias?
```suggestion
CumulativeOptions,
CumulativeOptions as CumulativeSumOptions,
```
##########
cpp/src/arrow/compute/function_internal.h:
##########
@@ -510,6 +491,29 @@ static inline enable_if_same_result<T, Datum> GenericFromScalar(
return Status::Invalid("Cannot deserialize Datum from ", value->ToString());
}
+template <typename>
+constexpr bool is_optional_impl = false;
+template <typename T>
+constexpr bool is_optional_impl<std::optional<T>> = true;
+
+template <typename T>
+using is_optional =
+ std::integral_constant<bool, is_optional_impl<std::decay_t<T>> ||
+ std::is_same<T, std::nullopt_t>::value>;
+
+template <typename T, typename R = void>
+using enable_if_optional = enable_if_t<is_optional<T>::value, Result<T>>;
+
+template <typename T>
+static inline enable_if_optional<T> GenericFromScalar(
Review Comment:
That seems like a better default indeed
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -60,6 +60,11 @@ struct Add {
static enable_if_decimal_value<T> Call(KernelContext*, Arg0 left, Arg1 right, Status*) {
return left + right;
}
+
+ template <typename T>
+ static constexpr T Identity() {
Review Comment:
This is great. Could you make this into a doccomment? Also, it might be more obviously part of the interface for these kernel ops if you made it a trait:
```c++
/// The term identity is from the mathematical notation monoid.
/// For any associative binary operation, identity is defined as
///
/// Op(identity, x) = x for all x.
template <typename Op>
struct Identity;
template <>
struct Identity<Add> {
template <typename Value>
static constexpr Value value{0};
};
template <>
struct Identity<AddChecked> : Identity<Add> {};
// ...
```
Then the value of the identity can be referenced with `Identity<Add>::value<int32_t>` or so
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc:
##########
@@ -49,15 +50,12 @@ struct CumulativeOptionsWrapper : public OptionsWrapper<OptionsType> {
}
const auto& start = options->start;
- if (!start || !start->is_valid) {
- return Status::Invalid("Cumulative `start` option must be non-null and valid");
- }
- // Ensure `start` option matches input type
- if (!start->type->Equals(*args.inputs[0])) {
- ARROW_ASSIGN_OR_RAISE(
- auto casted_start,
- Cast(Datum(start), args.inputs[0], CastOptions::Safe(), ctx->exec_context()));
+ // Ensure `start` option, if given, matches input type,
Review Comment:
```suggestion
// Ensure `start` option, if given, matches input type
```
--
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] bkietz merged pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz merged PR #36020:
URL: https://github.com/apache/arrow/pull/36020
--
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] conbench-apache-arrow[bot] commented on pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36020:
URL: https://github.com/apache/arrow/pull/36020#issuecomment-1606268370
Conbench analyzed the 6 benchmark runs on commit `e3eb5898`.
There were 6 benchmark results indicating a performance regression:
- Commit Run on `ursa-thinkcentre-m75q` at [2023-06-25 03:03:51Z](http://conbench.ursa.dev/compare/runs/e68d3f0b75384131a17b5b38a6274c1d...a7e4561927c347279aec1272a1d9f746/)
- [params=1048576/1, source=cpp-micro, suite=arrow-acero-aggregate-benchmark](http://conbench.ursa.dev/compare/benchmarks/0649785c58d57f1a800092f56146e73d...06497aecf3b87be58000c4199f6f6806)
- [params=<BooleanType>/1048576/0, source=cpp-micro, suite=arrow-acero-aggregate-benchmark](http://conbench.ursa.dev/compare/benchmarks/0649785b8df570bf8000f53e3c2d6ae9...06497aec21697bf28000c469386d72a2)
- and 4 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/14538169361) has more details.
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1225211390
##########
cpp/src/arrow/compute/function_internal.h:
##########
@@ -510,6 +491,29 @@ static inline enable_if_same_result<T, Datum> GenericFromScalar(
return Status::Invalid("Cannot deserialize Datum from ", value->ToString());
}
+template <typename>
+constexpr bool is_optional_impl = false;
+template <typename T>
+constexpr bool is_optional_impl<std::optional<T>> = true;
+
+template <typename T>
+using is_optional =
+ std::integral_constant<bool, is_optional_impl<std::decay_t<T>> ||
+ std::is_same<T, std::nullopt_t>::value>;
+
+template <typename T, typename R = void>
+using enable_if_optional = enable_if_t<is_optional<T>::value, Result<T>>;
+
+template <typename T>
+static inline enable_if_optional<T> GenericFromScalar(
Review Comment:
The current implementation for `GenericFromScalar` and `GenericToScalar` is buggy for `std::optional`.` GenericToScalar(std::nullopt)` creates a StringScalar with an empty string. Thus `GenericFromScalar(GenericToScalar(std::nullopt))` != std::nullopt`.
I used Scalar(NullType) to represent std::nullopt which should be fine because AFAIU nothing else would be serialized to NullType.
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1225212144
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -60,6 +60,11 @@ struct Add {
static enable_if_decimal_value<T> Call(KernelContext*, Arg0 left, Arg1 right, Status*) {
return left + right;
}
+
+ template <typename T>
+ static constexpr T Identity() {
Review Comment:
The term identity is from the mathematical notation monoid. For any associative binary operation, identity is definded as Op(identity, x) = x for all x.
--
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] icexelloss commented on pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on PR #36020:
URL: https://github.com/apache/arrow/pull/36020#issuecomment-1602974604
Thanks @bkietz - created https://github.com/apache/arrow/issues/36248 to track the Asof Join backpresure test issue
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237842742
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -605,6 +606,96 @@ struct Sign {
}
};
+struct Max {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0 arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ return std::max(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::max(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMinSentinel();
+ }
+};
+
+struct Min {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0 arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ return std::min(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::min(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMaxSentinel();
+ }
+};
+
+/// The term identity is from the mathematical notation monoid.
+/// For any associative binary operation, identity is defined as:
+/// Op(identity, x) = x for all x.
+template <typename Op>
+struct Identity;
+
+template <>
+struct Identity<Add> {
+ template <typename Value>
+ static constexpr Value value{0};
+};
+
+template <>
+struct Identity<AddChecked> : Identity<Add> {};
+
+template <>
+struct Identity<Multiply> {
+ template <typename Value>
+ static constexpr Value value{1};
+};
+
+template <>
+struct Identity<MultiplyChecked> : Identity<Multiply> {};
+
+template <>
+struct Identity<Max> {
+ template <typename Value>
+ static constexpr Value value{std::numeric_limits<Value>::min()};
Review Comment:
I'll remove them for this pr. Will send another pr for decimals.
--
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] bkietz commented on pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on PR #36020:
URL: https://github.com/apache/arrow/pull/36020#issuecomment-1602538003
CI failure is a [backpressure test flake-out in AsOfJoin](https://github.com/apache/arrow/actions/runs/5340154873/jobs/9679635756?pr=36020#step:6:2882), unrelated.
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237842999
##########
python/pyarrow/_compute.pyx:
##########
@@ -1928,31 +1928,41 @@ class PartitionNthOptions(_PartitionNthOptions):
self._set_options(pivot, null_placement)
-cdef class _CumulativeSumOptions(FunctionOptions):
+cdef class _CumulativeOptions(FunctionOptions):
def _set_options(self, start, skip_nulls):
- if not isinstance(start, Scalar):
+ if start is None:
+ self.wrapped.reset(new CCumulativeOptions(skip_nulls))
+ elif isinstance(start, Scalar):
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
+ else:
try:
start = lib.scalar(start)
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
except Exception:
_raise_invalid_function_option(
start, "`start` type for CumulativeSumOptions", TypeError)
- self.wrapped.reset(new CCumulativeSumOptions((<Scalar> start).unwrap(), skip_nulls))
-
-class CumulativeSumOptions(_CumulativeSumOptions):
+class CumulativeOptions(_CumulativeOptions):
"""
- Options for `cumulative_sum` function.
+ Options for `cumulative_*` functions.
+
+ - cumulative_sum
+ - cumulative_sum_checked
+ ...
Review Comment:
My bad. All of them are listed now.
--
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] jorisvandenbossche commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1238433064
##########
python/pyarrow/compute.py:
##########
@@ -33,7 +33,7 @@
AssumeTimezoneOptions,
CastOptions,
CountOptions,
- CumulativeSumOptions,
+ CumulativeOptions,
Review Comment:
For backwards compatibility, that's probably useful, yes (also on the C++ side this was done).
(although we should maybe actually add it as its own class and deprecate it, so we can remove the alias at some point)
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237274608
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc:
##########
@@ -160,18 +169,52 @@ const FunctionDoc cumulative_sum_doc{
("`values` must be numeric. Return an array/chunked array which is the\n"
"cumulative sum computed over `values`. Results will wrap around on\n"
"integer overflow. Use function \"cumulative_sum_checked\" if you want\n"
- "overflow to return an error."),
+ "overflow to return an error. The default start is 0."),
{"values"},
- "CumulativeSumOptions"};
+ "CumulativeOptions"};
const FunctionDoc cumulative_sum_checked_doc{
"Compute the cumulative sum over a numeric input",
("`values` must be numeric. Return an array/chunked array which is the\n"
"cumulative sum computed over `values`. This function returns an error\n"
"on overflow. For a variant that doesn't fail on overflow, use\n"
- "function \"cumulative_sum\"."),
+ "function \"cumulative_sum\". The default start is 0."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_prod_doc{
+ "Compute the cumulative product over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative product computed over `values`. Results will wrap around on\n"
+ "integer overflow. Use function \"cumulative_prod_checked\" if you want\n"
+ "overflow to return an error. The default start is 1."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_prod_checked_doc{
+ "Compute the cumulative product over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative product computed over `values`. This function returns an error\n"
+ "on overflow. For a variant that doesn't fail on overflow, use\n"
+ "function \"cumulative_prod\". The default start is 1."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_max_doc{
+ "Compute the cumulative max over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative max computed over `values`. The default start is the minimum\n"
+ "value of input type."),
{"values"},
- "CumulativeSumOptions"};
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_min_doc{
+ "Compute the cumulative min over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative min computed over `values`. The default start is the maximum\n"
+ "value of input type."),
Review Comment:
```suggestion
"value of input type."),
"value of input type (so that any other value will replace the\n
"start as the new minimum)."),
```
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237288808
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -60,6 +60,11 @@ struct Add {
static enable_if_decimal_value<T> Call(KernelContext*, Arg0 left, Arg1 right, Status*) {
return left + right;
}
+
+ template <typename T>
+ static constexpr T Identity() {
Review Comment:
Changed Identity to a trait
--
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 #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36020:
URL: https://github.com/apache/arrow/pull/36020#issuecomment-1585546035
:warning: GitHub issue #32190 **has been automatically assigned in GitHub** to PR creator.
--
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] bkietz commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1238443706
##########
python/pyarrow/compute.py:
##########
@@ -33,7 +33,7 @@
AssumeTimezoneOptions,
CastOptions,
CountOptions,
- CumulativeSumOptions,
+ CumulativeOptions,
Review Comment:
Follow up issue here https://github.com/apache/arrow/issues/36240
--
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] bkietz commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237583858
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -605,6 +606,96 @@ struct Sign {
}
};
+struct Max {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0 arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ return std::max(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::max(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMinSentinel();
+ }
Review Comment:
I think this isn't used anymore (also in Min).
```suggestion
```
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -605,6 +606,96 @@ struct Sign {
}
};
+struct Max {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0 arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ return std::max(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::max(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMinSentinel();
+ }
+};
+
+struct Min {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0 arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ return std::min(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::min(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMaxSentinel();
+ }
+};
+
+/// The term identity is from the mathematical notation monoid.
+/// For any associative binary operation, identity is defined as:
+/// Op(identity, x) = x for all x.
+template <typename Op>
+struct Identity;
+
+template <>
+struct Identity<Add> {
+ template <typename Value>
+ static constexpr Value value{0};
+};
+
+template <>
+struct Identity<AddChecked> : Identity<Add> {};
+
+template <>
+struct Identity<Multiply> {
+ template <typename Value>
+ static constexpr Value value{1};
+};
+
+template <>
+struct Identity<MultiplyChecked> : Identity<Multiply> {};
+
+template <>
+struct Identity<Max> {
+ template <typename Value>
+ static constexpr Value value{std::numeric_limits<Value>::min()};
Review Comment:
These cumulative ops aren't currently instantiated for decimal types anyway: https://github.com/apache/arrow/blob/44edc27/cpp/src/arrow/compute/kernels/codegen_internal.h#L1070-L1100
... if you feel like filing a follow up issue for them, it should be fairly straightforward to specify these as specializations:
```suggestion
template <typename Value>
static constexpr Value value{std::numeric_limits<Value>::min()};
template <>
static constexpr Decimal128 value<Decimal128> = Decimal128::GetMinSentinel();
```
Multiplication will require more effort since we can't construct `1` without knowing the scale.
##########
python/pyarrow/_compute.pyx:
##########
@@ -1928,31 +1928,41 @@ class PartitionNthOptions(_PartitionNthOptions):
self._set_options(pivot, null_placement)
-cdef class _CumulativeSumOptions(FunctionOptions):
+cdef class _CumulativeOptions(FunctionOptions):
def _set_options(self, start, skip_nulls):
- if not isinstance(start, Scalar):
+ if start is None:
+ self.wrapped.reset(new CCumulativeOptions(skip_nulls))
+ elif isinstance(start, Scalar):
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
+ else:
try:
start = lib.scalar(start)
+ self.wrapped.reset(new CCumulativeOptions(
+ pyarrow_unwrap_scalar(start), skip_nulls))
except Exception:
_raise_invalid_function_option(
start, "`start` type for CumulativeSumOptions", TypeError)
- self.wrapped.reset(new CCumulativeSumOptions((<Scalar> start).unwrap(), skip_nulls))
-
-class CumulativeSumOptions(_CumulativeSumOptions):
+class CumulativeOptions(_CumulativeOptions):
"""
- Options for `cumulative_sum` function.
+ Options for `cumulative_*` functions.
+
+ - cumulative_sum
+ - cumulative_sum_checked
+ ...
Review Comment:
Sorry, I didn't make it clear enough that my suggestion was not complete. Please list the rest of the cumulative functions here
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1225201776
##########
cpp/src/arrow/compute/api_vector.h:
##########
@@ -210,21 +210,29 @@ class ARROW_EXPORT PartitionNthOptions : public FunctionOptions {
NullPlacement null_placement;
};
-/// \brief Options for cumulative sum function
-class ARROW_EXPORT CumulativeSumOptions : public FunctionOptions {
+/// \brief Options for cumulative functions
+/// \note Also aliased as CumulativeSumOptions for backward compatibility
+class ARROW_EXPORT CumulativeOptions : public FunctionOptions {
public:
- explicit CumulativeSumOptions(double start = 0, bool skip_nulls = false);
- explicit CumulativeSumOptions(std::shared_ptr<Scalar> start, bool skip_nulls = false);
- static constexpr char const kTypeName[] = "CumulativeSumOptions";
- static CumulativeSumOptions Defaults() { return CumulativeSumOptions(); }
-
- /// Optional starting value for cumulative operation computation
- std::shared_ptr<Scalar> start;
+ explicit CumulativeOptions(bool skip_nulls = false);
+ explicit CumulativeOptions(double start, bool skip_nulls = false);
+ explicit CumulativeOptions(std::shared_ptr<Scalar> start, bool skip_nulls = false);
+ static constexpr char const kTypeName[] = "CumulativeOptions";
+ static CumulativeOptions Defaults() { return CumulativeOptions(); }
+
+ /// Optional starting value for cumulative operation computation, default depends on the
+ /// operation and input type.
+ /// - sum: 0
+ /// - prod: 1
+ /// - min: maximum of the input type
+ /// - max: minimum of the input type
+ std::optional<std::shared_ptr<Scalar>> start;
Review Comment:
The current implementation defaults `start` to a DoubleScalar with value zero. This is undesirable because:
1. An unnecessary `Cast` needs to happen if input type is not double.
2. For min and max we can't determine the default start value before input type is known.
So I changed this to an optional. If it is not set, the kernel will determine the start value at initialization. It's implemented with templates so there is zero cost.
--
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] jorisvandenbossche commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1238433064
##########
python/pyarrow/compute.py:
##########
@@ -33,7 +33,7 @@
AssumeTimezoneOptions,
CastOptions,
CountOptions,
- CumulativeSumOptions,
+ CumulativeOptions,
Review Comment:
For backwards compatibility, that's probably useful, yes (also on the C++ side this was done).
(although we should maybe actually add it as its own class and deprecate it, so we can remove the alias at some point. But this can also be done as a follow-up later)
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1225212555
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -605,6 +625,70 @@ struct Sign {
}
};
+struct Max {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0 arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ return std::max(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ if (std::isnan(left)) {
Review Comment:
The handling of nan is to be consistent with compute functions min_element_wise and max_element_wise
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237274608
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops.cc:
##########
@@ -160,18 +169,52 @@ const FunctionDoc cumulative_sum_doc{
("`values` must be numeric. Return an array/chunked array which is the\n"
"cumulative sum computed over `values`. Results will wrap around on\n"
"integer overflow. Use function \"cumulative_sum_checked\" if you want\n"
- "overflow to return an error."),
+ "overflow to return an error. The default start is 0."),
{"values"},
- "CumulativeSumOptions"};
+ "CumulativeOptions"};
const FunctionDoc cumulative_sum_checked_doc{
"Compute the cumulative sum over a numeric input",
("`values` must be numeric. Return an array/chunked array which is the\n"
"cumulative sum computed over `values`. This function returns an error\n"
"on overflow. For a variant that doesn't fail on overflow, use\n"
- "function \"cumulative_sum\"."),
+ "function \"cumulative_sum\". The default start is 0."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_prod_doc{
+ "Compute the cumulative product over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative product computed over `values`. Results will wrap around on\n"
+ "integer overflow. Use function \"cumulative_prod_checked\" if you want\n"
+ "overflow to return an error. The default start is 1."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_prod_checked_doc{
+ "Compute the cumulative product over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative product computed over `values`. This function returns an error\n"
+ "on overflow. For a variant that doesn't fail on overflow, use\n"
+ "function \"cumulative_prod\". The default start is 1."),
+ {"values"},
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_max_doc{
+ "Compute the cumulative max over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative max computed over `values`. The default start is the minimum\n"
+ "value of input type."),
{"values"},
- "CumulativeSumOptions"};
+ "CumulativeOptions"};
+
+const FunctionDoc cumulative_min_doc{
+ "Compute the cumulative min over a numeric input",
+ ("`values` must be numeric. Return an array/chunked array which is the\n"
+ "cumulative min computed over `values`. The default start is the maximum\n"
+ "value of input type."),
Review Comment:
```suggestion
"value of input type (so that any other value will replace the\n
"start as the new minimum)."),
```
--
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] js8544 commented on a diff in pull request #36020: GH-32190: [C++][Compute] Implement cumulative prod, max and min functions
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36020:
URL: https://github.com/apache/arrow/pull/36020#discussion_r1237842388
##########
cpp/src/arrow/compute/kernels/base_arithmetic_internal.h:
##########
@@ -605,6 +606,96 @@ struct Sign {
}
};
+struct Max {
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_not_floating_value<T> Call(KernelContext*, Arg0 arg0,
+ Arg1 arg1, Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ return std::max(arg0, arg1);
+ }
+
+ template <typename T, typename Arg0, typename Arg1>
+ static constexpr enable_if_floating_value<T> Call(KernelContext*, Arg0 left, Arg1 right,
+ Status*) {
+ static_assert(std::is_same<T, Arg0>::value && std::is_same<Arg0, Arg1>::value);
+ if (std::isnan(left)) {
+ return right;
+ } else if (std::isnan(right)) {
+ return left;
+ } else {
+ return std::max(left, right);
+ }
+ }
+
+ template <typename T>
+ static constexpr enable_if_decimal_value<T, T> Identity() {
+ return T::GetMinSentinel();
+ }
Review Comment:
Removed
--
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