Skip to content

GH-43440: [R] Unable to filter a factor column with %in%#43446

Merged
nealrichardson merged 1 commit intoapache:mainfrom
nealrichardson:is-in-dict
Sep 22, 2024
Merged

GH-43440: [R] Unable to filter a factor column with %in%#43446
nealrichardson merged 1 commit intoapache:mainfrom
nealrichardson:is-in-dict

Conversation

@nealrichardson
Copy link
Copy Markdown
Member

@nealrichardson nealrichardson commented Jul 27, 2024

Rationale for this change

Fixes #43440

What changes are included in this PR?

The binding for %in% sends the DictionaryType's value_type to cast_or_parse(). It's possible that it would be better to handle this in cast_or_parse(), but it is used in lots of places and I wasn't sure that was correct everywhere. We could certainly find out, but that's a bigger testing exercise than I wanted to take on this afternoon.

Are these changes tested?

Yes.

Are there any user-facing changes?

The bug is fixed.

@github-actions
Copy link
Copy Markdown

⚠️ GitHub issue #43440 has been automatically assigned in GitHub to PR creator.

value_set <- cast_or_parse(value_set, x$type()),
silent = TRUE
value_set <- cast_or_parse(value_set, x_type),
silent = !getOption("arrow.debug", FALSE)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added this while I was in here because one of my initial thoughts was that we were trying to cast to dictionary and it was failing, but the error was being swallowed. It turned out that it was in fact casting to dictionary but that wasn't what was needed.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 27, 2024
Copy link
Copy Markdown
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

Thanks for this! I agree with keeping this limited here and then expanding later if we see if it's a bigger issue.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 21, 2024
@nealrichardson nealrichardson merged commit 81b94dc into apache:main Sep 22, 2024
@nealrichardson nealrichardson deleted the is-in-dict branch September 22, 2024 15:15
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 81b94dc.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[R] Unable to filter a factor column in a Dataset using %in%

2 participants