Inconsistent handling of Tuple and NamedTuple

[bkamins]

Currently we have this:

julia> Tables.columns(NamedTuple{(:a, :b),Tuple{Int,Int}}[])
Tables.CopiedColumns{NamedTuple{(:a, :b),Tuple{Array{Int64,1},Array{Int64,1}}}}: (a = Int64[], b = Int64[])

julia> Tables.columntable(Tuple{Int,Int}[])
NamedTuple()

Is this intended?

(I know it is a corner case but I test against it in DataFrames.jl)

[quinnj]

Somewhat intentional; i.e. we have explicit code in namedtuples.jl that treats any AbstractVector{<:NamedTuple} as a rowtable, and we don't have similar code for AbstractVector{<:Tuple}. We could add that I guess; I don't think it would mess anything else up.

[bkamins]

we don't have similar code for AbstractVector{<:Tuple}

This is how things currently work for non-empty vectors of respective types:

julia> Tables.columntable([(1,2), (3,4)])
(1 = [1, 3], 2 = [2, 4])

julia> Tables.columntable([(a=1,b=2), (a=3,b=4)])
(a = [1, 3], b = [2, 4])

[quinnj]

Yeah, the difference is we have:

schema(x::AbstractVector{NamedTuple{names, types}}) where {names, types} = Schema(names, types)

defined for vector of namedtuples, but no similar definition for vector of tuple. In this case, Tables.columns goes to the schemaless fallback column building routine, but the vector is empty, so we end up with an empty table.

Marking this as "help wanted" if someone wanted to take a stab at implementing this, since it's not too hard and could give someone a taste of Tables.jl internals.

[bkamins]

Yes - this is exactly this difference. Actually this is a more general thing as schema can be inferred from collection eltype for structs also (which currently does not happen):

julia> struct A
       x
       y
       end

julia> Tables.columntable([A(1,2)])
(x = [1], y = [2])

julia> Tables.columntable(A[])
NamedTuple()

[quinnj]

Good point. So instead of just special-casing AbstractVector{<:Tuple}, we could instead enhance the buildcolumns routines if the input is empty to check if there's an eltype we can use to generate a schema.

[bkamins]

This is what I assumed would be best (hopefully someone will be willing to grab it as a hacktoberfest challenge - maybe tomorrow? - I will post on #data).

(if this is not resolved by someone else till DataFrames.jl 0.22 is released I can try to propose a PR)

[quinnj]

I can leave a few bread crumbs for anyone who wants to take a stab at this:

• Here is where we hit this code path (i.e. calling Tables.columns(empty_vector)): as you can see, we just return NamedTuple()
• Instead, we want to check if the rowitr input has an eltype and if so, generate a smarter NamedTuple than just empty.
• We have the allocatecolumns function that will return a "smart" NamedTuple given a Tables.Schema{names, types}, so it's really a matter of generating a Tables.Schema{names, types} from our empty rowitr
• We do know that rowitr will be of type IteratorWrapper and iterates IteratorRow{T}, so I think what we'd need is some inspection code to generate names and types from the T of IteratorRow{T}; whether that be a Tuple, or struct Foo

[quinnj]

@bkamins , on a separate note, even with a potential fix to this issue, we still have issues in DataFrame because we hit this line and all(col isa AbstractVector, x) is true because x is empty. I wonder if we should also check that !isempty(x) so that if it is empty, we fallback to Tables.jl code?

[bkamins]

Right - I will review that constructor and make a PR (as there might be some more logic to add below also). In particular I would change all(col -> isa(col, AbstractVector), x) to eltype(x) <: AbstractVector || eltype(x) === Any I think (it will be faster and should catch the cases when we potentially can have a vector of vectors)

EDIT: it is not that simple - I will propose what I think is OK in the PR

[bkamins]

Fixed in a commit to JuliaData/DataFrames.jl#2464

[quinnj]

Ok, I went ahead and did this: #206

[bkamins]

Thank you!