CategoricalArray constructor sometimes does not copy all data

[bkamins]

See
JuliaData/DataFrames.jl#1309 (comment)
for an example

The problem is that lines:
https://github.com/JuliaData/CategoricalArrays.jl/blob/master/src/array.jl#L269
and
https://github.com/JuliaData/CategoricalArrays.jl/blob/master/src/array.jl#L270
do not guarantee that a copy is performed (sometimes convert returns the reference to the original object).

CC @nalimilan

[bkamins]

Just to add. The problem is that after:

x = categorical(1:10)
y = categorical(x)

we have that x and y are different objects (if they were the same this would be a normal default behavior like convert).

So the decision is to make: should categegorical always create a new object.
If yes - then it should guarantee to copy everything.
If no - then we should have a function that performs copy of pool and refs (but probably not a deepcopy of everything).

[nalimilan]

Good catch. The problem is not in convert, though, as it shouldn't make a copy unless necessary. It's the constructor which should copy the refs just like we currently do for the pool here. Comments should also be adapted to say that constructors should always make copies (for reference, design decision happened here: JuliaLang/julia#23107).

[bkamins]

I support making a copy.
Do I understand correctly that you would make an appropriate PR?
(I could try this PR, but as you can see from the above comment I do not know the details of design of CategoricalArrays so I do not want to mess up something).

[nalimilan]

I'm quite busy right now with other PRs, so I'd appreciate if you could do it. Most of the work should be to write a test I think.

[bkamins]

I have made a PR.

[bkamins]

closing as this is fixed.