Join without merge #240
Merged
Join without merge #240
Conversation
This file contains 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
Conflicts: R/step-colorder.R tests/testthat/test-step-join.R
hadley
reviewed
May 11, 2021
R/step-colorder.R
Outdated
|
|
||
| step_call(parent, | ||
| if (is.integer(col_order)) { | ||
| if (all(col_order == seq_along(col_order))) { |
There was a problem hiding this comment.
I think that should use identical() since == is vectorised?
There was a problem hiding this comment.
And maybe move the check until after handling the different types? i.e. if (identical(vars, x$vars)) ...
There was a problem hiding this comment.
The check doesn't work like this as the data.table can have duplicate names. And this actually happens in merge():
library(data.table)
merge(
data.table(x = 1, y = 2),
data.table(x = 1, y = 3, y.x = 4),
by = "x"
)
#> Warning in merge.data.table(data.table(x = 1, y = 2), data.table(x = 1, : column
#> names 'y.x' are duplicated in the result
#> x y.x y.y y.x
#> 1: 1 2 3 4Created on 2021-05-11 by the reprex package (v2.0.0)
And this then also happens in the new implementation of full_join()
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
10 tasks
hadley
approved these changes
Jun 29, 2021
hadley
reviewed
Oct 6, 2021
| switch(x$style, | ||
| full = call2("merge", lhs, rhs, all = TRUE, by.x = x$on$x, by.y = x$on$y, allow.cartesian = TRUE), | ||
| left = call2("[", lhs, rhs, on = on, allow.cartesian = TRUE), | ||
| inner = call2("[", lhs, rhs, on = on, nomatch = NULL), |
There was a problem hiding this comment.
@mgirlich does this deliberately not include allow.cartesian = TRUE? Or was it omitted by accident?
There was a problem hiding this comment.
I'm pretty sure this was by accident.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The main goal was to ensure correct handling of a named
byin the joins and to ensure correct column names also in edge cases. Now,left/right/inner_join()always use[.data.table(). Unfortunately, this makes the translation ofleft_join()longer as it now usually requires calls to both,setcolorder()andsetnames().Could make use of
step_setnames()added in PR #241.Relates to #239. Fixes #243 and fixes #222 and fixes #198.