From 5c27b10d41a57c6de69198b1c1ccab566c2a32c0 Mon Sep 17 00:00:00 2001
From: Glenn Moynihan <glenn.moynihan@invenialabs.co.uk>
Date: Tue, 20 Apr 2021 13:56:14 +0100
Subject: [PATCH] Support RowTable

Note that we cannot call `apply!` on rowtables since the elements
of the NamedTuple rows are immutable in this instance.

e.g. row = NamedTuple{(:a, :b)}(a=1, b=2)

convert to-from a columntable defeats the purpose as it wouldn't be
mutating the underlying table.
---
 Project.toml         |  2 +-
 src/apply.jl         | 25 +++++++++++++++++++++----
 test/runtests.jl     |  2 +-
 test/types/tables.jl | 30 +++++++++++++++++-------------
 4 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/Project.toml b/Project.toml
index 554f125..9354bff 100644
--- a/Project.toml
+++ b/Project.toml
@@ -1,7 +1,7 @@
 name = "FeatureTransforms"
 uuid = "8fd68953-04b8-4117-ac19-158bf6de9782"
 authors = ["Invenia Technical Computing Corporation"]
-version = "0.3.7"
+version = "0.3.8"
 
 [deps]
 Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
diff --git a/src/apply.jl b/src/apply.jl
index 8837c27..818190e 100644
--- a/src/apply.jl
+++ b/src/apply.jl
@@ -89,25 +89,35 @@ function apply(table, t::Transform; cols=_get_cols(table), header=nothing, kwarg
     return Tables.materializer(table)(_to_table(result, header))
 end
 
+# Prevents Vector{NamedTuple} from using the AbstractArray method above
+function apply(table::Tables.RowTable, t::Transform; kwargs...)
+    return rowtable(apply(Tables.columntable(table), t; kwargs...))
+end
+
 """
     apply!(table::T, ::Transform; [cols])::T where T
 
 Applies the [`Transform`](@ref) to each of the specified columns in the `table`.
 If no `cols` are specified, then the [`Transform`](@ref) is applied to all columns.
+
+!!! Note
+  `apply!` does not support `RowTable`s since the `NamedTuple` rows are immutable.
 """
 function apply!(table::T, t::Transform; cols=_get_cols(table), kwargs...)::T where T
     Tables.istable(table) || throw(MethodError(apply!, (table, t)))
 
-    # Extract a columns iterator that we should be able to use to mutate the data.
-    # NOTE: Mutation is not guaranteed for all table types, but it avoid copying the data
-    coltable = Tables.columntable(table)
     for cname in _to_vec(cols)
-        apply!(getproperty(coltable, cname), t; kwargs...)
+        apply!(getproperty(table, cname), t; kwargs...)
     end
 
     return table
 end
 
+# Prevents Vector{NamedTuple} from using the AbstractArray method above
+function apply!(table::Tables.RowTable, t::Transform; kwargs...)
+    return throw(MethodError(apply!, (table, t)))
+end
+
 """
     apply_append(A::AbstractArray, ::Transform; append_dim, kwargs...)
 
@@ -135,6 +145,13 @@ function apply_append(table, t; kwargs...)
     return T(merge(Tables.columntable(table), result))
 end
 
+# Prevents Vector{NamedTuple} from using the AbstractArray method above
+function apply_append(table::Tables.RowTable, t; kwargs...)
+    T = Tables.materializer(table)
+    result = apply(table, t; kwargs...)
+    return T(merge.(Tables.rowtable(table), result))
+end
+
 # These methods format data according to the cardinality of the Transform.
 # Most Transforms don't require any formatting, only those that are ManyToOne do.
 # Note: we don't yet have a ManyToMany transform, so those might need separate treatment.
diff --git a/test/runtests.jl b/test/runtests.jl
index c79833b..7fedca2 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -8,7 +8,7 @@ using FeatureTransforms
 using FeatureTransforms: _periodic
 using FeatureTransforms: cardinality, OneToOne, OneToMany, ManyToOne, ManyToMany
 using FeatureTransforms.TestUtils
-using Tables: columntable, istable, rowtable
+using Tables: columntable, isrowtable, istable, rowtable
 using Test
 using TimeZones
 
diff --git a/test/types/tables.jl b/test/types/tables.jl
index 38e5430..e9961a6 100644
--- a/test/types/tables.jl
+++ b/test/types/tables.jl
@@ -1,5 +1,4 @@
-# TODO: test on rowtable https://github.com/invenia/FeatureTransforms.jl/issues/64
-@testset "$TableType" for TableType in (columntable, DataFrame)
+@testset "$TableType" for TableType in (columntable, rowtable, DataFrame)
 
     table = TableType((a=[1, 2, 3], b=[4, 5, 6]))
 
@@ -91,17 +90,22 @@
     @testset "apply!" begin
         T = FakeOneToOneTransform()
 
-        _table = deepcopy(table)
-        FeatureTransforms.apply!(_table, T)
-        @test _table == TableType((a=ones(3), b=ones(3)))
-
-        _table = deepcopy(table)
-        FeatureTransforms.apply!(_table, T; cols=:a)
-        @test _table == TableType((a=[1, 1, 1], b=[4, 5, 6]))
-
-        _table = deepcopy(table)
-        @test_broken FeatureTransforms.apply!(_table, T; cols=:b, dims=[1, 2])
-        @test_broken _table == TableType((a=[1, 2, 3], b=[1, 1, 6]))
+        # Cannot mutate NamedTuple elements
+        if isrowtable(table)
+            @test_throws MethodError FeatureTransforms.apply!(table, T)
+        else
+            _table = deepcopy(table)
+            FeatureTransforms.apply!(_table, T)
+            @test _table == TableType((a=ones(3), b=ones(3)))
+
+            _table = deepcopy(table)
+            FeatureTransforms.apply!(_table, T; cols=:a)
+            @test _table == TableType((a=[1, 1, 1], b=[4, 5, 6]))
+
+            _table = deepcopy(table)
+            @test_broken FeatureTransforms.apply!(_table, T; cols=:b, dims=[1, 2])
+            @test_broken _table == TableType((a=[1, 2, 3], b=[1, 1, 6]))
+        end
     end
 
     @testset "apply_append" begin