JuliaStats / NullableArrays.jl

DEPRECATED Prototype of the new JuliaStats NullableArrays package
Other
35 stars 21 forks source link

Ensure that `copy` and `getindex` maintain `parent` field #202

Closed spurll closed 7 years ago

spurll commented 7 years ago

Closes #199

codecov-io commented 7 years ago

Codecov Report

Merging #202 into master will decrease coverage by 0.54%. The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   48.17%   47.62%   -0.55%     
==========================================
  Files          14       14              
  Lines        1013     1010       -3     
==========================================
- Hits          488      481       -7     
- Misses        525      529       +4
Impacted Files Coverage Δ
src/primitives.jl 65.41% <100%> (ø) :arrow_up:
src/constructors.jl 56.6% <50%> (ø) :arrow_up:
src/reduce.jl 57.6% <0%> (-3.2%) :arrow_down:
src/operators.jl 63.26% <0%> (-2.12%) :arrow_down:

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 d44f078...60ba8a1. Read the comment docs.

spurll commented 7 years ago

Hm, I think that Codecov report is wrong, since I didn't touch most of those impacted files.

However, I realise that I'll need to make another commit to solve an outstanding issue related to sorting DataTables, so I'm going to mark this WiP.

ararslan commented 7 years ago

Hm, I think that Codecov report is wrong

Yeah, coverage is weird.

However, I realise that I'll need to make another commit to solve an outstanding issue related to sorting DataTables

Is that directly related to this change?

spurll commented 7 years ago

The problem I'm currently trying to solve is the fact that the new NullableArray created by getindex doesn't maintain the parent reference either:

julia> str = "something"
"something"

julia> arr = [WeakRefString(pointer(str, 1), 4, 1), WeakRefString(pointer(str, 5), 5, 5)]
2-element Array{WeakRefString{UInt8},1}:
 "some" 
 "thing"

julia> na = NullableArray{WeakRefString{UInt8}, 1}(arr, [false, false], Vector{UInt8}(str))
2-element NullableArrays.NullableArray{WeakRefString{UInt8},1}:
 "some" 
 "thing"

julia> na.parent
9-element Array{UInt8,1}:
 0x73
 0x6f
 0x6d
 0x65
 0x74
 0x68
 0x69
 0x6e
 0x67

julia> na[:].parent
0-element Array{UInt8,1}

It's certainly related, though maybe it warrants a separate PR.

spurll commented 7 years ago

This should now be good to go. Though it's possible that the change I made to similar will be contentious, it is necessary to maintain the parent reference without massive changes to the codebase.