Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-4865: [Rust] Support list casts #3896

Closed
wants to merge 5 commits into from

Conversation

nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Mar 14, 2019

This is a follow up from the initial cast kernel PR, and adds support for:

  • List to List
  • Primitive to List

The only remaining expansion to the cast kernel will be temporal casts, then I think we'll be able to cover all cast use-cases.

@nevi-me
Copy link
Contributor Author

nevi-me commented Mar 14, 2019

@sunchao @andygrove @paddyhoran PTAL

@codecov-io
Copy link

codecov-io commented Mar 14, 2019

Codecov Report

Merging #3896 into master will increase coverage by 0.82%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3896      +/-   ##
==========================================
+ Coverage   87.85%   88.68%   +0.82%     
==========================================
  Files         726      592     -134     
  Lines       88304    78721    -9583     
  Branches     1252        0    -1252     
==========================================
- Hits        77578    69812    -7766     
+ Misses      10612     8909    -1703     
+ Partials      114        0     -114
Impacted Files Coverage Δ
go/arrow/math/uint64_amd64.go
go/arrow/memory/memory_avx2_amd64.go
js/src/enum.ts
go/arrow/array/builder.go
js/src/Arrow.node.ts
js/src/schema.ts
go/arrow/type_traits_boolean.go
js/src/ipc/node/writer.ts
js/src/visitor/vectorloader.ts
js/src/visitor.ts
... and 126 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 d2e1ee9...cfe0a39. Read the comment docs.

(List(_), List(ref to)) => {
let data = array.data_ref();
let underlying_array = make_array(data.child_data()[0].clone());
let cast_array = cast(&underlying_array, &*to)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: &*to -> to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -131,7 +131,7 @@ pub type ArrayRef = Arc<Array>;

/// Constructs an array using the input `data`. Returns a reference-counted `Array`
/// instance.
fn make_array(data: ArrayDataRef) -> ArrayRef {
pub fn make_array(data: ArrayDataRef) -> ArrayRef {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this pub(crate) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fn test_cast_i32_to_list_i32() {
let a = Int32Array::from(vec![5, 6, 7, 8, 9]);
let array = Arc::new(a) as ArrayRef;
let b = cast(&array, &DataType::List(Box::new(DataType::Int32))).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also test length, offset for each value in the casted list array? same in test_cast_i32_to_list_i32_nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for all tests

Copy link
Contributor

@paddyhoran paddyhoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @nevi-me

@nevi-me
Copy link
Contributor Author

nevi-me commented Mar 15, 2019

@sunchao please see https://issues.apache.org/jira/browse/ARROW-4886, I've disabled casting sliced primitives to lists for now until we can slice list arrays.

@andygrove andygrove closed this in 90d665e Mar 15, 2019
@sunchao
Copy link
Member

sunchao commented Mar 15, 2019

@nevi-me : sounds good. I'll try to get the slice issue fixed ASAP.

BTW: in the null case, how does the conversion from primitive array to list array look like: is a null value converted to null or [null]?

@nevi-me
Copy link
Contributor Author

nevi-me commented Mar 15, 2019

I'll check what happens, my initial thought is a list with a single null, because I would expect [null, null] if I had an array with 2 values being cast

@nevi-me nevi-me deleted the ARROW-4865 branch March 16, 2019 07:06
@nevi-me
Copy link
Contributor Author

nevi-me commented Mar 16, 2019

@sunchao I found the issue. When slicing an array, we count the nulls from a sliced offset bitmask, but we return the original bitmask without the offset applied. I've opened https://issues.apache.org/jira/browse/ARROW-4914

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants