Skip to content

Reject transformations on SubDataFrame with copycols=false#3522

Merged
bkamins merged 10 commits intoJuliaData:mainfrom
arkgupta17:improve-subdataframe-copycols-error
Feb 22, 2026
Merged

Reject transformations on SubDataFrame with copycols=false#3522
bkamins merged 10 commits intoJuliaData:mainfrom
arkgupta17:improve-subdataframe-copycols-error

Conversation

@arkgupta17
Copy link
Copy Markdown
Contributor

@arkgupta17 arkgupta17 commented Feb 3, 2026

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

@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 4, 2026

Fixes #3515

@bkamins 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
bkamins requested changes Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bkamins bkamins added the bug label Feb 4, 2026
@bkamins bkamins added this to the patch milestone Feb 4, 2026
@arkgupta17 arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from e4f6745 to 3ab53c7 Compare February 6, 2026 07:38
@arkgupta17
Copy link
Copy Markdown
Contributor Author

arkgupta17 commented Feb 6, 2026

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.

bkamins
bkamins reviewed Feb 6, 2026
View reviewed changes
@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 6, 2026

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

@arkgupta17 @bkamins
Update test/utils.jl
20e205c 
Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>
@arkgupta17
Copy link
Copy Markdown
Contributor Author

arkgupta17 commented Feb 6, 2026

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.

@arkgupta17 arkgupta17 requested a review from bkamins February 6, 2026 12:11
@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 6, 2026

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

bkamins
bkamins approved these changes Feb 12, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bkamins bkamins requested a review from nalimilan February 12, 2026 08:57
nalimilan
nalimilan reviewed Feb 15, 2026
View reviewed changes
arkgupta17 and others added 4 commits February 15, 2026 22:28
@arkgupta17 @nalimilan
Update test/utils.jl
7c6aea2 
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@arkgupta17 @nalimilan
Update test/utils.jl
7cf5052 
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@arkgupta17 @nalimilan
Update src/abstractdataframe/selection.jl
5d91c54 
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@arkgupta17
Reject transformations on SubDataFrame with copycols=false
1964644
@arkgupta17 arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from ae7dacd to 1964644 Compare February 15, 2026 17:44
bkamins
bkamins reviewed Feb 16, 2026
View reviewed changes
@arkgupta17 arkgupta17 requested a review from bkamins February 21, 2026 15:10
@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 21, 2026

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!

@arkgupta17 arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from 346bc91 to a04f14b Compare February 21, 2026 20:41
@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 21, 2026

The changes are not reverted

@arkgupta17 arkgupta17 force-pushed the improve-subdataframe-copycols-error branch from a04f14b to 1964644 Compare February 21, 2026 21:16
@arkgupta17
Copy link
Copy Markdown
Contributor Author

arkgupta17 commented Feb 21, 2026

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

bkamins
bkamins reviewed Feb 22, 2026
View reviewed changes
bkamins
bkamins reviewed Feb 22, 2026
View reviewed changes
bkamins
bkamins reviewed Feb 22, 2026
View reviewed changes
@bkamins bkamins merged commit b12f7fb into JuliaData:main Feb 22, 2026
7 of 8 checks passed
@bkamins
Copy link
Copy Markdown
Member

bkamins commented Feb 22, 2026

Thank you!

@arkgupta17 arkgupta17 deleted the improve-subdataframe-copycols-error branch February 22, 2026 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkamins bkamins bkamins approved these changes

@nalimilan nalimilan Awaiting requested review from nalimilan

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

patch

Development

Successfully merging this pull request may close these issues.

invalid index with SubDataFrame and copycols = false

3 participants

@arkgupta17 @bkamins @nalimilan
⚡