Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

WIP: allow value array to be any AbstractArray #170

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shashi
Copy link

@shashi shashi commented Dec 27, 2016

This makes possible construction of NullableArray of StructOfArrays, or other funky arrays which solve orthogonal problems.

The following fails with this patch applied:

julia> isequal(NullableArray(Union{},1), NullableArray(Float64,1))
ERROR: indexing not defined for NullableArrays.NullableArray{Union{},1}
 in _getindex(::Base.LinearFast, ::NullableArrays.NullableArray{Union{},1}, ::Int64) at ./abstractarray.jl:763
 in next at ./iterator.jl:98 [inlined]
 in isequal(::NullableArrays.NullableArray{Union{},1}, ::NullableArrays.NullableArray{Float64,1}) at ./abstractarray.jl:1385

Stepping through this with Gallium doesn't fail with this error, it instead succeeds in returning true! cc @Keno

@shashi
Copy link
Author

shashi commented Dec 27, 2016

Note that there are no problems if we are comparing nullablearrays of other types:

julia> isequal(NullableArray(Int64,1), NullableArray(Float64,1))
true

This strikes me as strange (it is possible I'm doing something really stupid). I suspect the problem is Union{} and inlining but I don't know how to dive deeper into this...

@shashi
Copy link
Author

shashi commented Dec 27, 2016

Super weird: test passes on 0.4 and 0.5 on travis but appveyor fails with the same error

image

@@ -19,7 +19,7 @@ It allows users to easily define operations on arrays with null values by
reusing operations that only work on arrays without any null values.
"""
immutable NullableArray{T, N} <: AbstractArray{Nullable{T}, N}
values::Array{T, N}
values::AbstractArray{T, N}
Copy link
Member

Choose a reason for hiding this comment

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

This should be made a type parameter of the array or it's going to kill performance.

@nalimilan
Copy link
Member

Just bumped into this, might be related to the bug you're getting? JuliaLang/julia#10326

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

Successfully merging this pull request may close these issues.

2 participants