Skip to content

ARROW-13995: [R] Bindings for join node - #11155

Closed
nealrichardson wants to merge 7 commits into
apache:masterfrom
nealrichardson:join-dplyr
Closed

ARROW-13995: [R] Bindings for join node#11155
nealrichardson wants to merge 7 commits into
apache:masterfrom
nealrichardson:join-dplyr

Conversation

@nealrichardson

@nealrichardsonnealrichardson commented Sep 14, 2021

Copy link
Copy Markdown
Member

This is based on #11150.

Among the issues observed:

  • Dictionary columns aren't allowed even in the left data, though you can first Project to remove them
  • Duplicate column names aren't allowed at all, even though there is a provision for deduping with a prefix

@apacheapache deleted a comment from github-actionsBotSep 14, 2021
@nealrichardsonnealrichardson changed the title [WIP] ARROW-13995: [R] Bindings for join nodeARROW-13995: [R] Bindings for join nodeSep 20, 2021
@github-actions

Copy link
Copy Markdown

@jonkeanejonkeane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, a few comments / suggestions. Once #11150 merges I'll rebase + push

Comment threadr/R/dplyr-join.R Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not a comment on this code, but it never occurred to me that it's a bit funny that by isn't tidy-evaled and must be character strings (in dplyr!)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It makes sense for the by = c(col_x = "col_y") interface, but a bit(?) odd for by = "col" case

Comment threadr/tests/testthat/test-dplyr-join.R Outdated
Comment threadr/tests/testthat/test-dplyr-join.R Outdated
Comment threadr/tests/testthat/test-dplyr-join.R Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can do this when I rebase, but should we have tests for the error handling as well (e.g. columns named but not in x or y, etc.)?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Oh yeah, there are lots of missing tests, and these datasets aren't ideal for testing joins. We may want to just use the datasets that dplyr uses in its tests or something.

@nealrichardson
nealrichardson deleted the join-dplyr branch October 1, 2021 16:19
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
This is based on apache#11150.
Among the issues observed:
* Dictionary columns aren't allowed even in the left data, though you can first `Project` to remove them
* Duplicate column names aren't allowed at all, even though there is a provision for deduping with a prefix
Closesapache#11155 from nealrichardson/join-dplyr
Lead-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@asfimportasfimport mentioned this pull request Sep 30, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@nealrichardson@jonkeane