Repository: JuliaData/Tables.jl (julia)

Issue:
default propertynames/getproperty and columnnames/getcolumn definitions for AbstractRow are mutually recursive
This might just be a documentation issue (or I could've missed something), but:

```jl
julia> struct Foo <: Tables.AbstractRow
           x::Int
       end

julia> propertynames(Foo(1))
ERROR: StackOverflowError:
Stacktrace:
 [1] propertynames(::Foo) at /Users/jarrettrevels/.julia/dev/Tables/src/Tables.jl:167
 [2] columnnames(::Foo) at /Users/jarrettrevels/.julia/dev/Tables/src/Tables.jl:105
 ... (the last 2 lines are repeated 39990 more times)
 [79983] propertynames(::Foo) at /Users/jarrettrevels/.julia/dev/Tables/src/Tables.jl:167

julia> Tables.columnnames(Foo(1))
ERROR: StackOverflowError:
Stacktrace:
 [1] columnnames(::Foo) at /Users/jarrettrevels/.julia/dev/Tables/src/Tables.jl:105
 [2] propertynames(::Foo) at /Users/jarrettrevels/.julia/dev/Tables/src/Tables.jl:167
 ... (the last 2 lines are repeated 39990 more times)
 [79983] columnnames(::Foo) at /Users/jarrettrevels/.julia/dev/Tables/src/Tables.jl:105
```

ref https://github.com/JuliaData/Tables.jl/blob/fdf4d114bbca31e547c18cde9c18ff84ac093e09/src/Tables.jl#L105

ref https://github.com/JuliaData/Tables.jl/blob/fdf4d114bbca31e547c18cde9c18ff84ac093e09/src/Tables.jl#L167

We can fix either by breaking the mutual recursion (e.g. delete the `propertynames(::RorC)` overload) or by documenting that one of these two functions *must* be overloaded by new `AbstractRow` subtypes. I'm a Tables newcomer, so I'm unsure which approach is preferred (I'm assuming the latter?). I'm guessing a similar issue exists for AbstractColumns but haven't checked.

EDIT:

> documenting that one of these two functions *must* be overloaded by new `AbstractRow` subtypes

`getcolumn` etc is under "Required Methods" in the docs, but I interpret that in combo with "Default Definition"  to mean "this method needs to be defined *somewhere*; if the default definition seems good, then you're good to go"

Pull request:
fixes #221

Patch:
diff --git a/Project.toml b/Project.toml
index 591ee54..7d76372 100644
--- a/Project.toml
+++ b/Project.toml
@@ -4,25 +4,25 @@ authors = ["quinnj <quinn.jacobd@gmail.com>"]
 version = "1.2.2"
 
 [deps]
-LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
 DataAPI = "9a962f9c-6df0-11e9-0e5d-c546b8b5ee8a"
-TableTraits = "3783bdb8-4a98-5b6b-af9a-565f29a5fe9c"
-IteratorInterfaceExtensions = "82899510-4779-5014-852e-03e436cf321d"
-Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 DataValueInterfaces = "e2d170a0-9d28-54be-80f0-106bbe20a464"
-
-[extras]
+IteratorInterfaceExtensions = "82899510-4779-5014-852e-03e436cf321d"
+LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
+TableTraits = "3783bdb8-4a98-5b6b-af9a-565f29a5fe9c"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
-DataValues = "e7dc6d0d-1eca-5fa6-8ad6-5aecde8b7ea5"
-QueryOperators = "2aef5ad7-51ca-5a8f-8e88-e75cf067b44b"
-SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"
 
 [compat]
-julia = "1"
 DataAPI = "1"
 DataValueInterfaces = "1"
 IteratorInterfaceExtensions = "0.1.1, 1"
 TableTraits = "0.4.1, 1"
+julia = "1"
+
+[extras]
+DataValues = "e7dc6d0d-1eca-5fa6-8ad6-5aecde8b7ea5"
+QueryOperators = "2aef5ad7-51ca-5a8f-8e88-e75cf067b44b"
+SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf"
+Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 
 [targets]
 test = ["Test", "DataValues", "QueryOperators", "SparseArrays"]
diff --git a/src/Tables.jl b/src/Tables.jl
index da1f8e1..a31819e 100644
--- a/src/Tables.jl
+++ b/src/Tables.jl
@@ -31,6 +31,9 @@ Interface definition:
 | **Optional methods**                                     |                             |                                                                                                                                                              |
 | `Tables.getcolumn(table, ::Type{T}, i::Int, nm::Symbol)` | Tables.getcolumn(table, nm) | Given a column eltype `T`, index `i`, and column name `nm`, retrieve the column. Provides a type-stable or even constant-prop-able mechanism for efficiency. |
 
+Note that subtypes of `Tables.AbstractColumns` **must** overload all required methods listed
+above instead of relying on these methods' default definitions.
+
 While types aren't required to subtype `Tables.AbstractColumns`, benefits of doing so include:
   * Indexing interface defined (using `getcolumn`); i.e. `tbl[i]` will retrieve the column at index `i`
   * Property access interface defined (using `columnnames` and `getcolumn`); i.e. `tbl.col1` will retrieve column named `col1`
@@ -61,6 +64,9 @@ Interface definition:
 | **Optional methods**                                   |                           |                                                                                                                                                                  |
 | `Tables.getcolumn(row, ::Type{T}, i::Int, nm::Symbol)` | Tables.getcolumn(row, nm) | Given a column element type `T`, index `i`, and column name `nm`, retrieve the column value. Provides a type-stable or even constant-prop-able mechanism for efficiency. |
 
+Note that subtypes of `Tables.AbstractRow` **must** overload all required methods listed above
+instead of relying on these methods' default definitions.
+
 While custom row types aren't required to subtype `Tables.AbstractRow`, benefits of doing so include:
   * Indexing interface defined (using `getcolumn`); i.e. `row[i]` will return the column value at index `i`
   * Property access interface defined (using `columnnames` and `getcolumn`); i.e. `row.col1` will retrieve the value for the column named `col1`
@@ -155,6 +161,11 @@ columns(x::T) where {T <: AbstractDict} = error("to treat $T as a table, it must
 # default definitions for AbstractRow, AbstractColumns
 const RorC = Union{AbstractRow, AbstractColumns}
 
+# avoids mutual recursion with default definitions (issue #221)
+getcolumn(::T, ::Int) where {T <: RorC} = error("`Tables.getcolumn` must be specifically overloaded for $T <: Union{AbstractRow, AbstractColumns}`")
+getcolumn(::T, ::Symbol) where {T <: RorC} = error("`Tables.getcolumn` must be specifically overloaded for $T <: Union{AbstractRow, AbstractColumns}`")
+columnnames(::T) where {T <: RorC} = error("`Tables.columnnames` must be specifically overloaded for $T <: Union{AbstractRow, AbstractColumns}`")
+
 Base.IteratorSize(::Type{R}) where {R <: RorC} = Base.HasLength()
 Base.length(r::RorC) = length(columnnames(r))
 Base.IndexStyle(::Type{<:RorC}) = Base.IndexLinear()
@@ -246,7 +257,7 @@ columnnames(x::RorC2) = columnnames(getx(x))
 """
     Tables.istable(x) => Bool
 
-Check if an object has specifically defined that it is a table. Note that 
+Check if an object has specifically defined that it is a table. Note that
 not all valid tables will return true, since it's possible to satisfy the
 Tables.jl interface at "run-time", e.g. a `Generator` of `NamedTuple`s iterates
 `NamedTuple`s, which satisfies the `AbstractRow` interface, but there's no static way
@@ -329,7 +340,7 @@ materializer(::Type{T}) where {T} = columntable
 
 Accesses data of input table source `x` by returning an [`AbstractColumns`](@ref)-compatible
 object, which allows retrieving entire columns by name or index. A retrieved column
-is an object that is indexable and has a known length, i.e. supports 
+is an object that is indexable and has a known length, i.e. supports
 `length(col)` and `col[i]` for any `i = 1:length(col)`. Note that
 even if the input table source is row-oriented by nature, an efficient generic
 definition of `Tables.columns` is defined in Tables.jl to build a `AbstractColumns`-


Test patch:
diff --git a/test/runtests.jl b/test/runtests.jl
index 6af230b..a8b641b 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -171,7 +171,7 @@ end
     @test Tables.buildcolumns(nothing, rt) == nt
 end
 
-@testset "Materializer" begin 
+@testset "Materializer" begin
     rt = [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8"), (a=3, b=6.0, c="9")]
     nt = (a=[1,2,3], b=[4.0, 5.0, 6.0], c=["7", "8", "9"])
 
@@ -265,7 +265,7 @@ end
     @test M[:, 1] == [1, 2, 3]
     # 182
     @test M === m #checks that both are the same object in memory
-    @test Mt == permutedims(m) 
+    @test Mt == permutedims(m)
     # 167
     @test !Tables.istable(Matrix{Union{}}(undef, 2, 3))
 end
@@ -483,6 +483,8 @@ Tables.getcolumn(r::Row, nm::Symbol) = getfield(r, nm)
 Tables.getcolumn(r::Row, ::Type{T}, i::Int, nm::Symbol) where {T} = getfield(r, i)
 Tables.columnnames(r::Row) = fieldnames(Row)
 
+struct DummyRow <: Tables.AbstractRow end
+
 @testset "AbstractRow" begin
 
     row = Row(1, missing, "hey")
@@ -516,6 +518,10 @@ Tables.columnnames(r::Row) = fieldnames(Row)
     @test Tables.schema(art) === nothing
     @test isequal(Tables.columntable(art), ct)
 
+    r = DummyRow()
+    @test_throws ErrorException("`Tables.getcolumn` must be specifically overloaded for DummyRow <: Union{AbstractRow, AbstractColumns}`") Tables.getcolumn(r, 1)
+    @test_throws ErrorException("`Tables.getcolumn` must be specifically overloaded for DummyRow <: Union{AbstractRow, AbstractColumns}`") Tables.getcolumn(r, :a)
+    @test_throws ErrorException("`Tables.columnnames` must be specifically overloaded for DummyRow <: Union{AbstractRow, AbstractColumns}`") Tables.columnnames(r)
 end
 
 struct Columns <: Tables.AbstractColumns
@@ -529,6 +535,8 @@ Tables.getcolumn(r::Columns, nm::Symbol) = getfield(r, nm)
 Tables.getcolumn(r::Columns, ::Type{T}, i::Int, nm::Symbol) where {T} = getfield(r, i)
 Tables.columnnames(r::Columns) = fieldnames(Columns)
 
+struct DummyCols <: Tables.AbstractColumns end
+
 @testset "AbstractColumns" begin
 
     col = Columns([1, 2], [missing, 3.14], ["hey", "ho"])
@@ -557,6 +565,11 @@ Tables.columnnames(r::Columns) = fieldnames(Columns)
     @test Tables.columns(col) === col
     @test Tables.schema(col) === nothing
     @test isequal(Tables.columntable(col), ct)
+
+    c = DummyCols()
+    @test_throws ErrorException("`Tables.getcolumn` must be specifically overloaded for DummyCols <: Union{AbstractRow, AbstractColumns}`") Tables.getcolumn(c, 1)
+    @test_throws ErrorException("`Tables.getcolumn` must be specifically overloaded for DummyCols <: Union{AbstractRow, AbstractColumns}`") Tables.getcolumn(c, :a)
+    @test_throws ErrorException("`Tables.columnnames` must be specifically overloaded for DummyCols <: Union{AbstractRow, AbstractColumns}`") Tables.columnnames(c)
 end
 
 struct IsRowTable
@@ -612,7 +625,7 @@ end
     @test dct.a == [1, 2, 3]
     @test dct.b == [4.0, 5.0, 6.0]
     @test dct.c == ["7", "8", "9"]
-    
+
     dct = Tables.dictcolumntable(ct)
     @test dct.a == [1, 2, 3]
     @test dct.b == [4.0, 5.0, 6.0]