ARROW-5351: [Rust] Take kernel #4330

nevi-me · 2019-05-16T21:50:38Z

https://issues.apache.org/jira/browse/ARROW-5351

Implement take kernel, initial draft that hasn't been benchmarked, and is expected to be rough around the edges.

nevi-me · 2019-05-16T22:11:50Z

@sunchao I've used ArrayData comparison to test, I had expected this test to fail because of this.

I use Windows, so it looks like ArrayData sometimes fails on Windows when comparing 2 similar arrays by their data. The reason being that null/invalid array slots return non-deterministic data.

I'll attach an example tomorrow when I continue with this PR.

I'm mentioning this because you suggested that we avoid using string/debug for comparison of arrays

sunchao · 2019-05-16T23:02:41Z

Yes, more efforts are required to improve equality check for ArrayData, Array and others. Currently they just use the derived equality which is not good. We should find a way to ignore the null/invalid data when doing the equality check.

nevi-me · 2019-05-23T16:52:24Z

@andygrove @sunchao @paddyhoran I have a few decisions to make, and would appreciate some guidance/opinions.

I'm mostly done with this, but need to optimise bounds checking and implement options (such as whether to even check for bounds).

Question: pandas, numpy, and the C++ version (cc @bkietz) have the option of supplying negative indices. Is this something that we'd want in Rust? If so, I'll change the function's index parameter to Int32Array from the current unsigned one.

sunchao · 2019-05-24T02:24:41Z

What does a negative index mean? can you show some code examples where this is applied?

nevi-me · 2019-05-24T03:56:02Z

@sunchao here's a good brief reference from Wes https://issues.apache.org/jira/browse/ARROW-772?focusedCommentId=16340235&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16340235

bkietz · 2019-05-24T13:16:06Z

It's worth noting that out of bounds and negative indices are currently just an error in C++

nevi-me · 2019-05-24T13:45:37Z

Thanks @bkietz, I noticed that there's open JIRAs for them. I wanted to get them out of the way earlier so we can stabilise the take signature.

nevi-me · 2019-06-02T17:11:14Z

I've done all I can here.

I noticed that if I have an array with 6 values, array.is_null(6) and array.is_null(7) won't panic because the array length's within a multiple of 8; so the 7th and 8th values are returned as null in the buffer. I don't know if this is an issue, and if we should document this behaviour.

wesm · 2019-06-12T16:01:45Z

@sunchao can you review? It seems then #4331 can be finished after this is merged

sunchao

Thanks @nevi-me for the update and sorry for the delay on reviewing!

sunchao · 2019-06-12T16:42:25Z

rust/arrow/src/compute/kernels/take.rs

Why use import here instead of putting it on the top-level?

Just a habit, I've become accustomed to importing enums inside the scope that I need them, to avoid repeating TimeUnit::A, TimeUnit::B, etc. Would you prefer I move it to the top-level?

I'm not sure whether there's a "official style guide" on this but it may be better to put it in top-level to 1) make it easier to find out what are the dependencies for a module, 2) potentially avoid repeating the use clause in all the places that need it.

sunchao · 2019-06-12T16:43:09Z

rust/arrow/src/compute/kernels/take.rs

nit: instead of Option<&TakeOptions> can we use Option<TakeOptions>? it might be more convenient to use.

sunchao · 2019-06-12T16:49:02Z

rust/arrow/src/compute/kernels/take.rs

Should we call this indices?

sunchao · 2019-06-12T16:49:19Z

rust/arrow/src/compute/kernels/take.rs

Same as above, s/index/indices?

sunchao · 2019-06-12T16:57:48Z

rust/arrow/src/compute/kernels/take.rs

Similar to the C++ version, it may be better if we can optimize this by considering whether values/indices are all valid or not. This doesn't necessarily have to be done in this PR though.

Do you mean checking if we have 0 nulls and bypassing the null checks? I'll have a look at what C++ does, and can update this in a subsequent PR

Yes exactly. Sure it can be done in a follow-up.

sunchao · 2019-06-12T17:15:06Z

rust/arrow/src/compute/kernels/take.rs

Should we call this values? array is self explanatory given that the type is ArrayRef.

sunchao · 2019-06-12T17:30:26Z

rust/arrow/src/compute/util.rs

