BoundsError on last(::MatrixTable)

[yha]

julia> last(Tables.table(rand(3,2)))
ERROR: BoundsError: attempt to access 3×2 Matrix{Float64} at index [1:3, 3]
Stacktrace:
 [1] throw_boundserror(A::Matrix{Float64}, I::Tuple{Base.Slice{Base.OneTo{Int64}}, Int64})
   @ Base .\abstractarray.jl:651
 [2] checkbounds
   @ .\abstractarray.jl:616 [inlined]
 [3] view
   @ .\subarray.jl:177 [inlined]
 [4] getcolumn
   @ C:\Users\sternlab\.julia\packages\Tables\OWzlh\src\matrix.jl:39 [inlined]
 [5] getindex
   @ C:\Users\sternlab\.julia\packages\Tables\OWzlh\src\Tables.jl:179 [inlined]
 [6] last(a::Tables.MatrixTable{Matrix{Float64}})
   @ Base .\abstractarray.jl:437
 [7] top-level scope
   @ REPL[10]:1

This breaks display of MatrixTable in Pluto.

[bkamins]

The problem is that:

Base.length(m::MatrixTable) = size(getfield(m, :matrix), 1)

which is inconsistent with the general API for AbstractColumns

[quinnj]

which is inconsistent with the general API for AbstractColumns

Not really; the API for AbstractColumns is just that you implement Tables.columnnames and the Tables.getcolumn methods, and then you get various getproperty and getindex methods "for free".

The problem here is that MatrixTable implements the Iterable interface (eltype, length, and iterate), but not the Indexable interface (firstindex, lastindex, getindex), which I guess Pluto is assuming is implemented for some reason? Or rather, it does have the Indexable interface implemented, but it's from the AbstractColumns generic definitions, and doesn't match the Iterable implementation. cc: @fonsp

[yha]

it does have the Indexable interface implemented, but it's from the AbstractColumns generic definitions, and doesn't match the Iterable implementation.

That sounds to me like a bug by itself.
Is there any reason why the MatrixTable object itself must implement these, rather than rows(::MatrixTable) and columns(::MatrixTable)? Pluto accesses the rows through rows anyway, so I think that will solve the problem there too.
EDIT: actually, the indexable interface is also not implemented correctly, since lastindex returns the last row index. But even if this is fixed, and both interfaces are correctly implemented, we would still have last(rows(::MatrixTable)) returning the last column.

[bkamins]

@quinnj - I find it inconsistent as we have:

Base.length(r::RorC) = length(columnnames(r))

which is a part of public API for RorC.

and MatrixTable overrides this definition in an inconsistent way.

[quinnj]

It sounds like others are bringing up the fact that last is an "iterator"-related function, yet relies on the Indexable interface: JuliaLang/julia#43101

@bkamins, yes, as I was reviewing, I thought maybe the iterator/indexable split was ok, but that is a case of inconsistency. It seems we need to return a separate iterator object from Tables.rows on MatrixTable

[bkamins]

I think we should stick to a rule that if a subtype adds a method that supertype supports this method should do the same thing conceptually (of course it can have a different implementation for performance reasons, but should not change how things works).

Reason - assume someone writes a generic code using Tables.jl and relying on how last works on RorC then such code will be SILENTLY broken if it is passed MatrixTable (i.e. it might even work without error but produce a wrong result).

[quinnj]

Ok, fix is up: #262