alamb commented on a change in pull request #1326:
URL: https://github.com/apache/arrow-rs/pull/1326#discussion_r809061485
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2032,10 +2032,10 @@ macro_rules! typed_compares {
/// Applies $OP to $LEFT and $RIGHT which are two dictionaries which have (the
same) key type $KT
macro_rules! typed_dict_cmp {
- ($LEFT: expr, $RIGHT: expr, $OP: expr, $KT: tt) => {{
+ ($LEFT: expr, $RIGHT: expr, $OP: expr, $OP_BOOL: expr, $KT: tt) => {{
Review comment:
👍 nice readability improvement
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2360,7 +2365,12 @@ pub fn neq_dyn(left: &dyn Array, right: &dyn Array) ->
Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(true), Some(false), None]),
result);
/// ```
pub fn lt_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, lt_bool, lt, lt_utf8, lt_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a < b, |a, b| (!a) & b)
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a < b, |a, b| a < b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2318,7 +2318,7 @@ where
pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_compares!(left, right, |a, b| a == b)
+ typed_dict_compares!(left, right, |a, b| a == b, |a, b| !(a ^ b))
Review comment:
I don't understand this change -- I think the `a == b` is easier to
understand and I would expect that llvm would create optimized code for
whatever was being compared.
If this is clippy being silly about comparing booleans perhaps we can just
ignore the lint
```suggestion
typed_dict_compares!(left, right, |a, b| a == b, |a, b| a == b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2415,7 +2435,12 @@ pub fn gt_dyn(left: &dyn Array, right: &dyn Array) ->
Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(false), Some(true), None]),
result);
/// ```
pub fn gt_eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, gt_eq_bool, gt_eq, gt_eq_utf8, gt_eq_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a >= b, |a, b| !((!a) &
b))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a >= b, |a, b| a >= b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2379,7 +2389,12 @@ pub fn lt_dyn(left: &dyn Array, right: &dyn Array) ->
Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(false), Some(true), Some(true),
None]), result);
/// ```
pub fn lt_eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, lt_eq_bool, lt_eq, lt_eq_utf8, lt_eq_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a <= b, |a, b| !(a &
(!b)))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a <= b, |a, b| a <= b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2341,7 +2341,12 @@ pub fn eq_dyn(left: &dyn Array, right: &dyn Array) ->
Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(false), None, Some(true)]),
result);
/// ```
pub fn neq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, neq_bool, neq, neq_utf8, neq_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a != b, |a, b| (a ^ b))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a != b, |a, b| a != b)
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -4790,5 +4851,76 @@ mod tests {
result.unwrap(),
BooleanArray::from(vec![false, true, false])
);
+
+ let result = neq_dyn(&dict_array1, &dict_array2);
+ assert!(result.is_ok());
Review comment:
As a style thing, I think it is ok to just `.unwrap()` the result -- if
there is a problem it will panic one line later, but I think the source of the
problem would still be quite clear
```suggestion
```
##########
File path: arrow/src/compute/kernels/comparison.rs
##########
@@ -2397,7 +2412,12 @@ pub fn lt_eq_dyn(left: &dyn Array, right: &dyn Array) ->
Result<BooleanArray> {
/// assert_eq!(BooleanArray::from(vec![Some(true), Some(false), None]),
result);
/// ```
pub fn gt_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
- typed_compares!(left, right, gt_bool, gt, gt_utf8, gt_binary)
+ match left.data_type() {
+ DataType::Dictionary(_, _) => {
+ typed_dict_compares!(left, right, |a, b| a > b, |a, b| a & (!b))
Review comment:
```suggestion
typed_dict_compares!(left, right, |a, b| a > b, |a, b| a > b)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