pub(crate could be changed to pub(super) if this is only used within the compute module? or, is it better to put this in take.rs?

the name is a little bit misleading - it gives the impression that this is taking indices from a list - maybe rename to take_value_indices_from_list or something else?

Can we keep it here for now? I wanted to use it in a sort kernel, which would sort an array and return indices; then use this function (for lists) to take the appropriate value indices. Agree on the name, I like your suggestion

I see. Yes lets keep it here then.

sunchao · 2019-06-12T17:33:07Z

rust/arrow/src/compute/kernels/take.rs

Some optimizations can be done here such as if it is taking the whole list or a contiguous sublist. This can be left as a TODO though.

sunchao · 2019-06-12T17:35:34Z

rust/arrow/src/compute/kernels/take.rs

Is this correct? seems it is using value array's null count.

Hi @sunchao, this was quite difficult to deal with; but I've fixed it. The index's null count was only correct if the values array had no null counts. I now recompute the null count from the offsets, where an offset of vec![0,1,1,2,2] contains 2 null values as 1 and 2 repeat. I'm getting ready to push the latest changes.

sunchao · 2019-06-12T17:36:56Z

rust/arrow/src/compute/kernels/take.rs

nit: might be better to use ArrayDataBuilder here.

I still prefer ArrayData::new() as it's less verbose and reduces my chances of missing something with the builder, but I'll change to ArrayDataBuilder

Ideally ArrayDataBuilder should do a validation to make sure itself is well-formed before returning the constructed ArrayData. The advantage with ArrayDataBuilder is that 1): you don't need to handle default values by passing None; 2) it is a little bit clearer on the argument-parameter mapping (e.g., which buffer is used as null bit map, which buffer is value buffer, etc).

nevi-me · 2019-07-06T14:35:25Z

Hi @sunchao, PTAL. I've used ArrayEqual for my comparisons, correctly calculated list offsets, and fixed other review comments. Apologies for taking very long on this, haven't had capacity in past weeks. I'm available now again, so I'll look at other Rust work.

@liurenjie1024 @andygrove @paddyhoran may you please also review when you can. Thanks

codecov-io · 2019-07-07T00:42:44Z

Codecov Report

Merging #4330 into master will decrease coverage by 0.13%.
The diff coverage is 93.15%.

@@            Coverage Diff             @@
##           master    #4330      +/-   ##
==========================================
- Coverage   82.62%   82.49%   -0.14%     
==========================================
  Files         335       87     -248     
  Lines       43377    24787   -18590     
  Branches     1418        0    -1418     
==========================================
- Hits        35841    20448   -15393     
+ Misses       7174     4339    -2835     
+ Partials      362        0     -362

Impacted Files	Coverage Δ
rust/arrow/src/array/builder.rs	`92.3% <100%> (+0.04%)`	⬆️
rust/arrow/src/array/array.rs	`92.57% <100%> (+0.21%)`	⬆️
rust/arrow/src/compute/kernels/take.rs	`92.5% <92.5%> (ø)`
rust/arrow/src/compute/util.rs	`88.57% <95.34%> (+10.79%)`	⬆️
r/src/recordbatch.cpp
r/R/Table.R
js/src/util/fn.ts
r/src/symbols.cpp
r/R/Column.R
go/arrow/ipc/file_reader.go
... and 246 more

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 7838886...6e7af2d. Read the comment docs.

sunchao

Thanks @nevi-me ! This looks good. I just have a few more nits - after that we can get this committed.

sunchao · 2019-07-09T15:49:24Z

rust/arrow/src/compute/kernels/take.rs

nit: do we still need clone here?

sunchao · 2019-07-09T15:55:07Z

rust/arrow/src/compute/kernels/take.rs

This may be a little confusing. Should we document the behavior of the take method using an example? e.g., an index array with both null & non-null elements, and a value array with both null & non-null elements.

I've added an example, but not as a runnable doc example as the function is private. PTAL

sunchao · 2019-07-09T16:09:09Z

rust/arrow/src/compute/kernels/take.rs

nit: let's fix the format: TODO Some -> TODO: some. Also break this long line into two.

sunchao · 2019-07-09T16:09:58Z

rust/arrow/src/compute/util.rs

nit: add a : after TODO.

