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

Can't construct a nullable vector from a vector of nullables #98

Open
andyferris opened this issue Mar 1, 2016 · 8 comments
Open

Can't construct a nullable vector from a vector of nullables #98

andyferris opened this issue Mar 1, 2016 · 8 comments

Comments

@andyferris
Copy link
Contributor

Case in point:

julia> NullableVector{Int64}([Nullable{1},Nullable{2},Nullable{3}])
ERROR: MethodError: `convert` has no method matching convert(::Type{Int64}, ::Type{Nullable{1}})
This may have arisen from a call to the constructor Int64(...),
since type constructors fall back to convert methods. 
Closest candidates are:
  call{T}(::Type{T}, ::Any) 
  convert(::Type{Int64}, ::Int8)
  convert(::Type{Int64}, ::UInt8)
  ...
 in copy! at abstractarray.jl:310
 in call at essentials.jl:56

This seems really unintuitive.

I came across this when I tried to make Tables.jl understand about storing Nullable values in NullableArrays, where I have some convenience constructors for when, e.g., a single row (or a small number of rows) of the table are used in instantiation and my constructor needs to convert one or more Nullables in a vector to a NullableVector.

Is there a strong reason this isn't implemented? Would you welcome a PR?

PS - Similar automatic conversion works with Vector:

julia> Vector{Int64}([1.0,2.0,3.0])
3-element Array{Int64,1}:
 1
 2
 3
@johnmyleswhite
Copy link
Member

I think this will work if you define the appropriate convert method.

@andyferris
Copy link
Contributor Author

OK, that works:

julia> function Base.convert{T,N}(::Type{NullableArray{T,N}},in::Array{Nullable{T},N})
           out = NullableArray{T,N}(Array(T,size(in)),Array(Bool,size(in)))
           for i = 1:length(in)
               (out.isnull[i] = in[i].isnull) ? nothing : out.values[i] = in[i].value
           end
           out
       end
convert (generic function with 549 methods)

julia> NullableArray{Int64,1}(x)
4-element NullableArrays.NullableArray{Int64,1}:
     1
     2
     3
 #NULL

But we'll also need the outer constructor:

julia> NullableArray(x)
4-element NullableArrays.NullableArray{Nullable{Int64},1}:
 Nullable(1)      
 Nullable(2)      
 Nullable(3)      
 Nullable{Int64}()

I will attempt a PR with both

@johnmyleswhite
Copy link
Member

You don't need an outer constructor: you just need a different convert method. I think you're missing two cases at least:

  • Base.convert{T,N}(::Type{NullableArray, in::Array{Nullable{T},N})
  • Base.convert{S, T,N}(::Type{NullableArray{S, N}, in::Array{Nullable{T},N})

@andyferris
Copy link
Contributor Author

OK, I had to use an outer constructor instead of Base.convert{T,N}(::Type{NullableArray, in::Array{Nullable{T},N}) because an existing constructor took ownership, but the others are all convert

@davidagold
Copy link
Contributor

Hmm. Define

function Base.convert{T}(::Type{T}, x::Nullable{T})
    if isnull(x)
        error()
    else
        get(x)
    end
end

We get some ambiguity warnings. These aside, we then have the following:

julia> A = [Nullable(1), Nullable(2)];

julia> B = [Nullable(1) Nullable(2); Nullable(1) Nullable(2)];

julia> NullableArray(A)
2-element NullableArrays.NullableArray{Nullable{Int64},1}:
 Nullable{Int64}(1)
 Nullable{Int64}(2)

julia> NullableArray(B)
2x2 NullableArrays.NullableArray{Nullable{Int64},2}:
 Nullable{Int64}(1)  Nullable{Int64}(2)
 Nullable{Int64}(1)  Nullable{Int64}(2)

julia> convert(NullableArray, A)
2-element NullableArrays.NullableArray{Nullable{Int64},1}:
 Nullable{Int64}(1)
 Nullable{Int64}(2)

julia> convert(NullableArray, B)
2x2 NullableArrays.NullableArray{Nullable{Int64},2}:
 Nullable{Int64}(1)  Nullable{Int64}(2)
 Nullable{Int64}(1)  Nullable{Int64}(2)

julia> NullableVector{Int}(A)
2-element NullableArrays.NullableArray{Int64,1}:
 1
 2

julia> NullableVector(A)
ERROR: MethodError: Cannot `convert` an object of type Array{Nullable{Int64},1} to an object of type NullableArrays.NullableArray{T,1}
This may have arisen from a call to the constructor NullableArrays.NullableArray{T,1}(...),
since type constructors fall back to convert methods.
Closest candidates are:
  convert{T}(::Type{T}, ::Nullable{T})
  convert{T}(::Type{T}, ::T)
  (::Type{BoundsError})(::ANY)
  ...
 in NullableArrays.NullableArray{T,1}(::Array{Nullable{Int64},1}) at ./int.jl:419
 in eval(::Module, ::Any) at ./boot.jl:267

Not sure which I prefer: introducing conversion methods at the NullableArray level or at the Nullable level. The issue with the latter is that it involves filing the PR with Base, and it's not clear that they want to introduce such sweeping behavior. But it also seems more to the point.

@andyferris
Copy link
Contributor Author

Sure, a convert method in Base might make sense, much like we can convert a Float to an Int (if it is close to an integer) or a Complex to a Real (if its imaginary part is small). InexactError would probably suit for null values.

But I think it is orthogonal/additional to this discussion. I can't see that your approach will work when some of the Vector{Nullable{Int64}} is null. Some logic needs to be applied to unpack one storage format and putting it in the other, and IMHO that belongs here.

I suppose it would be more "complete" if we also defined the reverse transformation, but it's so easily done via collect...

@davidagold
Copy link
Contributor

@andyferris That's a good point. I hope to be able to take a closer look at your PR tomorrow and get it merged shortly.

@davidagold
Copy link
Contributor

Is this resolved with #99 ?

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

No branches or pull requests

3 participants