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/04/25 03:26:04 UTC
[GitHub] [arrow] kou commented on a diff in pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
kou commented on code in PR #11882:
URL: https://github.com/apache/arrow/pull/11882#discussion_r857213194
##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -99,6 +99,30 @@ struct EnumTraits<compute::CompareOperator>
return "<INVALID>";
}
};
+
+template <>
+struct EnumTraits<compute::BetweenOptions::Inclusive>
+ : BasicEnumTraits<compute::BetweenOptions::Inclusive,
+ compute::BetweenOptions::Inclusive::BOTH,
+ compute::BetweenOptions::Inclusive::LEFT,
+ compute::BetweenOptions::Inclusive::RIGHT,
+ compute::BetweenOptions::Inclusive::NEITHER> {
+ static std::string name() { return "Inclusive"; }
Review Comment:
```suggestion
static std::string name() { return "BetweenOptions::Inclusive"; }
```
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -68,6 +73,57 @@ struct GreaterEqual {
}
};
+template <BetweenOptions::Inclusive>
+struct BetweenImpl;
+
+template <>
+struct BetweenImpl<BetweenOptions::Inclusive::BOTH> {
+ template <typename Arg0, typename Arg1, typename Arg2>
+ static constexpr bool Call(const Arg0& middle, const Arg1& left, const Arg2& right) {
+ static_assert(std::is_same<Arg0, Arg1>::value && std::is_same<Arg1, Arg2>::value, "");
Review Comment:
Could you specify a description of this condition instead of `""`?
##########
docs/source/cpp/compute.rst:
##########
@@ -687,35 +687,44 @@ Decimal values are accepted, but are cast to Float64 first.
Comparisons
~~~~~~~~~~~
-These functions expect two inputs of numeric type (in which case they will be
+These functions expect two or three inputs of numeric type (in which case they will be
cast to the :ref:`common numeric type <common-numeric-type>` before comparison),
-or two inputs of Binary- or String-like types, or two inputs of Temporal types.
-If any input is dictionary encoded it will be expanded for the purposes of
-comparison. If any of the input elements in a pair is null, the corresponding
+or two or three inputs of Binary- or String-like types, or two or three inputs of Temporal
+types. If any input is dictionary encoded it will be expanded for the purposes of
+comparison. If any of the input elements in a pair or triple is null, the corresponding
output element is null. Decimal arguments will be promoted in the same way as
for ``add`` and ``subtract``.
-+----------------+------------+---------------------------------------------+---------------------+
-| Function names | Arity | Input types | Output type |
-+================+============+=============================================+=====================+
-| equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean |
-+----------------+------------+---------------------------------------------+---------------------+
-| greater | Binary | Numeric, Temporal, Binary- and String-like | Boolean |
-+----------------+------------+---------------------------------------------+---------------------+
-| greater_equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean |
-+----------------+------------+---------------------------------------------+---------------------+
-| less | Binary | Numeric, Temporal, Binary- and String-like | Boolean |
-+----------------+------------+---------------------------------------------+---------------------+
-| less_equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean |
-+----------------+------------+---------------------------------------------+---------------------+
-| not_equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean |
-+----------------+------------+---------------------------------------------+---------------------+
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
+| Function names | Arity | Input types | Output type | Options class | Notes |
++================+=========+=============================================+=============+==========================+=======+
+| between | Ternary | Numeric, Temporal, Binary- and String-like | Boolean | :struct:`BetweenOptions` | \(1) |
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
+| equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean | | |
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
+| greater | Binary | Numeric, Temporal, Binary- and String-like | Boolean | | |
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
+| greater_equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean | | |
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
+| less | Binary | Numeric, Temporal, Binary- and String-like | Boolean | | |
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
+| less_equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean | | |
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
+| not_equal | Binary | Numeric, Temporal, Binary- and String-like | Boolean | | |
++----------------+---------+---------------------------------------------+-------------+--------------------------+-------+
These functions take any number of inputs of numeric type (in which case they
will be cast to the :ref:`common numeric type <common-numeric-type>` before
comparison) or of temporal types. If any input is dictionary encoded it will be
expanded for the purposes of comparison.
+* \(1) Options are used to control whether either comparison endpoint is inclusive.
+ The default is ``BetweenOptions::Inclusive::BOTH``, other possible value are
+ ``BetweenOptions::Inclusive::LEFT``, ``BetweenOptions::Inclusive::RIGHT`` and
+ ``BetweenOptions::Inlcusive::NEITHER`` corresponding to ``a <= value <= b``,
+ ``a <= value < b``, ``a < value <= b`` and ``a < value < b`` (respectively).
+ Strings are presently compared by their UTF8 codepoint values.
Review Comment:
```suggestion
Strings are presently compared by their Unicode codepoint values.
```
##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -1240,5 +1253,21 @@ ARROW_EXPORT Result<Datum> AssumeTimezone(const Datum& values,
AssumeTimezoneOptions options,
ExecContext* ctx = NULLPTR);
+/// \brief Between compares each element in `values`
+/// with `left` as a lower bound and 'right' as an upperbound
+///
+/// \param[in] values input to compare between left and right
+/// \param[in] left used as the lower bound for comparison
+/// \param[in] right used as the upper bound for comparison
+/// \param[in] ctx the function execution context, optional
+///
+/// \return the resulting datum
+///
+/// \note Bounds are not inclusive
Review Comment:
It seems that this isn't resolved.
It seems that both of `left` and `right` are included by default.
--
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