fix vcat type piracy

[bkamins]

Fixes #3456

[MasonProtter]

This is still piracy though. You're now pirating yet another method, this time it should hopefully give desired behaviour, but this will likely end up causing problems.

Fundamentally, the reduce(vcat thing is just piracy. Type parameters don't count for "owning" a type, so when you write

Base.reduce(::typeof(vcat), ::AbstractVector{<:AbstractDataFrame}) = ...

that just isn't really allowed because you don't own reduce, you don't own vcat, and you don't own AbstractVector.

[bkamins]

I agree. I used the simplest solution. In general this is not a type piracy I think as I own AbstractDataFrame. The problem is that <:AbstractDataFrame allows for Union{}. I pushed a fix, which is not elegant but entirely avoids type piracy.

[MasonProtter]

In general this is not a type piracy I think as I own AbstractDataFrame.

Unfortunately, this is not really the case. At least according to the language devs, owning a parameter does not count as owning a type. So owning T does not mean you own Vector{T}.

[bkamins]

This is interesting. Could you please provide a reference for my further exploration. Thank you!

[MasonProtter]

This really needs to be documented better, but there's this: https://docs.julialang.org/en/v1/manual/style-guide/#Don't-overload-methods-of-base-container-types and also this sentence in the following section in piracy:

"Type piracy" refers to the practice of extending or redefining methods in Base or other packages on types that you have not defined.

An easy aspect of this to overlook is that while you have defined the type DataFrame, you have not defined the type Vector{DataFrame}.

See also this conversation: https://julialang.slack.com/archives/C688QKS7Q/p1723647223155419

and this one: https://julialang.zulipchat.com/#narrow/stream/225542-helpdesk/topic/is.20this.20type.20piracy.3F/near/313431232

Here's my understanding of what's wrong with this sort of 'parametric piracy':

Let's suppose there's a Base.f(::Vector). When Base writes code that calls Base.f(::Vector) they are calling a function on a concrete type, and so they think they know exactly what the implementation of it should do. This is vital for various 'internals' of Base and also lots of other packages. However, if you go and write a method like Base.f(::Vector{YourT}), you have actually written and even more specific method than the concrete method Base already wrote, and there is no way for Base to preemptively define an even more specific method, which causes all sorts of implementation headaches for Base (or whatever other package).

[bkamins]

@nalimilan - can we merge this PR as it is?
We will not remove this method for now, but at least currently, the type piracy issue is minimized.

[nalimilan]

Could you push a commit that moves the code and another one that changes the signature? Otherwise it will be hard to understand what changed when looking at the history.

Maybe it would make sense to add tests with e.g. a Vector{Union{DataFrame, SubDataFrame}}and a Vector{AbstractDataFrame} if we don't have one already to ensure this doesn't regress?

[bkamins]

@nalimilan - OK. I did both.

[bkamins]

Thank you!