Updates from code review

Author: rofinn

src/tables.jl

@@ -117,54 +117,75 @@ end
 # end
 
 """
-    populate!(A, table, value)
+    AxisKeys.populate!(A, table, value; force=false, quiet=false)
 
-Populate A with the contents of the `value` column in a provided table.
-The provided table contain columns corresponding to the keys in A and support row iteration.
+Populate `A` with the contents of the `value` column in a provided `table`.
+The `table` must contain columns corresponding to the keys in `A` and support row iteration.
+If the keys in `A` do not uniquely identify rows in the `table` then an
+`ArgumentError` is throw. If `force` is true then the duplicate (non-unique) entries will be
+overwritten. Setting `quiet` to true will avoid any non-unique warnings.
 """
-function populate!(A, table, value::Symbol)
-    cols = [value, dimnames(A)...]
+function populate!(A, table, value::Symbol; force=false, quiet=false)
+    # Use a BitArray mask to detect duplicates and error instead of overwriting.
+    mask = falses(size(A))
+    overwritten = false
+
     for r in Tables.rows(table)
-        setkey!(A, (Tables.getcolumn(r, c) for c in cols)...)
+        vals = Tuple(Tables.getcolumn(r, c) for c in dimnames(A))
+        inds = map(findindex, vals, axiskeys(A))
+
+        # Handle duplicate error if applicable
+        if mask[inds...]
+            if force
+                overwritten = true
+            else
+                throw(ArgumentError("Key $vals is not unique"))
+            end
+        end
+
+        # Set or mask marking that we've set this index
+        setindex!(mask, true, inds...)
+
+        # Insert our value into the data array
+        setindex!(A, Tables.getcolumn(r, value), inds...)
+    end
+
+    if overwritten && !quiet
+        @warn "Columns $(dimnames(A)) do not uniquely identify rows in the table"
     end
+
     return A
 end
 
 """
-    KeyedArray(table, value, keys...; default=undef)
-    NamedDimsArray(table, value, keys...; default=undef)
+    wrapdims(table, [Type,] value, keys...; default=undef, sort=false, force=false, quiet=false)
 
 Construct a KeyedArray/NamedDimsArray from a `table` supporting column and row.
-`keys` columns are extracted as is, without sorting, and are assumed to uniquely identify
-each `value`. The `default` value is used in cases where no value is identified for a given
-keypair.
+The `default` value is used in cases where no value is identified for a given keypair.
+If the `keys` columns do not uniquely identify rows in the table then an `ArgumentError` is
+throw. If `force` is true then the duplicate (non-unique) entries will be
+overwritten. Setting `quiet` to true will avoid any non-unique warnings.
 """
-function KeyedArray(table, value::Symbol, keys::Symbol...; kwargs...)
-   return _construct_from_table(KeyedArray, table, value, keys...; kwargs...)
+function wrapdims(table, value::Symbol, keys::Symbol...; kwargs...)
+    wrapdims(table, KeyedArray, value, keys...; kwargs...)
 end
 
-function NamedDimsArray(table, value::Symbol, keys::Symbol...; kwargs...)
-   return _construct_from_table(NamedDimsArray, table, value, keys...; kwargs...)
-end
-
-
-# Internal function for constructing the KeyedArray or NamedDimsArray.
-# This code doesn't care which type we produce so we just pass that along.
-function _construct_from_table(
-    T::Type, table, value::Symbol, keys::Symbol...;
-    default=undef, issorted=false
-)
+function wrapdims(table, T::Type, value::Symbol, keys::Symbol...; default=undef, sort::Bool=false, kwargs...)
     # get columns of the input table source
     cols = Tables.columns(table)
 
     # Extract key columns
-    kw = Tuple(k => unique(Tables.getcolumn(cols, k)) for k in keys)
+    pairs = map(keys) do k
+        col = unique(Tables.getcolumn(cols, k))
+        sort && Base.sort!(col)
+        return k => col
+    end
 
     # Extract data/value column
     vals = Tables.getcolumn(cols, value)
 
     # Initialize the KeyedArray
-    sz = length.(last.(kw))
+    sz = length.(last.(pairs))
 
     A = if default === undef
         data = similar(vals, sz)
@@ -173,7 +194,7 @@ function _construct_from_table(
         fill!(data, default)
     end
 
-    A = T(data; kw...)
-    populate!(A, table, value)
+    A = T(data; pairs...)
+    populate!(A, table, value; kwargs...)
     return A
 end

test/_packages.jl

@@ -44,7 +44,7 @@ end
 
         # Test fully constructing from a table
         # Common when working with adhoc data
-        B = KeyedArray(table, :value, :time, :loc, :id)
+        B = wrapdims(table, :value, :time, :loc, :id)
         @test B == A
 
         # Test populating an existing array (e.g., expected data based on calculated targets/offsets)
@@ -59,8 +59,30 @@ end
         @test C == A
 
         # Constructing a NamedDimsArray with different default value and table type
-        D = NamedDimsArray(Tables.rowtable(A), :value, :time, :loc, :id; default=missing)
-        @test D == A
+        # Partial populating
+        table = Tables.rowtable(A)
+        n = length(table)
+        idx = rand(Bool, n)
+        D = wrapdims(table[idx], :value, :time, :loc, :id; default=missing)
+        # dimnames should still match, but we'll have missing values
+        @test dimnames(D) == dimnames(A)
+        @test any(ismissing, D)
+
+        # Construction with invalid columns error as expected, but the specific error is
+        # dependent on the table type.
+        # ERROR: ArgumentError: wrong number of names, got (:q, :time, :loc, :id) with ndims(A) == 1
+        @test_throws ArgumentError wrapdims(Tables.rowtable(A), :q, :time, :loc, :id)
+        # ERROR: ArgumentError: wrong number of names, got (:value, :p, :loc, :id) with ndims(A) == 1
+        @test_throws ArgumentError wrapdims(Tables.rowtable(A), :value, :p, :loc, :id)
+        # ERROR: type NamedTuple has no field q
+        @test_throws ErrorException wrapdims(Tables.columntable(A), :q, :time, :loc, :id)
+        # ERROR: type NamedTuple has no field p
+        @test_throws ErrorException wrapdims(Tables.columntable(A), :value, :p, :loc, :id)
+
+        # Construction with duplicates
+        # ERROR: ArgumentError: Key (Date("2019-01-01"), -5) is not unique
+        @test_throws ArgumentError wrapdims(table, :value, :time, :loc)
+        @test wrapdims(table, :value, :time, :loc; force=true) == C(:, :, Key("c"))
     end
 end
 @testset "stack" begin