Make sure that CategoricalArray constructor allocates new pool and refs (JuliaData#110)

Following the convention established in Base.

Author: bkamins

src/array.jl

@@ -35,7 +35,7 @@ Similar to definition above, but uses reference type `R` instead of the default
 
     CategoricalArray(A::AbstractArray; ordered::Bool=false)
 
-Construct a `CategoricalArray` with the values from `A` and the same element type.
+Construct a new `CategoricalArray` with the values from `A` and the same element type.
 
 If the element type supports it, levels are sorted in ascending order;
 else, they are kept in their order of appearance in `A`. The `ordered` keyword
@@ -154,16 +154,13 @@ CategoricalMatrix{T}(m::Int, n::Int; ordered=false) where {T} =
 
 ## Constructors from arrays
 
-# This method is needed to ensure that a copy of the pool is always made
-# so that ordered!() does not affect the original array
 function CategoricalArray{T, N, R}(A::CategoricalArray{S, N, Q};
                                    ordered=_isordered(A)) where {S, T, N, Q, R}
     V = unwrap_catvaluetype(T)
     res = convert(CategoricalArray{V, N, R}, A)
-    if res.pool === A.pool # convert() only makes a copy when necessary
-        res = CategoricalArray{V, N}(res.refs, deepcopy(res.pool))
-    end
-    ordered!(res, ordered)
+    refs = res.refs === A.refs ? deepcopy(res.refs) : res.refs
+    pool = res.pool === A.pool ? deepcopy(res.pool) : res.pool
+    ordered!(CategoricalArray{V, N}(refs, pool), ordered)
 end
 
 function CategoricalArray{T, N, R}(A::AbstractArray; ordered=_isordered(A)) where {T, N, R}

test/13_arraycommon.jl

@@ -579,7 +579,7 @@ end
         @test y.refs == x.refs
         @test index(y.pool) == index(x.pool)
         @test levels(y) == levels(x)
-        @test (y.refs === x.refs) == (eltype(x.refs) === eltype(y.refs))
+        @test y.refs !== x.refs
         @test y.pool !== x.pool
     end
     for y in (categorical(x),
@@ -591,7 +591,7 @@ end
         @test y.refs == x.refs
         @test index(y.pool) == index(x.pool)
         @test levels(y) == levels(x)
-        @test (y.refs === x.refs) == (eltype(x.refs) === eltype(y.refs))
+        @test y.refs !== x.refs
         @test y.pool !== x.pool
     end
     for y in (CategoricalArray(x, ordered=ordered),
@@ -611,7 +611,7 @@ end
         @test y.refs == x.refs
         @test index(y.pool) == index(x.pool)
         @test levels(y) == levels(x)
-        @test (y.refs === x.refs) == (eltype(x.refs) === eltype(y.refs))
+        @test y.refs !== x.refs
         @test y.pool !== x.pool
     end
     for y in (categorical(x, ordered=ordered),
@@ -623,7 +623,7 @@ end
         @test y.refs == x.refs
         @test index(y.pool) == index(x.pool)
         @test levels(y) == levels(x)
-        @test (y.refs === x.refs) == (eltype(x.refs) === eltype(y.refs))
+        @test y.refs !== x.refs
         @test y.pool !== x.pool
     end
     for y in (convert(CategoricalArray, x),
@@ -747,4 +747,18 @@ end
     @test vcat(z1, z2) isa CategoricalVector{Float64}
 end
 
+@testset "categorical() makes a copy of pool and refs" begin
+    xs = Any[Int8[1:10;], [Int8[1:10;]; missing]]
+    for x in xs, o1 in [true, false], o2 in [true, false], T in [Int64, Int8]
+        y = categorical(x, ordered=o1)
+        if x === xs[1]
+            z = CategoricalArray{T}(y, ordered=o2)
+        else
+            z = CategoricalArray{Union{T, Missing}}(y, ordered=o2)
+        end
+        @test z.refs !== y.refs
+        @test z.pool !== y.pool
+    end
+end
+
 end