Consider making `setproperty` copy `Vector`

[oxinabox]

This is infollow up to other threads trying to remove all the ways users can resize columns out of sync and thus break the DataFrame,

#1844 (comment)

There would be still one hole:

df.a = vector

One option, and I am not completely sold on it, but that I feel is worth considering.
Is that we could make setproperty and single argument setindex,
copy the input.

This would be consistent with the behavior of the constructor as copying the Vectors,

But unlike that there is no easy way to pass in a copycols=false kwarg or a Dataframe! constructor.

So if we were to do that, I suggest we would have a @nocopy macro to bypass this.,
so that

@nocopy df.a = vector

would not copy vector but would dispatch to some alternative nocopy_setcol
where as

df.a = vector

would dispatch to setcol (which would intern dispatch to nocopy_setcol(df, col, copy(vector))

[bkamins]

Actually we could have setcol and getcol with copycols kwarg and then decide what df.a and df[:a] should expand to. If we added setcol and getcol then probably we should rename haskey to hascol and add colindex as discussed earlier with @nalimilan.

But let us wait a bit to hear what other people think that df.a should be a default behavior.

[oxinabox]

We should focus on setcol here, since #1844 is basically on getcols, and we don't have pressing need to make them match (copying input is very different to copying output)

I think it makes sense to make it take a copycols kwarg.
I am not convinced that defaulting that to true is a good idea,
but

Pros of Defaulting copycols on setcol to true

• it would be consistent with the constructor,
• safe by default

Pros of Defaulting copycols on setcol to false

• it would be consistant with setfield on a normal struct
• faster and less allocating

(Ideally we just want copy on write still, or at least for julia to optimize away any copies of vectors that are going to go out of scope according to escape analysis. But neither of those seems likely any time soon.)

[bkamins]

we don't have pressing need to make them match

Not pressing 😄, but consistency in the design is very important, as then users, even if they do not know the documentation in detail, tend to do the right things (but yes - we should have separate issues to discuss it, I just wanted to put here the whole possible target design in one place).

So my intuition is that we should go either for copy by default here and in #1844 or no copy by default (and in the no copy case view could be considered in getcol, if we copy by default then probably view is not necessary in getcol as an option).

[oxinabox]

we don't have pressing need to make them match
Not pressing 😄, but consistency in the design is very important

To be clear, I agree. Note that I referenced consistency in both my dot points (for the constructors and for normal setfield respectively).
But consistency between set and get copying is not one I feel is particularly intuitive.
I can see an intuition that would expect them to be antisymetric.
This would come from an incorrect assumption that
"if it was copied on the way in, why would it be copied on the way out?".
They are rather different operations. to me. Even though it boils down to copying the right hand side,
if one thinks of it as a property of the thing being copied then it doesn't hold.
in x = df.y then it is the dataframe column being copied, not the vector.
whereas in df.y = x it is the vector being copied, not dataframe column.

I don't think it would be too unnatural to copy on the way in for purposes of matching how the constructor, and hcat and join, work.
Following a consistent logic that all added columns are always copies.

And then having some other behavior for getcol because efficiency matters, and columns are read more often than they are added; and so the tradeoff of being a bit more consitent on the copying-ness of getcol and setcol should match (in what amounts to an implementation detail if we have actually plugged all the holes some way or another), is not worth it.

Another scattered thought:

I do not like the idea that df2.x = df1.x would do two copies.
I would like to avoid that world.

[nalimilan]

Having df.col = vector make a copy indeed sounds consistent with what we now do with constructors. But that would mean we need a way to avoid making a copy, either using @nocopy df.col = vector, @nocopy df[col] = vector and/or setcol!(df, vector, copycols=false).

As a data point, do you know how easy/clean it would be to implement @nocopy? If we went that route, maybe it could be used elsewhere too.

[oxinabox]

how easy/clean it would be to implement @NoCopy? If we went that route, maybe it could be used elsewhere too.

There are many ways to do this (TensorFlow.jl does a similar tranform using traits to indicate what ops should change, but for simplicty here is one that just lists them all

The simple version is simple, makng it recursive and making it handle various different ways to express the same call is a bit more fiddly

using MacroTools

nocopy_setproperty(args...) = setproperty(args...)
nocopy_setproperty(df::AbstractDataFrame, args...) = setcol(D)

const no_copy_ops = (DataFrame, setproperty, ...)

macro nocopy(expr)
    expr isa Expr && expr.head ==:call && expr.args[1] in no_copy_ops
        expr.args[1] = subexpr.args[1] = getfield(DataFrames, Symbol(:nocopy_subexpr.args[1]))
    end
    return expr
end

macro recurisive_nocopy(expr)
    MacroTools.postwalk(expr) do subexpr
        if subexpr isa Expr && subexpr.head==:call
            if subexpr.args[1] in no_copy_ops
                # handle foo(x)
                subexpr.args[1] = getfield(DataFrames, Symbol(:nocopy_subexpr.args[1]))
            elseif subexpr.args[1] isa Expr 
                subsubexp = subexpr.args[1]
                # handle DataFrames.foo(x)
                if && subexpr.head == :. && subsubexpr.args[2] in no_copy_ops
                    subsubexpr.args[2] = getfield(DataFrames, Symbol(:nocopy_subexpr.args[1]))
                end
            end
            #TODO: handle DataFrames.Base.foo() etc this should be via recursion while there are :. s
        end
        return subexpr
    end
end

[bkamins]

If we decided to define setcol! and require @nocopy to be directly in front of the assignment
then the macro can be relatively simple I think.
It would just rewrite df.col = some_expression and df[col] = some_expression as setcol!(df, col, some_expression, copycols=false).

[bkamins]

In general the question is if we need this @nocopy macro and if setcol! would not be enough. I am not sure what would be the best API here.

[quinnj]

Here's a slightly more radical idea for the more general issue of preventing non-DataFrames column mutations (which can lead to undesirable column states): the jl_array_t source type (basically the C-equivalent of Array) has a "shared array" bit, which prevents array resizing when flipped on (as far as I know this is only set by mmapped arrays and SharedArrays). We could potentially convince @JeffBezanson and/or @vtjnash to export C-side getter/setter for setting this "shared" bit on Arrays. Then DataFrames could set the bit when ingesting, and unset->mutate->set on mutating operations. That would mean if users still had references to the original vectors, doing resizing would fail (of course unless they do their own unset->mutate operation, which hopefully would be used appropriately).

Now, this isn't quite a silver bullet because we still have the myriad custom array types, but perhaps this could become a formal API of AbstractArray types; in fact, now that I think about it, @Keno threw out some ideas already in JuliaLang/julia#31630 around a freeze/melt API formally, but geared more towards potential compiler optimizations.

[vtjnash]

There's no end to the set of things that users could do to make your application state inconsistent. At best you can just make it more tedious. We track that bit to avoid memory corruption (so we can at least given decent errors as it falls apart). In the future, I hope it'll get subsumed by the Buffer type.

But I suspect that requiring magic code sequences like unset->mutate->set or freeze/melt will just lead to more awkward source code on the whole for little or no benefit. It seems a bit like the const annotation in C(++): seems great on paper, but doesn't actually mean what you want, gets in the way much of time, and can't actually enable the optimizations you usually wanted.

[NoCopy]

I guess I was called out by accident. No worries.

Looks like you have a good convo rolling and know your shit. Respect my friends.

[bkamins]

So what is the conclusion here?

• should df.col = vector make a copy (I understand yes)
• should we define setcol! (I am not sure - it depends on what we decide with @nocopy as we should provide an operation allowing to add a vector to a data frame without copying)
• should we define @nocopy (I am not very convinced - this is a nice idea, but it might be too magic to normal users - but maybe I am wrong)

[oxinabox]

• should df.col = vector make a copy

Yes.

should we define setcol!

Yes, which takes a copycols=true kwarg.

should we define @nocopy

Maybe, but the way it should work if we do is to convert calls to setproperty and setindex into setcol!(...; copycols=false).
So we do still need the setcol!

[bkamins]

I will introduce it with setindex! PR which is next in the queue after #1847 and #1840 are merged.
Also then #1844 can be implemented in parallel to setindex! PR.