Repository: JuliaData/Tables.jl
Pull request #304 (2022-09-21 23:01:00)

Change view to viewhint in Tables.subset

codecov[bot]: # [Codecov](https://codecov.io/gh/JuliaData/Tables.jl/pull/304?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData) Report
Base: **95.07**% // Head: **93.86**% // Decreases project coverage by **`-1.21%`** :warning:
> Coverage data is based on head [(`0e73120`)](https://codecov.io/gh/JuliaData/Tables.jl/pull/304?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData) compared to base [(`63067e3`)](https://codecov.io/gh/JuliaData/Tables.jl/commit/63067e3d6ec445ea8021b4460dd0820679acd16c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData).
> Patch coverage: 62.50% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #304      +/-   ##
==========================================
- Coverage   95.07%   93.86%   -1.22%     
==========================================
  Files           7        7              
  Lines         711      717       +6     
==========================================
- Hits          676      673       -3     
- Misses         35       44       +9     
```


| [Impacted Files](https://codecov.io/gh/JuliaData/Tables.jl/pull/304?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData) | Coverage Δ | |
|---|---|---|
| [src/dicts.jl](https://codecov.io/gh/JuliaData/Tables.jl/pull/304/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData#diff-c3JjL2RpY3RzLmps) | `93.96% <60.00%> (-1.61%)` | :arrow_down: |
| [src/namedtuples.jl](https://codecov.io/gh/JuliaData/Tables.jl/pull/304/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData#diff-c3JjL25hbWVkdHVwbGVzLmps) | `97.50% <60.00%> (-2.50%)` | :arrow_down: |
| [src/Tables.jl](https://codecov.io/gh/JuliaData/Tables.jl/pull/304/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData#diff-c3JjL1RhYmxlcy5qbA==) | `86.78% <66.66%> (-1.53%)` | :arrow_down: |
| [src/tofromdatavalues.jl](https://codecov.io/gh/JuliaData/Tables.jl/pull/304/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData#diff-c3JjL3RvZnJvbWRhdGF2YWx1ZXMuamw=) | `98.46% <0.00%> (-1.54%)` | :arrow_down: |
| [src/fallbacks.jl](https://codecov.io/gh/JuliaData/Tables.jl/pull/304/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData#diff-c3JjL2ZhbGxiYWNrcy5qbA==) | `97.93% <0.00%> (-0.70%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/JuliaData/Tables.jl/pull/304?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaData).

bkamins: @jariji and @ablaom - can you have a quick look at the PR? Thank you!
jariji: LGTM
bkamins: @quinnj - do we make this change and make a release (@nalimilan and I need to know it before releasing DataFrames.jl 1.4 😄)

Patch:
diff --git a/src/Tables.jl b/src/Tables.jl
index 1bfe17e..a5596c4 100644
--- a/src/Tables.jl
+++ b/src/Tables.jl
@@ -574,7 +574,7 @@ struct Partitioner{T}
 end
 
 """
-    Tables.subset(x, inds; view=nothing)
+    Tables.subset(x, inds; viewhint=nothing)
 
 Return one or more rows from table `x` according to the position(s) specified by `inds`:
 
@@ -584,19 +584,23 @@ Return one or more rows from table `x` according to the position(s) specified by
 
 If other types of `inds` are passed than specified above the behavior is undefined.
 
-The `view` argument influences whether the returned object is a view of the original table
+The `viewhint` argument tries to influence whether the returned object is a view of the original table
 or an independent copy:
 
-- If `view=nothing` (the default) then the implementation for a specific table type
+- If `viewhint=nothing` (the default) then the implementation for a specific table type
   is free to decide  whether to return a copy or a view.
-- If `view=true` then a view is returned and if `view=false` a copy is returned.
+- If `viewhint=true` then a view is returned and if `viewhint=false` a copy is returned.
   This applies both to returning a row or a table.
 
-Any specialized implementation of `subset` must support the `view=nothing` argument.
-Support for `view=true` or `view=false` is optional
-(i.e. implementations may ignore the keyword argument and return a view or a copy regardless of `view` value).
+Any specialized implementation of `subset` must support the `viewhint=nothing` argument.
+Support for `viewhint=true` or `viewhint=false` is optional
+(i.e. implementations may ignore the keyword argument and return a view or a copy regardless of `viewhint` value).
 """
-function subset(x::T, inds; view::Union{Bool, Nothing}=nothing) where {T}
+function subset(x::T, inds; viewhint::Union{Bool, Nothing}=nothing, view::Union{Bool, Nothing}=nothing) where {T}
+    if view !== nothing
+        @warn "`view` keyword argument is deprecated for `Tables.subset`, use `viewhint` instead"
+        viewhint = view
+    end
     # because this method is being called, we know `x` didn't define it's own Tables.subset
     # first check if it supports column access, and if so, apply inds and wrap columns in a DictColumnTable
     if columnaccess(x)
@@ -604,7 +608,7 @@ function subset(x::T, inds; view::Union{Bool, Nothing}=nothing) where {T}
         if inds isa Integer
             return ColumnsRow(cols, inds)
         else
-            ret = view === true ? _map(c -> Base.view(c, inds), cols) : _map(c -> c[inds], cols)
+            ret = viewhint === true ? _map(c -> Base.view(c, inds), cols) : _map(c -> c[inds], cols)
             return DictColumnTable(schema(cols), ret)
         end
     end
@@ -612,7 +616,7 @@ function subset(x::T, inds; view::Union{Bool, Nothing}=nothing) where {T}
     r = rows(x)
     if r isa AbstractVector
         inds isa Integer && return r[inds]
-        ret = view === true ? Base.view(x, inds) : x[inds]
+        ret = viewhint === true ? Base.view(x, inds) : x[inds]
         (ret isa AbstractVector) || throw(ArgumentError("`Tables.subset`: invalid `inds` argument, expected `AbstractVector` output, got $(typeof(ret))"))
         return ret
     end
diff --git a/src/dicts.jl b/src/dicts.jl
index ee5e6db..5b38514 100644
--- a/src/dicts.jl
+++ b/src/dicts.jl
@@ -124,8 +124,12 @@ function Base.iterate(x::DictRowTable, st=1)
     return DictRow(x.names, x.values[st]), st + 1
 end
 
-function subset(x::DictRowTable, inds; view::Union{Bool,Nothing} = nothing)
-    values = view === true ? Base.view(getfield(x, :values), inds) : getfield(x, :values)[inds]
+function subset(x::DictRowTable, inds; viewhint::Union{Bool,Nothing}=nothing, view::Union{Bool,Nothing}=nothing)
+    if view !== nothing
+        @warn "`view` keyword argument is deprecated for `Tables.subset`, use `viewhint` instead"
+        viewhint = view
+    end
+    values = viewhint === true ? Base.view(getfield(x, :values), inds) : getfield(x, :values)[inds]
     if inds isa Integer
         return DictRow(getfield(x, :names), values)
     else
diff --git a/src/namedtuples.jl b/src/namedtuples.jl
index 2245ccc..49e3ea7 100644
--- a/src/namedtuples.jl
+++ b/src/namedtuples.jl
@@ -110,11 +110,15 @@ end
 const ColumnTable = NamedTuple{names, T} where {names, T <: NTuple{N, AbstractVector}} where {N}
 rowcount(c::ColumnTable) = length(c) == 0 ? 0 : length(c[1])
 
-function subset(x::ColumnTable, inds; view::Union{Bool,Nothing}=nothing)
+function subset(x::ColumnTable, inds; viewhint::Union{Bool,Nothing}=nothing, view::Union{Bool,Nothing}=nothing)
+    if view !== nothing
+        @warn "`view` keyword argument is deprecated for `Tables.subset`, use `viewhint` instead"
+        viewhint = view
+    end
     if inds isa Integer
         return map(c -> c[inds], x)
     else
-        return view === true ? map(c -> vectorcheck(Base.view(c, inds)), x) : map(c -> vectorcheck(c[inds]), x)
+        return viewhint === true ? map(c -> vectorcheck(Base.view(c, inds)), x) : map(c -> vectorcheck(c[inds]), x)
     end
 end
 


Test patch:
diff --git a/test/runtests.jl b/test/runtests.jl
index 317af1e..25206bb 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -147,30 +147,30 @@ end
 
     @testset "columntable subset" begin
         @test Tables.subset(nt, 1) == (a=1, b=4.0, c="7")
-        @test Tables.subset(nt, 1, view=false) == (a=1, b=4.0, c="7")
-        @test Tables.subset(nt, 1, view=nothing) == (a=1, b=4.0, c="7")
+        @test Tables.subset(nt, 1, viewhint=false) == (a=1, b=4.0, c="7")
+        @test Tables.subset(nt, 1, viewhint=nothing) == (a=1, b=4.0, c="7")
         @test Tables.subset(nt, 1:2) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
-        @test Tables.subset(nt, 1:2, view=false) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
-        @test Tables.subset(nt, 1:2, view=nothing) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
+        @test Tables.subset(nt, 1:2, viewhint=false) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
+        @test Tables.subset(nt, 1:2, viewhint=nothing) == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
         @test_throws ArgumentError Tables.subset(nt, [1:2 1:2])
 
-        @test Tables.subset(nt, 1, view=true) == (a=1, b=4.0, c="7")
-        rs = Tables.subset(nt, 1:2, view=true)
+        @test Tables.subset(nt, 1, viewhint=true) == (a=1, b=4.0, c="7")
+        rs = Tables.subset(nt, 1:2, viewhint=true)
         @test rs == (a=[1,2], b=[4.0, 5.0], c=["7","8"])
         @test rs.a.parent === nt.a
     end
 
     @testset "rowtable subset" begin
         @test Tables.subset(rt, 1) == (a=1, b=4.0, c="7")
-        @test Tables.subset(rt, 1, view=false) == (a=1, b=4.0, c="7")
-        @test Tables.subset(rt, 1, view=nothing) == (a=1, b=4.0, c="7")
+        @test Tables.subset(rt, 1, viewhint=false) == (a=1, b=4.0, c="7")
+        @test Tables.subset(rt, 1, viewhint=nothing) == (a=1, b=4.0, c="7")
         @test Tables.subset(rt, 1:2) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
-        @test Tables.subset(rt, 1:2, view=false) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
-        @test Tables.subset(rt, 1:2, view=nothing) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
+        @test Tables.subset(rt, 1:2, viewhint=false) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
+        @test Tables.subset(rt, 1:2, viewhint=nothing) == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
         @test_throws ArgumentError Tables.subset(rt, [1:2 1:2])
 
-        @test Tables.subset(rt, 1, view=true) == (a=1, b=4.0, c="7")
-        rs = Tables.subset(rt, 1:2, view=true)
+        @test Tables.subset(rt, 1, viewhint=true) == (a=1, b=4.0, c="7")
+        rs = Tables.subset(rt, 1:2, viewhint=true)
         @test rs == [(a=1, b=4.0, c="7"), (a=2, b=5.0, c="8")]
         @test rs.parent === rt
     end
@@ -794,7 +794,7 @@ end
     @test drow.a == 1 && drow.b == 2 && drow.c == 3
     drows = Tables.subset(drt, [1, 2])
     @test length(drows) == 2
-    drowsv = Tables.subset(drt, [1, 2]; view=true)
+    drowsv = Tables.subset(drt, [1, 2]; viewhint=true)
     @test length(drowsv) == 2
 end