MINOR: [R] Work around test failure in tidyquery revdep #43498
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
38 tasks
jonkeane
reviewed
Jul 31, 2024
github-actions
bot
added
awaiting changes
jonkeane
reviewed
Jul 31, 2024
| } | ||
|
|
||
| # Else, warn, collect(), and run in regular dplyr | ||
| call <- get("call", envir = env) |
Member
There was a problem hiding this comment.
I'm a little surprised by this deletion, was this essentially overwritting what was in call when it ran? Or something else?
Member
Author
There was a problem hiding this comment.
I just moved it up to L227
Member
There was a problem hiding this comment.
AAAAAH I totally missed that. Thanks
github-actions
bot
added
awaiting merge
jonkeane
pushed a commit
that referenced
this pull request
Jul 31, 2024
### Rationale for this change See #43317 (comment). `tidyquery` is assembling queries in some way such that when `summarize.arrow_dplyr_query()` is called, the calling environment isn't a call, so `match.call()` fails. ### What changes are included in this PR? This PR wraps the `match.call()` call in a `try()`. The call is only used to do `abandon_ship()` on in-memory data anyway. So if the call is not available, it treats it like you're making a query on a Dataset and it tells you to `collect()` yourself. ### Are these changes tested? I couldn't figure out how to reproduce what was going on inside `tidyquery` to write a reproducer, and I don't think this is worth adding `tidyquery` to Suggests for. I confirmed locally that `tidyquery` tests pass with this change, so our revdeps should be clear. ### Are there any user-facing changes? 🙅 Authored-by: Neal Richardson <neal.p.richardson@gmail.com> Signed-off-by: Jonathan Keane <jkeane@gmail.com>
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 0dec116. There were 3 benchmark results indicating a performance regression:
The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
See #43317 (comment).
tidyqueryis assembling queries in some way such that whensummarize.arrow_dplyr_query()is called, the calling environment isn't a call, somatch.call()fails.What changes are included in this PR?
This PR wraps the
match.call()call in atry(). The call is only used to doabandon_ship()on in-memory data anyway. So if the call is not available, it treats it like you're making a query on a Dataset and it tells you tocollect()yourself.Are these changes tested?
I couldn't figure out how to reproduce what was going on inside
tidyqueryto write a reproducer, and I don't think this is worth addingtidyqueryto Suggests for. I confirmed locally thattidyquerytests pass with this change, so our revdeps should be clear.Are there any user-facing changes?
🙅