You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "bkmgit (via GitHub)" <gi...@apache.org> on 2023/07/11 06:23:10 UTC
[GitHub] [arrow] bkmgit opened a new pull request, #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
bkmgit opened a new pull request, #11882:
URL: https://github.com/apache/arrow/pull/11882
Implement Between ternary kernel in C++ and add Python bindings so that one does not need to call two comparisons such as ( left <= value ) and ( value <= right )
--
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] ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11882:
URL: https://github.com/apache/arrow/pull/11882#issuecomment-3547083467
Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.
--
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] edponce commented on pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
Posted by GitBox <gi...@apache.org>.
edponce commented on PR #11882:
URL: https://github.com/apache/arrow/pull/11882#issuecomment-1106534046
@pitrou @kou Do you have additional reviews for this PR or can it be merged as-is?
--
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] ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings [arrow]
Posted by "bkmgit (via GitHub)" <gi...@apache.org>.
bkmgit commented on PR #11882:
URL: https://github.com/apache/arrow/pull/11882#issuecomment-3561755382
Still of interest
--
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] kou commented on a diff in pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [arrow] amol- closed pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- closed pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
URL: https://github.com/apache/arrow/pull/11882
--
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] bkmgit commented on pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
Posted by "bkmgit (via GitHub)" <gi...@apache.org>.
bkmgit commented on PR #11882:
URL: https://github.com/apache/arrow/pull/11882#issuecomment-1630407438
@jorisvandenbossche Thanks for re-opening
--
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] bkmgit commented on pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
Posted by "bkmgit (via GitHub)" <gi...@apache.org>.
bkmgit commented on PR #11882:
URL: https://github.com/apache/arrow/pull/11882#issuecomment-1630195816
@amol- do not have rights to reopen, can recreate a new one with the same history
--
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] amol- commented on pull request #11882: ARROW-9843: [C++][Python] Implement Between ternary kernel and Python bindings
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #11882:
URL: https://github.com/apache/arrow/pull/11882#issuecomment-1490659040
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
--
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