GH-33666: [R] Remove extraneous argument to semi_join #33693
Conversation
|
|
|
I'm struggling to work out why the linter is failing this; it works fine for me locally(!) |
There was a PR merged (#33620 (comment)) that broke it on master. I have merged the fix on master if you rebase should be fixed. |
thisisnic
force-pushed
the
GH-33666_test_failures
branch
from
275bb71
to
d8822cb
Compare
| @@ -82,7 +82,7 @@ test_that("left_join with join_by", { | |||
| left_join( | |||
| to_join %>% | |||
| rename(the_grouping = some_grouping), | |||
| join_by(some_grouping == the_grouping) | |||
| join_by(some_grouping == the_grouping) | |||
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
It's the consequence of running the styler.
r/R/dplyr-join.R
Outdated
| ...) { | ||
| do_join(x, y, by, copy, suffix, ..., join_type = "LEFT_SEMI") | ||
| do_join(x, y, by, copy, ..., join_type = "LEFT_SEMI") |
There was a problem hiding this comment.
Are changes in this file necessary to make the tests pass? (Or do they have another purpose, like perhaps making this function more consistent with other arrow join implementations?)
There was a problem hiding this comment.
It makes the API consistent with dplyr, but not necessary to make the tests pass.
There was a problem hiding this comment.
I'm ok with that change; however, I think it should be consistent for all of our join wrappers (anti, left, right, etc.). Currently they all define the suffix argument explicitly (and I think the default value of c(".x", ".y") is necessary and may result in an error if you try to semi_join overlapping columns?).
There was a problem hiding this comment.
I've removed this change as the anti_join() signature is also different to dplyr's. I don't quite understand what you mean; please can we discuss this in #33709?
|
Benchmark runs are scheduled for baseline = bff9e5e and contender = fa59051. fa59051 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
This PR removes the `keep` argument from the test for `semi_join()`, which are causing the unit tests to fail. It also removes the argument `suffix` argument (which is not part of the dplyr function signature) from the function signature here. Closes: #33666 Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
This PR removes the
keepargument from the test forsemi_join(), which are causing the unit tests to fail. It also removes the argumentsuffixargument (which is not part of the dplyr function signature) from the function signature here.Closes: #33666