Reject transformations on SubDataFrame with copycols=false by arkgupta17 · Pull Request #3522 · JuliaData/DataFrames.jl

arkgupta17 · 2026-02-03T20:19:02Z

This PR rejects transformations on SubDataFrame when copycols=false and adds a regression test ensuring a clear ArgumentError mentioning copycols=false

bkamins · 2026-02-04T15:43:35Z

Fixes #3515

bkamins

Thank you for the PR. It catches a bug that we have. I have left some suggestions.

bkamins · 2026-02-04T15:42:44Z

src/abstractdataframe/selection.jl

    return newdf
 end

-function manipulate(dfv::SubDataFrame, @nospecialize(args...); copycols::Bool, keeprows::Bool,


could you please remove changes that are only for layout reasons? This will simplify analysis of commits to main in the future. Thank you!.

src/abstractdataframe/selection.jl

bkamins · 2026-02-04T16:03:44Z

src/abstractdataframe/selection.jl

+                push!(newinds, ind_idx)
            else
                newind = index(dfv)[ind]
                push!(newinds, newind)
            end
        end


Suggested change

push!(newinds, ind_idx)

else

newind = index(dfv)[ind]

push!(newinds, newind)

end

end

end

newind = index(dfv)[ind]

push!(newinds, newind)

end

This is a good catch. The following change is needed in the code. (just please check the alignment of lines as I edit in a browser).

Also can you please add the following test:

julia> df = DataFrame(a = [1, 2, 3], b = [1,2,3]) 3×2 DataFrame Row │ a b │ Int64 Int64 ─────┼────────────── 1 │ 1 1 2 │ 2 2 3 │ 3 3 julia> v = @view df[1:2, :] 2×2 SubDataFrame Row │ a b │ Int64 Int64 ─────┼────────────── 1 │ 1 1 2 │ 2 2 julia> select(v, "a", "b", copycols=false) newinds = Any[] 0×0 SubDataFrame

this is on main and it is incorrect as you see as the result should be:

2×2 DataFrame Row │ a b │ Int64 Int64 ─────┼────────────── 1 │ 1 1 2 │ 2 2

arkgupta17 · 2026-02-06T07:40:18Z

Thanks for pointing that out — you’re absolutely right.

I’ve removed all layout-only changes and reverted the file to upstream formatting.
The commit now contains only the minimal functional fix for copycols=false column selection on SubDataFrame, plus the regression test you requested.

Please let me know if this looks good now.

test/utils.jl

bkamins · 2026-02-06T10:03:46Z

Thank you! I added one small suggestion. Otherwise it looks good. Do you know why nightly fails?

Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>

arkgupta17 · 2026-02-06T12:08:45Z

Thank you for the review and the suggestion — I’ve applied it.

I’m not sure yet why nightly fails. I’ll investigate the nightly logs locally and report back once I understand the cause.

bkamins · 2026-02-06T17:28:51Z

OK - I understand that === fails, so maybe let us check == and check if parent of columns is ===.

bkamins

Looks good. Thank you! The nightly fail is unrelated.

src/abstractdataframe/selection.jl

test/utils.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2026-02-16T16:18:32Z

src/abstractdataframe/selection.jl

+        newinds = Any[]
        seen_single_column = Set{Int}()
        for ind in args
+           if ind isa Pair || ind isa AbstractVecOrMat{<:Pair}


still some indentation issues here

This is still the place where the issue is. The code is indended by 3 spaces and should be by 4

I have fixed the issue.

Thank you!

bkamins · 2026-02-21T20:19:46Z

Hi,

I think you accenditenally run some autoformatter. I requested to fix just one place where indentation was incorrect. Could you please revert last commit and just fix the indentation in the PR? Thank you!

bkamins · 2026-02-21T21:06:37Z

The changes are not reverted

arkgupta17 · 2026-02-21T21:23:43Z

I have reset the branch to commit 1964644 and ensured only the intended indentation structure is present.

bkamins · 2026-02-22T14:25:58Z

src/abstractdataframe/selection.jl

        seen_single_column = Set{Int}()
        for ind in args
+            if ind isa Pair || ind isa AbstractVecOrMat{<:Pair}
+              throw(ArgumentError(


the PR still has wrong indentations. Sometimes 3 spaces, sometimes 2 spaces. Please make sure that in the PR all indentations are at 3 spaces. Thank you!

You mean 4 spaces, right? ;-)

I think everything is fine now!

4 spaces - right

src/abstractdataframe/selection.jl

bkamins · 2026-02-22T20:13:06Z

Thank you!

bkamins linked an issue Feb 4, 2026 that may be closed by this pull request

invalid index with SubDataFrame and copycols = false #3515

Closed

bkamins requested changes Feb 4, 2026

View reviewed changes

bkamins added the bug label Feb 4, 2026

bkamins added this to the patch milestone Feb 4, 2026

Reject transformations on SubDataFrame with copycols=false

3ab53c7

arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from e4f6745 to 3ab53c7 Compare February 6, 2026 07:38

bkamins reviewed Feb 6, 2026

View reviewed changes

test/utils.jl Outdated Show resolved Hide resolved

Update test/utils.jl

20e205c

Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>

arkgupta17 requested a review from bkamins February 6, 2026 12:11

Reject transformations on SubDataFrame with copycols=false

92e2e7c

bkamins approved these changes Feb 12, 2026

View reviewed changes

bkamins requested a review from nalimilan February 12, 2026 08:57

nalimilan reviewed Feb 15, 2026

View reviewed changes

src/abstractdataframe/selection.jl Outdated Show resolved Hide resolved

src/abstractdataframe/selection.jl Outdated Show resolved Hide resolved

test/utils.jl Outdated Show resolved Hide resolved

test/utils.jl Outdated Show resolved Hide resolved

arkgupta17 and others added 4 commits February 15, 2026 22:28

Update test/utils.jl

7c6aea2

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update test/utils.jl

7cf5052

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/abstractdataframe/selection.jl

5d91c54

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Reject transformations on SubDataFrame with copycols=false

1964644

arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from ae7dacd to 1964644 Compare February 15, 2026 17:44

arkgupta17 requested review from bkamins and nalimilan February 16, 2026 15:56

bkamins reviewed Feb 16, 2026

View reviewed changes

arkgupta17 requested a review from bkamins February 21, 2026 15:10

arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from 346bc91 to a04f14b Compare February 21, 2026 20:41

arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from a04f14b to 1964644 Compare February 21, 2026 21:16

Update indentation

ea764ad

bkamins reviewed Feb 22, 2026

View reviewed changes

update indentation

ad9e3ae

bkamins reviewed Feb 22, 2026

View reviewed changes

src/abstractdataframe/selection.jl Show resolved Hide resolved

bkamins reviewed Feb 22, 2026

View reviewed changes

src/abstractdataframe/selection.jl Show resolved Hide resolved

update indentation

b4e9b25

bkamins approved these changes Feb 22, 2026

View reviewed changes

bkamins merged commit b12f7fb into JuliaData:main Feb 22, 2026
7 of 8 checks passed

arkgupta17 deleted the improve-subdataframe-copycols-error branch February 22, 2026 20:29

Conversation

arkgupta17 commented Feb 3, 2026

Uh oh!

bkamins commented Feb 4, 2026

Uh oh!

bkamins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkgupta17 commented Feb 6, 2026

Uh oh!

Uh oh!

bkamins commented Feb 6, 2026

Uh oh!

arkgupta17 commented Feb 6, 2026

Uh oh!

bkamins commented Feb 6, 2026

Uh oh!

bkamins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkamins commented Feb 21, 2026

Uh oh!

bkamins commented Feb 21, 2026

Uh oh!

arkgupta17 commented Feb 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkamins commented Feb 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants