Add `DictionaryArray` support in `eq_dyn` kernel #1263

viirya · 2022-02-02T18:14:32Z

Which issue does this PR close?

This is part of #1201.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

viirya · 2022-02-02T18:15:33Z

arrow/src/compute/kernels/comparison.rs

+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(left: &DictionaryArray<K>, right: &DictionaryArray<K>) -> Result<BooleanArray>


I don't find any eq implementation for DictionaryArray. Not sure if this is correct approach. cc @alamb

viirya

Is this in the right direction? As there is no existing eq implementation for DictionaryArray, I open this as wip work to get some feedback if any. Thanks.

codecov-commenter · 2022-02-02T18:34:35Z

Codecov Report

Merging #1263 (d5ffe59) into master (35e16be) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #1263    +/-   ##
========================================
  Coverage   83.04%   83.05%            
========================================
  Files         180      180            
  Lines       52424    52844   +420     
========================================
+ Hits        43537    43891   +354     
- Misses       8887     8953    +66

Impacted Files	Coverage Δ
arrow/src/compute/kernels/comparison.rs	`92.29% <100.00%> (+0.58%)`	⬆️
arrow/src/compute/kernels/filter.rs	`84.77% <0.00%> (-7.73%)`	⬇️
arrow/src/util/bench_util.rs	`95.23% <0.00%> (ø)`
parquet_derive/src/parquet_field.rs	`66.21% <0.00%> (ø)`
arrow/src/array/transform/mod.rs	`84.64% <0.00%> (+0.12%)`	⬆️
arrow/src/array/data.rs	`83.30% <0.00%> (+0.17%)`	⬆️
arrow/src/util/bit_chunk_iterator.rs	`93.83% <0.00%> (+0.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35e16be...d5ffe59. Read the comment docs.

tustvold

This looks good to me, @alamb might have some clever ideas, but I suspect without substantial changes to the rest of the file (well beyond the scope of this PR), I'm not sure how to avoid this change from being very verbose...

tustvold · 2022-02-02T18:51:05Z

arrow/src/compute/kernels/comparison.rs

+/// Perform `left == right` operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err
+/// with a casting error.
+pub fn eq_dict<K, T>(


Definitely a bonus feature, but you might want to make this generic in the comparison operation, perhaps look at simd_compare_op for inspiration

Thanks. Make it generic and rename to cmp_dict.

tustvold · 2022-02-02T18:51:36Z

arrow/src/compute/kernels/comparison.rs

+    K: ArrowNumericType,
+    T: ArrowNumericType,
+{
+    assert_eq!(left.keys().len(), right.keys().len());


Looking at the other implementations this typically returns an error

if $left.len() != $right.len() { return Err(ArrowError::ComputeError( "Cannot perform comparison operation on arrays of different length" .to_string(), )); }

Changed to an error.

tustvold · 2022-02-02T18:59:16Z

arrow/src/compute/kernels/comparison.rs

@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
    }};
 }

+macro_rules! typed_dict_cmp {


It's entirely personal preference, and this file does tend to use macros for this stuff, but personally I find generics easier to interpret, debug and maintain. Just putting the idea out there

macros are used pretty heavily in this file, so it may be better to follow along with that pattern

tustvold · 2022-02-02T19:01:26Z

arrow/src/compute/kernels/comparison.rs

+        }
+    }
+
+    Ok(BooleanArray::from(result))


It is likely to be significantly faster to use MutableBuffer::from_trusted_len_iter_bool(iter) here, where iter is the body of the for loop. This not only avoids incrementally growing a Vec, but also allocating space for Vec<bool> and then copying it into a Buffer - instead it will construct the packed bitmask directly

tustvold · 2022-02-02T19:10:42Z

arrow/src/compute/kernels/comparison.rs

+                    (DataType::Int8, DataType::Int8) => {
+                        let left = as_dictionary_array::<Int8Type>($LEFT);
+                        let right = as_dictionary_array::<Int8Type>($RIGHT);
+                        typed_dict_cmp!(left, right, $OP_PRIM, Int8Type)


I do wonder if there might be some way to reuse typed_compares here somehow, with the notion of the source type somehow encoded in OP_PRIM, etc... but it may be more effort than it is worth

alamb

Thanks @viirya -- this is looking quite cool.

alamb · 2022-02-02T22:00:15Z

arrow/src/compute/kernels/comparison.rs

+    for idx in 0..left.keys().len() {
+        unsafe {


I think you could avoid some amount of this unsafe-ness and Vec manipulation with something like this (untested)

let result: BooleanArray = left.keys().iter() .zip(right.keys.iter()) .map(|(left_key, right_key)| { let left_key = left_key.to_usize().expect(".."); let right_key = right_key.to_usize().expect(".."); ... left_value == right_value }) .collect()

Though given this pattern, perhaps we could (in a separate PR) implement an iterator for DictionaryArray, so this code could devolve into

let result: BooleanArray = left.iter() .zip(right.iter()) .map(|(left_value, right_value)| { left_value == right_value }) .collect()

🤔 then I wonder how many of the existing macros would "just work" 🤔

Changed to suggested approach. I can work on an iterator for DictionaryArray in a separate PR later.

alamb · 2022-02-02T22:01:09Z

arrow/src/compute/kernels/comparison.rs

@@ -2030,12 +2030,112 @@ macro_rules! typed_compares {
    }};
 }

+macro_rules! typed_dict_cmp {


macros are used pretty heavily in this file, so it may be better to follow along with that pattern

viirya · 2022-02-04T07:02:05Z

Thanks for the review comments! I will address them and finish soon.

…emove unsafeness.

viirya · 2022-02-09T01:39:42Z

arrow/src/compute/kernels/comparison.rs

+                        let left = as_dictionary_array::<UInt64Type>($LEFT);
+                        let right = as_dictionary_array::<UInt64Type>($RIGHT);
+                        typed_dict_cmp!(left, right, $OP_PRIM, UInt64Type)
+                    }


Seems we don't need implement for other types? ArrowDictionaryKeyType is only implemented for Int8Type...UInt64Type.

Yes, I believe that is correct -- the only supported key types are Int8Type...UInt64Type (at least in arrow-rs, I am not sure about the arrow spec in general).

…array cases

viirya · 2022-02-09T17:49:28Z

@alamb @tustvold This is ready for review. Please take a look again. Thanks.

alamb · 2022-02-10T14:45:55Z

arrow/src/compute/kernels/comparison.rs

+            (DataType::UInt64, DataType::UInt64) => {
+                cmp_dict::<$KT, UInt64Type, _>($LEFT, $RIGHT, $OP)
+            }
+            (DataType::Utf8, DataType::Utf8) => {


Maybe it is also useful to include Float32 and Float64 -- but we an also do that as a follow on PR if someone is looking for it

oops, I forgot Float32 and Float64. Going to add them.

alamb

Thanks @viirya this is looking very nice.

cc @tustvold @matthewmturner

alamb · 2022-02-10T14:50:47Z

arrow/src/compute/kernels/comparison.rs

+                        let left = as_dictionary_array::<UInt64Type>($LEFT);
+                        let right = as_dictionary_array::<UInt64Type>($RIGHT);
+                        typed_dict_cmp!(left, right, $OP_PRIM, UInt64Type)
+                    }


Yes, I believe that is correct -- the only supported key types are Int8Type...UInt64Type (at least in arrow-rs, I am not sure about the arrow spec in general).

alamb · 2022-02-10T14:51:07Z

arrow/src/compute/kernels/comparison.rs

+}
+
+/// Helper function to perform boolean lambda function on values from two dictionary arrays, this
+/// version does not attempt to use SIMD.


Suggested change

/// version does not attempt to use SIMD.

/// version does not attempt to use SIMD explicitly (though the compiler may auto vectorize)

alamb · 2022-02-10T14:52:07Z

arrow/src/compute/kernels/comparison.rs

@@ -2030,6 +2030,271 @@ macro_rules! typed_compares {
    }};
 }

+macro_rules! typed_dict_cmp {


Suggested change

macro_rules! typed_dict_cmp {

/// Applies $OP to $LEFT and $RIGHT which are two dictionaries which have (the same) key type $KT

macro_rules! typed_dict_cmp {

alamb · 2022-02-10T14:53:26Z

arrow/src/compute/kernels/comparison.rs

+                    let left_key = left_k.to_usize().expect("Dictionary index not usize");
+                    let right_key =
+                        right_k.to_usize().expect("Dictionary index not usize");
+                    unsafe {


alamb · 2022-02-10T14:54:44Z

arrow/src/compute/kernels/comparison.rs

+}
+
+/// Perform given operation on two `DictionaryArray`s.
+/// Only when two arrays are of the same type the comparison will happen otherwise it will err


Suggested change

/// Only when two arrays are of the same type the comparison will happen otherwise it will err

/// Returns an error if the two arrays have different value type

changed as suggested

alamb · 2022-02-10T15:02:10Z

arrow/src/compute/kernels/comparison.rs

@@ -2045,7 +2310,12 @@ macro_rules! typed_compares {
 /// assert_eq!(BooleanArray::from(vec![Some(true), None, Some(false)]), result);
 /// ```
 pub fn eq_dyn(left: &dyn Array, right: &dyn Array) -> Result<BooleanArray> {
-    typed_compares!(left, right, eq_bool, eq, eq_utf8, eq_binary)
+    match left.data_type() {


this is wonderful

alamb · 2022-02-10T15:16:39Z

arrow/src/compute/kernels/comparison.rs

+
+    #[test]
+    fn test_eq_dyn_dictionary_i8_array() {
+        let key_type = DataType::Int8;


Thank you for these tests. It clearly is quite messy to create DictionaryArrays :(

I have some ideas of how to make it less of a mess and will file a follow on ticket / PRs.

#1300 as a proposed PR to make it better

yea, I found it is a bit painful to create dictionary array. thanks for the proposal!

viirya · 2022-02-10T17:23:50Z

@alamb Thanks! Added Float32 and Float64 and also changed the comments as suggested.

jhorstmann · 2022-02-13T09:59:16Z

Looks good. I also find generics to be nicer, mostly because of IDE support, but those would be tricky here since there is no generic trait containing the value/value_unchecked methods.

I was wondering, what does the arrow spec say about dictionary values being null? Probably no one uses that combination, but in theory, a valid key could map to a null value.

alamb · 2022-02-13T13:30:48Z

I was wondering, what does the arrow spec say about dictionary values being null? Probably no one uses that combination, but in theory, a valid key could map to a null value.

https://arrow.apache.org/docs/format/Columnar.html#dictionary-encoded-layout

says explicitly

Note that a dictionary is permitted to contain duplicate values or nulls:

tustvold · 2022-02-13T13:53:39Z

From the link @alamb provided

The null count of such arrays is dictated only by the validity bitmap of its indices, irrespective of any null values in the dictionary.

i.e. any nulls on the values array are simply ignored, and only the null mask of the indices array (keys in arrow-rs) are considered.

alamb · 2022-02-13T14:00:19Z

i.e. any nulls on the values array are simply ignored, and only the null mask of the indices array (keys in arrow-rs) are considered.

I agree that this is my interpretation as well. Maybe it is worth a PR to clarify the format docs to be super explicit on this point?

viirya marked this pull request as draft February 2, 2022 18:14

github-actions bot added the arrow Changes to the arrow crate label Feb 2, 2022

viirya commented Feb 2, 2022

View reviewed changes

viirya force-pushed the eq_dict_dyn branch from 6ee1506 to 6d71521 Compare February 2, 2022 18:19

Implement DictionaryArray support in eq_dyn

727aba1

viirya force-pushed the eq_dict_dyn branch from 6d71521 to 727aba1 Compare February 2, 2022 18:22

tustvold reviewed Feb 2, 2022

View reviewed changes

alamb reviewed Feb 2, 2022

View reviewed changes

viirya added 4 commits February 8, 2022 15:35

For review comment: make eq_dict as generic and rename to cmp_dict. R…

eff3e10

…emove unsafeness.

Other integer types

e8399e0

Fix clippy error

11326f6

Fix format

9029d46

viirya commented Feb 9, 2022

View reviewed changes

viirya marked this pull request as ready for review February 9, 2022 01:40

viirya changed the title ~~wip. Implement DictionaryArray support in eq_dyn~~ Implement DictionaryArray support in eq_dyn Feb 9, 2022

viirya added 2 commits February 8, 2022 17:57

Merge remote-tracking branch 'upstream/master' into eq_dict_dyn

33eda30

Fix clippy and format

debbaba

viirya force-pushed the eq_dict_dyn branch from 5a68d89 to debbaba Compare February 9, 2022 02:04

Add cmp_dict_utf8 and cmp_dict_binary to cover the utf8/binary value …

fefd9ba

…array cases

viirya force-pushed the eq_dict_dyn branch from 3b9b40c to fefd9ba Compare February 9, 2022 04:13

viirya added 2 commits February 8, 2022 20:48

Add binary test

d769fd9

Add remaining types

65f286b

viirya mentioned this pull request Feb 10, 2022

Make it easier to iterate over DictionaryArray #1295

Closed

alamb reviewed Feb 10, 2022

View reviewed changes

alamb approved these changes Feb 10, 2022

View reviewed changes

alamb mentioned this pull request Feb 10, 2022

Improve ergonomics to construct DictionaryArrays from Key and Value arrays #1299

Closed

alamb mentioned this pull request Feb 10, 2022

Add DictionaryArray::try_new() to create dictionaries from pre existing arrays #1300

Merged

Add Float32 and Float64 and update a few comments.

d5ffe59

alamb merged commit 6ee1fda into apache:master Feb 13, 2022

alamb changed the title ~~Implement DictionaryArray support in eq_dyn~~ Add DictionaryArray support in eq_dyn kernel Feb 16, 2022

alamb added the enhancement Any new improvement worthy of a entry in the changelog label Feb 16, 2022

	/// version does not attempt to use SIMD.
	/// version does not attempt to use SIMD explicitly (though the compiler may auto vectorize)

	macro_rules! typed_dict_cmp {
	/// Applies $OP to $LEFT and $RIGHT which are two dictionaries which have (the same) key type $KT
	macro_rules! typed_dict_cmp {

	/// Only when two arrays are of the same type the comparison will happen otherwise it will err
	/// Returns an error if the two arrays have different value type

Add DictionaryArray support in eq_dyn kernel #1263

Add DictionaryArray support in eq_dyn kernel #1263

Conversation

viirya commented Feb 2, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 2, 2022 • edited Loading

Codecov Report

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Feb 10, 2022

jhorstmann commented Feb 13, 2022

alamb commented Feb 13, 2022

tustvold commented Feb 13, 2022 • edited Loading

alamb commented Feb 13, 2022

Add `DictionaryArray` support in `eq_dyn` kernel #1263

Add `DictionaryArray` support in `eq_dyn` kernel #1263

codecov-commenter commented Feb 2, 2022 •

edited

Loading

tustvold left a comment •

edited

Loading

tustvold commented Feb 13, 2022 •

edited

Loading