[GitHub] [arrow] jonkeane commented on a change in pull request #12113: ARROW-14679: [R] [C++] Handle suffix argument in joins

[GitBox <gi...@apache.org>]

jonkeane commented on a change in pull request #12113:
URL: https://github.com/apache/arrow/pull/12113#discussion_r823923555



##########
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 think it's defined up above on 103. The tests that exercise `implicit_schema` are at https://github.com/apache/arrow/blob/dfca6a704ad7e8e87e1c8c3d0224ba13b25786ea/r/tests/testthat/test-dplyr-collapse.R#L34-L87
   
   It would be fantastic to get a few of these joins to those tests too.

##########
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) {

Review comment:
       We also could move this to the top level of the file. That's more typical for helpers like this (and that'll mean we can test it separately if we wanted to). 




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org