sunchao · 2019-07-09T16:16:36Z

rust/arrow/src/compute/util.rs

nit: we can check this right after line 75, on whether start = end, so that we don't need to do the extra map operation.

sunchao · 2019-07-09T16:16:49Z

rust/arrow/src/compute/util.rs

nit: remove this line?

Tests are still incomplete, and there aren't benchmarks yet

runtime is linear as would be expected

nevi-me · 2019-07-09T20:38:09Z

Thanks @sunchao, I've addressed your feedback.

sunchao · 2019-07-10T16:58:45Z

Looks good @nevi-me ! could you try to commit this yourself as you're already a committer now? it's also good for you to learn about the workflow :)

nevi-me · 2019-07-13T10:51:26Z

Thanks @sunchao, I'm still setting up machine for the process, I'll commit it 😃🙏🏾

https://issues.apache.org/jira/browse/ARROW-5351 Implement `take` kernel, initial draft that hasn't been benchmarked, and is expected to be rough around the edges. Author: Neville Dipale <[email protected]> Closes #4330 from nevi-me/ARROW-5351 and squashes the following commits: 6e7af2d <Neville Dipale> address review comments 2467241 <Neville Dipale> address review feedback 0fc3f73 <Neville Dipale> address review comments 1adfccd <Neville Dipale> update tests, add bounds test 38c7a23 <Neville Dipale> add take kernel to mod exports b29e160 <Neville Dipale> add some benchmarks 5f44899 <Neville Dipale> add list and struct tests 0301468 <Neville Dipale> complete take functions for different arrays c675410 <Neville Dipale> ARROW-5351: Take kernel

sunchao added the Component: Rust label May 16, 2019

nevi-me mentioned this pull request May 22, 2019

ARROW-5357: [Rust] Change Buffer::len to represent total bytes instead of used bytes #4331

Closed

nevi-me force-pushed the ARROW-5351 branch from 0f7d5e7 to b7b652c Compare May 23, 2019 16:49

nevi-me marked this pull request as ready for review May 23, 2019 21:26

nevi-me force-pushed the ARROW-5351 branch 2 times, most recently from c37d72f to b124dd4 Compare June 2, 2019 16:52

sunchao reviewed Jun 12, 2019

View reviewed changes

nevi-me force-pushed the ARROW-5351 branch from b124dd4 to 11e0478 Compare July 6, 2019 14:32

sunchao approved these changes Jul 9, 2019

View reviewed changes

nevi-me added 9 commits July 9, 2019 20:34

ARROW-5351: [Rust] Take kernel

c675410

complete take functions for different arrays

0301468

Tests are still incomplete, and there aren't benchmarks yet

add list and struct tests

5f44899

add some benchmarks

b29e160

runtime is linear as would be expected

add take kernel to mod exports

38c7a23

update tests, add bounds test

1adfccd

address review comments

0fc3f73

address review feedback

2467241

address review comments

6e7af2d

nevi-me force-pushed the ARROW-5351 branch from 11e0478 to 6e7af2d Compare July 9, 2019 20:35

nevi-me closed this in a54888a Jul 17, 2019

nevi-me deleted the ARROW-5351 branch October 17, 2020 19:18

asfimport mentioned this pull request Aug 1, 2019

[Rust] Add support for take kernel functions #21810

Closed

ARROW-5351: [Rust] Take kernel #4330

ARROW-5351: [Rust] Take kernel #4330

Uh oh!

Conversation

nevi-me commented May 16, 2019

Uh oh!

nevi-me commented May 16, 2019

Uh oh!

sunchao commented May 16, 2019

Uh oh!

nevi-me commented May 23, 2019

Uh oh!

sunchao commented May 24, 2019

Uh oh!

nevi-me commented May 24, 2019

Uh oh!

bkietz commented May 24, 2019

Uh oh!

nevi-me commented May 24, 2019

Uh oh!

nevi-me commented Jun 2, 2019

Uh oh!

wesm commented Jun 12, 2019

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me commented Jul 6, 2019

Uh oh!

codecov-io commented Jul 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me Jun 18, 2019 •

edited

Loading

nevi-me Jun 18, 2019 •

edited

Loading

codecov-io commented Jul 7, 2019 •

edited

Loading