From cea261994bec50c30014b3ea51a9d0c48ace65d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Mon, 28 Nov 2022 14:37:38 +0100 Subject: [PATCH] fix select bug with copycols=false on SubDataFrame (#3231) --- NEWS.md | 8 ++++++++ Project.toml | 2 +- src/abstractdataframe/selection.jl | 17 ++--------------- src/other/index.jl | 6 +++++- test/index.jl | 6 +++--- test/select.jl | 11 +++++++++++ 6 files changed, 30 insertions(+), 20 deletions(-) diff --git a/NEWS.md b/NEWS.md index bda4d27d90..9aa98356e2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,11 @@ +# DataFrames.jl v1.4.4 Patch Release Notes + +## Bug fixes + +* Fix bug in `select` and `transform` with `copycols=false` on `SubDataFrame` + that incorrectly allowed passing transformations + ([#3231](/~https://github.com/JuliaData/DataFrames.jl/pull/3231)) + # DataFrames.jl v1.4.3 Patch Release Notes ## Bug fixes diff --git a/Project.toml b/Project.toml index 21e8f9cb69..09e004993a 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "DataFrames" uuid = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0" -version = "1.4.3" +version = "1.4.4" [deps] Compat = "34da2185-b29b-5c13-b0c7-acf172513d20" diff --git a/src/abstractdataframe/selection.jl b/src/abstractdataframe/selection.jl index 9e32989c68..8db4e16a90 100644 --- a/src/abstractdataframe/selection.jl +++ b/src/abstractdataframe/selection.jl @@ -336,16 +336,7 @@ make_pair_concrete(@nospecialize(x::Pair)) = make_pair_concrete(x.first) => make_pair_concrete(x.second) make_pair_concrete(@nospecialize(x)) = x -normalize_selection(idx::AbstractIndex, @nospecialize(sel), renamecols::Bool) = - try - idx[sel] - catch e - if e isa MethodError && e.f === getindex && e.args === (idx, sel) - throw(ArgumentError("Unrecognized column selector $sel in AsTable constructor")) - else - rethrow(e) - end - end +normalize_selection(idx::AbstractIndex, @nospecialize(sel), renamecols::Bool) = idx[sel] normalize_selection(idx::AbstractIndex, @nospecialize(sel::Base.Callable), renamecols::Bool) = sel normalize_selection(idx::AbstractIndex, sel::Colon, renamecols::Bool) = idx[:] @@ -1807,11 +1798,7 @@ function manipulate(dfv::SubDataFrame, @nospecialize(args...); copycols::Bool, k push!(seen_single_column, ind_idx) end else - newind = normalize_selection(index(dfv), make_pair_concrete(ind), renamecols) - if newind isa Pair - throw(ArgumentError("transforming and renaming columns of a " * - "SubDataFrame is not allowed when `copycols=false`")) - end + newind = index(dfv)[ind] push!(newinds, newind) end end diff --git a/src/other/index.jl b/src/other/index.jl index 2b1c882f14..8d694f0bb1 100644 --- a/src/other/index.jl +++ b/src/other/index.jl @@ -182,6 +182,9 @@ end Base.insert!(x::Index, idx::Integer, nm::AbstractString) = insert!(x, idx, Symbol(nm)) +@inline Base.getindex(x::AbstractIndex, idx::T) where {T} = + throw(ArgumentError("invalid index: $idx of type $T")) + @inline Base.getindex(x::AbstractIndex, idx::Bool) = throw(ArgumentError("invalid index: $idx of type Bool")) @@ -240,7 +243,8 @@ end return getindex(x, Vector{Int}(idx)) end -@inline Base.getindex(x::AbstractIndex, idx::AbstractRange{Bool}) = getindex(x, collect(idx)) +@inline Base.getindex(x::AbstractIndex, idx::AbstractRange{Bool}) = + getindex(x, collect(idx)) @inline function Base.getindex(x::AbstractIndex, idx::AbstractVector{Bool}) length(x) == length(idx) || throw(BoundsError(x, idx)) diff --git a/test/index.jl b/test/index.jl index 29993976c8..4d5e31e138 100644 --- a/test/index.jl +++ b/test/index.jl @@ -24,7 +24,7 @@ using DataFrames: Index, SubIndex, fuzzymatch end end - @test_throws MethodError i[1.0] + @test_throws ArgumentError i[1.0] @test_throws ArgumentError i[true] @test_throws ArgumentError i[false] @test_throws ArgumentError i[Union{Bool, Missing}[true, false]] @@ -477,8 +477,8 @@ end @test df[:, Cols(x -> x[1] == 'a')] == df[:, [1, 2]] @test df[:, Cols(x -> x[end] == '1')] == df[:, [1, 3]] @test df[:, Cols(x -> x[end] == '3')] == DataFrame() - @test_throws MethodError df[:, Cols(x -> true, 1)] == DataFrame() - @test_throws MethodError df[:, Cols(1, x -> true)] == DataFrame() + @test_throws ArgumentError df[:, Cols(x -> true, 1)] + @test_throws ArgumentError df[:, Cols(1, x -> true)] end @testset "views" begin diff --git a/test/select.jl b/test/select.jl index c91df2ddc4..380db401a3 100644 --- a/test/select.jl +++ b/test/select.jl @@ -315,6 +315,8 @@ end @test_throws BoundsError select(df, 6) @test_throws ArgumentError select(df, [1, 1]) @test_throws ArgumentError select(df, :f) + @test_throws ArgumentError select(df, 1.0) + @test_throws ArgumentError select(df, true) @test_throws BoundsError select!(df, [true, false]) @test select(df, 1:0) == DataFrame() @@ -876,6 +878,7 @@ end @test select(sdf, :x1, [:x1], copycols=false) isa SubDataFrame @test_throws ArgumentError select(sdf, :x1 => :r1, copycols=false) @test_throws ArgumentError select(sdf, :x1 => identity => :r1, copycols=false) + @test_throws ArgumentError select(sdf, identity, copycols=false) end @testset "pseudo-broadcasting" begin @@ -2733,4 +2736,12 @@ end e=[missing, 4, missing, 8]) end +@testset "selection on a view without copying" begin + df = DataFrame(a=1:2) + for dfv in (view(df, :, :), view(df, 1:2, 1:1)) + @test_throws ArgumentError select(dfv, x -> true, copycols=false) + @test_throws ArgumentError select(dfv, :a => identity, copycols=false) + end +end + end # module