Dictionary like scalar kernels #2591

psvri · 2022-08-25T17:11:07Z

Which issue does this PR close?

Partially implements #1975.

Rationale for this change

Enhancement to add like kernels for dictionary.

What changes are included in this PR?

Like kernels for dictionary and string

Are there any user-facing changes?

No

…ionary_like

psvri · 2022-08-25T17:13:33Z

A side effect of this PR resulted in some nice performance improvements as well. Some othese range form 2-3% to about 30/50% for some use cases.

On my OCI 4 core arm machine these are the improvements I am getting

Click me

like_utf8 scalar equals time:   [332.65 µs 332.75 µs 332.92 µs]                                    
                        change: [-11.762% -11.542% -11.363%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  3 (3.00%) high mild
  4 (4.00%) high severe

Benchmarking like_utf8 scalar contains: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 10.0s, enable flat sampling, or reduce sample count to 40.
like_utf8 scalar contains                                                                             
                        time:   [1.9687 ms 1.9700 ms 1.9712 ms]
                        change: [-0.8125% -0.7296% -0.6506%] (p = 0.00 < 0.05)
                        Change within noise threshold.

like_utf8 scalar ends with                                                                            
                        time:   [333.54 µs 333.57 µs 333.60 µs]
                        change: [-6.2180% -6.1563% -6.0802%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low severe
  2 (2.00%) high mild
  5 (5.00%) high severe

like_utf8 scalar starts with                                                                            
                        time:   [354.51 µs 354.59 µs 354.70 µs]
                        change: [-5.8751% -5.8337% -5.7908%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  2 (2.00%) high severe

like_utf8 scalar complex                                                                            
                        time:   [8.2678 ms 8.2691 ms 8.2704 ms]
                        change: [+0.3933% +0.4141% +0.4367%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

nlike_utf8 scalar equals                                                                            
                        time:   [359.67 µs 359.72 µs 359.76 µs]
                        change: [-33.604% -33.579% -33.553%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low severe
  1 (1.00%) low mild
  2 (2.00%) high mild
  1 (1.00%) high severe

nlike_utf8 scalar contains                                                                             
                        time:   [2.0056 ms 2.0071 ms 2.0086 ms]
                        change: [-9.1066% -9.0256% -8.9406%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

nlike_utf8 scalar ends with                                                                            
                        time:   [357.22 µs 357.28 µs 357.34 µs]
                        change: [-35.930% -35.904% -35.861%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low severe
  5 (5.00%) high mild
  1 (1.00%) high severe

nlike_utf8 scalar starts with                                                                            
                        time:   [377.53 µs 377.68 µs 377.84 µs]
                        change: [-33.147% -33.106% -33.066%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

nlike_utf8 scalar complex                                                                            
                        time:   [8.3052 ms 8.3066 ms 8.3081 ms]
                        change: [-2.9553% -2.9313% -2.9080%] (p = 0.00 < 0.05)
                        Performance has improved.

ilike_utf8 scalar equals                                                                             
                        time:   [2.8657 ms 2.8660 ms 2.8663 ms]
                        change: [-4.2960% -4.1779% -4.0962%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

ilike_utf8 scalar contains                                                                             
                        time:   [4.4981 ms 4.4989 ms 4.4997 ms]
                        change: [-56.457% -56.444% -56.432%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high severe

ilike_utf8 scalar ends with                                                                             
                        time:   [2.9145 ms 2.9158 ms 2.9172 ms]
                        change: [-3.4589% -3.3895% -3.3206%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high severe

ilike_utf8 scalar starts with                                                                             
                        time:   [2.9147 ms 2.9154 ms 2.9162 ms]
                        change: [-4.4854% -4.4584% -4.4289%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
  1 (1.00%) high severe

ilike_utf8 scalar complex                                                                            
                        time:   [10.155 ms 10.157 ms 10.158 ms]
                        change: [-1.9673% -1.9384% -1.9101%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

nilike_utf8 scalar equals                                                                             
                        time:   [2.9256 ms 2.9261 ms 2.9267 ms]
                        change: [-2.7385% -2.7064% -2.6726%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe

nilike_utf8 scalar contains                                                                             
                        time:   [4.5042 ms 4.5065 ms 4.5089 ms]
                        change: [-56.448% -56.424% -56.402%] (p = 0.00 < 0.05)
                        Performance has improved.

nilike_utf8 scalar ends with                                                                             
                        time:   [2.9386 ms 2.9390 ms 2.9394 ms]
                        change: [-1.3554% -1.3115% -1.2672%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

nilike_utf8 scalar starts with                                                                             
                        time:   [2.8966 ms 2.8975 ms 2.8983 ms]
                        change: [-3.2629% -3.2297% -3.1958%] (p = 0.00 < 0.05)
                        Performance has improved.

nilike_utf8 scalar complex                                                                            
                        time:   [10.201 ms 10.202 ms 10.205 ms]
                        change: [-2.0189% -1.9831% -1.9477%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

tustvold

Will review properly tomorrow, but I do wonder if we could use ArrayAccessor instead of macros for this?

viirya · 2022-08-25T18:10:47Z

arrow/src/compute/kernels/comparison.rs

@@ -233,6 +233,91 @@ pub fn like_utf8<OffsetSize: OffsetSizeTrait>(
    })
 }

+macro_rules! like_scalar {
+    ($LEFT: expr, $RIGHT: expr) => {{


This can be possibly rewritten as a function using ArrayAccessor.

psvri · 2022-08-25T18:21:14Z

I actually tried that.

There were a lot of if else paths in the kernels. It would mean I would have to pick one of the below options

write a function which would create a closure based on the pattern. This lead to some very ugly code with Fn's being wrapped in boxes. So I didn't prefer that.
Next option is to wrap the if else options inside the closure , but this would mean a comparison would happen for each element which wasn't optimal.
The next option was to use ArrayAccessor and compare_op inside each if else. But that is as good as in lineing the function our self. Hence I went with this approach.

Let me know your thoughts here.

tustvold · 2022-08-25T18:39:30Z

Can you not just take the contents of the macro and make it into a free generic function on a concrete ArrayAccessor<Item=&str>?

psvri · 2022-08-25T18:42:03Z

Okay , let me try that.

…ionary_like

viirya · 2022-08-25T20:26:29Z

arrow/src/compute/kernels/comparison.rs

+    nilike_scalar(left, right)
+}
+
+/// Perform SQL `left ILIKE right` operation on [`DictionaryArray`] with values


Suggested change

/// Perform SQL `left ILIKE right` operation on [`DictionaryArray`] with values

/// Perform SQL `left NOT ILIKE right` operation on [`DictionaryArray`] with values

tustvold

This looks good to me, thank you.

I wonder if we should add dyn versions of these kernels as a follow up? 🤔

tustvold · 2022-08-26T07:41:59Z

arrow/src/compute/kernels/comparison.rs

-                bit_util::set_bit(bool_slice, i);
+            unsafe {
+                if left.value_unchecked(i).ends_with(ends_with) {
+                    bit_util::set_bit(bool_slice, i);


Not something for this PR but using MutableBuffer::from_trusted_len_iter may be significantly faster as it performs byte-size writes instead bit

Tried it just now , I didnt find it giving that significant performance gains.

Fair, I guess the string comparisons are substantially more expensive than for primitives

tustvold · 2022-08-26T09:58:57Z

arrow/src/compute/kernels/comparison.rs

+    match left.value_type() {
+        DataType::Utf8 => {
+            let left = left.downcast_dict::<GenericStringArray<i32>>().unwrap();
+            like_scalar(left, right)


I was actually looking at the implementation of the other dictionary comparison kernels and they opt to instead evaluate the predicate against the dictionary, and then call unpack_dict_comparison to translate this to the values as a whole. Might be something to explore, I could see it being very beneficial for DictionaryArray with lots of repeated values

I agree, Shall I make this change in the same PR ?

Lets do it as a follow up

ursabot · 2022-08-26T22:51:49Z

Benchmark runs are scheduled for baseline = 63afe25 and contender = 9abc5f5. 9abc5f5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

psvri added 6 commits August 23, 2022 17:49

Intial implmentation of like kernels

3d0f47e

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

083eb10

Refactor nlike_scalar kernels

add2044

Fix cargo.toml

5f7b3db

Add other dict scalar kernels

313df58

Merge branch 'master' of https://github.com/apache/arrow-rs into dict…

439e485

…ionary_like

github-actions bot added the arrow Changes to the arrow crate label Aug 25, 2022

tustvold reviewed Aug 25, 2022

View reviewed changes

viirya reviewed Aug 25, 2022

View reviewed changes

psvri added 4 commits August 25, 2022 19:00

Merge remote-tracking branch 'refs/remotes/upstream/master' into dict…

266d723

…ionary_like

Replace macro with array accessor functions

1d57b23

Remove commented code

0a906f8

Fix typo in error message

0744694

viirya reviewed Aug 25, 2022

View reviewed changes

Fix doc comments

9c48b79

tustvold approved these changes Aug 26, 2022

View reviewed changes

tustvold reviewed Aug 26, 2022

View reviewed changes

viirya approved these changes Aug 26, 2022

View reviewed changes

tustvold merged commit 9abc5f5 into apache:master Aug 26, 2022

psvri deleted the dictionary_like branch December 4, 2022 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dictionary like scalar kernels #2591

Dictionary like scalar kernels #2591

psvri commented Aug 25, 2022

psvri commented Aug 25, 2022

tustvold left a comment •

edited

Loading

viirya Aug 25, 2022

psvri commented Aug 25, 2022

tustvold commented Aug 25, 2022 •

edited

Loading

psvri commented Aug 25, 2022

viirya Aug 25, 2022

tustvold left a comment

tustvold Aug 26, 2022

psvri Aug 26, 2022

tustvold Aug 26, 2022

tustvold Aug 26, 2022

psvri Aug 26, 2022

tustvold Aug 26, 2022

ursabot commented Aug 26, 2022

	/// Perform SQL `left ILIKE right` operation on [`DictionaryArray`] with values
	/// Perform SQL `left NOT ILIKE right` operation on [`DictionaryArray`] with values

Dictionary like scalar kernels #2591

Dictionary like scalar kernels #2591

Conversation

psvri commented Aug 25, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

psvri commented Aug 25, 2022

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psvri commented Aug 25, 2022

tustvold commented Aug 25, 2022 • edited Loading

psvri commented Aug 25, 2022

Choose a reason for hiding this comment

tustvold 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

ursabot commented Aug 26, 2022

tustvold left a comment •

edited

Loading

tustvold commented Aug 25, 2022 •

edited

Loading