westonpace commented on a change in pull request #12113:
URL: https://github.com/apache/arrow/pull/12113#discussion_r823289010
##########
File path: r/R/dplyr-collect.R
##########
@@ -103,10 +104,37 @@ implicit_schema <- function(.data) {
if (!is.null(.data$join) && !(.data$join$type %in% JoinType[1:4])) {
# Add cols from right side, except for semi/anti joins
right_cols <- .data$join$right_data$selected_columns
- new_fields <- c(new_fields, map(
+ left_cols <- .data$selected_columns
+ right_fields <- map(
right_cols[setdiff(names(right_cols), .data$join$by)],
~ .$type(.data$join$right_data$.data$schema)
- ))
+ )
+ # get right table and left table column names excluding the join key
+ right_cols_ex_by <- right_cols[setdiff(names(right_cols), .data$join$by)]
+ left_cols_ex_by <- left_cols[setdiff(names(left_cols), .data$join$by)]
+ # find the common column names in left and right tables
+ common_cols <- intersect(names(right_cols_ex_by), names(left_cols_ex_by))
+
+ # helper method to add suffix
+ add_suffix <- function(fields, common_cols, suffix) {
+ # helper function which adds the suffixes to the
+ # selected column names
+ # for join relation the selected columns are the
+ # columns with same name in left and right relation
+ col_names <- names(fields)
+ new_col_names <- col_names %>% map(function(x) {
+ if (is.element(x, common_cols)) {
+ paste(x, suffix, sep = "")
+ } else {
+ x
+ }
+ })
+ rlang::set_names(fields, new_col_names)
Review comment:
I don't think the `rlang::` is needed here based on my scanning of where
else `set_names` is used.
##########
File path: r/R/dplyr-collect.R
##########
@@ -103,10 +104,37 @@ implicit_schema <- function(.data) {
if (!is.null(.data$join) && !(.data$join$type %in% JoinType[1:4])) {
# Add cols from right side, except for semi/anti joins
right_cols <- .data$join$right_data$selected_columns
- new_fields <- c(new_fields, map(
+ left_cols <- .data$selected_columns
+ right_fields <- map(
right_cols[setdiff(names(right_cols), .data$join$by)],
~ .$type(.data$join$right_data$.data$schema)
- ))
+ )
+ # get right table and left table column names excluding the join key
+ right_cols_ex_by <- right_cols[setdiff(names(right_cols), .data$join$by)]
+ left_cols_ex_by <- left_cols[setdiff(names(left_cols), .data$join$by)]
+ # find the common column names in left and right tables
+ common_cols <- intersect(names(right_cols_ex_by), names(left_cols_ex_by))
+
+ # helper method to add suffix
+ add_suffix <- function(fields, common_cols, suffix) {
+ # helper function which adds the suffixes to the
+ # selected column names
+ # for join relation the selected columns are the
+ # columns with same name in left and right relation
+ col_names <- names(fields)
+ new_col_names <- col_names %>% map(function(x) {
+ if (is.element(x, common_cols)) {
+ paste(x, suffix, sep = "")
+ } else {
+ x
+ }
+ })
+ rlang::set_names(fields, new_col_names)
+ }
+ # adding suffixes to the common columns in left and right tables
+ left_fields <- add_suffix(new_fields, common_cols,
.data$join$suffix[[1]])
Review comment:
I'm not sure this can be correct. `new_fields` is not defined any
longer. Do you know if any of the tests exercise this path? I don't really
understand the purpose of the `implicit_schema` function.
##########
File path: r/R/query-engine.R
##########
@@ -19,7 +19,6 @@ do_exec_plan <- function(.data) {
plan <- ExecPlan$create()
final_node <- plan$Build(.data)
tab <- plan$Run(final_node)
-
Review comment:
Minor nit: Try and minimize unrelated whitespace-only changes when
possible.
##########
File path: r/R/dplyr-collect.R
##########
@@ -103,10 +104,37 @@ implicit_schema <- function(.data) {
if (!is.null(.data$join) && !(.data$join$type %in% JoinType[1:4])) {
# Add cols from right side, except for semi/anti joins
right_cols <- .data$join$right_data$selected_columns
- new_fields <- c(new_fields, map(
+ left_cols <- .data$selected_columns
+ right_fields <- map(
right_cols[setdiff(names(right_cols), .data$join$by)],
~ .$type(.data$join$right_data$.data$schema)
- ))
+ )
+ # get right table and left table column names excluding the join key
+ right_cols_ex_by <- right_cols[setdiff(names(right_cols), .data$join$by)]
+ left_cols_ex_by <- left_cols[setdiff(names(left_cols), .data$join$by)]
+ # find the common column names in left and right tables
+ common_cols <- intersect(names(right_cols_ex_by), names(left_cols_ex_by))
+
+ # helper method to add suffix
+ add_suffix <- function(fields, common_cols, suffix) {
+ # helper function which adds the suffixes to the
+ # selected column names
+ # for join relation the selected columns are the
+ # columns with same name in left and right relation
+ col_names <- names(fields)
+ new_col_names <- col_names %>% map(function(x) {
+ if (is.element(x, common_cols)) {
+ paste(x, suffix, sep = "")
Review comment:
```suggestion
paste0(x, suffix)
```
##########
File path: r/R/query-engine.R
##########
@@ -166,14 +165,18 @@ ExecPlan <- R6Class("ExecPlan",
# (as when we've done collapse() and not projected after) is
cheap/no-op
projection <- c(.data$selected_columns, .data$temp_columns)
node <- node$Project(projection)
-
if (!is.null(.data$join)) {
+ right_node <- self$Build(.data$join$right_data)
+ left_output <- names(.data)
Review comment:
Why aren't we doing `setdiff(names(.data), .data$join$by)`? It's
possible I just don't understand but it seems like both sides would have the
`by` columns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
