jhdf

Author: Mukulyadav2004

R/bmerge.R

@@ -1,12 +1,17 @@
+
+
 mergeType = function(x) {
   ans = typeof(x)
   if      (ans=="integer") { if (is.factor(x))             ans = "factor"    }
   else if (ans=="double")  { if (inherits(x, "integer64")) ans = "integer64" }
+  # do not call fitsInInt*(x) yet because i) if both types are double we don't need to coerce even if one or both sides
+  # are int-as-double, and ii) to save calling it until we really need it
   ans
 }
 
 cast_with_attrs = function(x, cast_fun) {
   ans = cast_fun(x)
+  # do not copy attributes when coercing factor (to character)
   if (!is.factor(x) && !is.null(attributes(x))) attributes(ans) = attributes(x)
   ans
 }
@@ -24,6 +29,18 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
 {
   callersi = i
   i = shallow(i)
+  # Just before the call to bmerge() in [.data.table there is a shallow() copy of i to prevent coercions here
+  # by bmerge changing the type of the user's input object by reference. We now shallow copy i again. If we then
+  # coerce a column in i only, we are just changing the temporary coercion used for the merge operation. If we
+  # set callersi too then we are keeping that coerced i column in the merge result returned to user.
+  # The type of the i column is always returned (i.e. just i set not callersi too), other than:
+  #   i) to convert int-as-double to int, useful for ad hoc joins when the L postfix is often forgotten.
+  #  ii) to coerce i.factor to character when joining to x.character
+  # So those are the only two uses of callersi below.
+  # Careful to only use plonk syntax (full column) on i and x from now on, otherwise user's i and x would
+  # change. This is why shallow() is very importantly internal only, currently.
+
+  # Using .SD in j to join could fail due to being locked and set() being used here, #1926
   .Call(C_unlock, i)
   x = shallow(x)
   .Call(C_unlock, x)
@@ -35,6 +52,10 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
   supported = c(ORDERING_TYPES, "factor", "integer64")
 
   if (nrow(i)) for (a in seq_along(icols)) {
+    # - check that join columns have compatible types
+    # - do type coercions if necessary on just the shallow local copies for the purpose of join
+    # - handle factor columns appropriately
+    # Note that if i is keyed, if this coerces i's key gets dropped by set()
     icol = icols[a]
     xcol = xcols[a]
     x_merge_type = mergeType(x[[xcol]])
@@ -43,48 +64,44 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
     iname = paste0("i.", names(i)[icol])
     if (!x_merge_type %chin% supported) stopf("%s is type %s which is not supported by data.table join", xname, x_merge_type)
     if (!i_merge_type %chin% supported) stopf("%s is type %s which is not supported by data.table join", iname, i_merge_type)
-
     if (x_merge_type=="factor" || i_merge_type=="factor") {
       if (roll!=0.0 && a==length(icols))
         stopf("Attempting roll join on factor column when joining %s to %s. Only integer, double or character columns may be roll joined.", xname, iname)
       if (x_merge_type=="factor" && i_merge_type=="factor") {
         if (verbose) catf("Matching %s factor levels to %s factor levels.\n", iname, xname)
-        set(i, j=icol, value=chmatch(levels(i[[icol]]), levels(x[[xcol]]), nomatch=0L)[i[[icol]]])
+        set(i, j=icol, value=chmatch(levels(i[[icol]]), levels(x[[xcol]]), nomatch=0L)[i[[icol]]])  # nomatch=0L otherwise a level that is missing would match to NA values
         next
       } else {
         if (x_merge_type=="character") {
           coerce_col(i, icol, "factor", "character", iname, xname, verbose=verbose)
-          set(callersi, j=icol, value=i[[icol]])
+          set(callersi, j=icol, value=i[[icol]])  # factor in i joining to character in x will return character and not keep x's factor; e.g. for antaresRead #3581
           next
         } else if (i_merge_type=="character") {
           if (verbose) catf("Matching character column %s to factor levels in %s.\n", iname, xname)
           newvalue = chmatch(i[[icol]], levels(x[[xcol]]), nomatch=0L)
-          if (anyNA(i[[icol]])) newvalue[is.na(i[[icol]])] = NA_integer_
+          if (anyNA(i[[icol]])) newvalue[is.na(i[[icol]])] = NA_integer_  # NA_character_ should match to NA in factor, #3809
           set(i, j=icol, value=newvalue)
           next
         }
       }
-      # Incompatible factor join: Factor vs (Not Factor and Not Character)
-      # The 'message' attribute must match the *old* error for direct calls to bmerge (e.g., DT[otherDT])
       condition_message <- sprintf(
         "Incompatible join types: %s (%s) and %s (%s). Factor columns must join to factor or character columns.", # Exact match for tests like 2044.24
         xname, x_merge_type,
         iname, i_merge_type
       )
-
       condition <- structure(
         list(
           message = condition_message,
-          c_bmerge_x_arg_bare_col_name = names(x)[xcol],
-          c_bmerge_x_arg_type          = x_merge_type,
-          c_bmerge_i_arg_bare_col_name = names(i)[icol],
-          c_bmerge_i_arg_type          = i_merge_type
+          bmerge_x_arg_col_name      = names(x)[xcol],
+          bmerge_x_arg_type          = x_merge_type,
+          bmerge_i_arg_col_name      = names(i)[icol],
+          bmerge_i_arg_type          = i_merge_type
         ),
         class = c("bmerge_incompatible_type_error", "data.table_error", "error", "condition")
       )
       stop(condition)
     }
-
+    # we check factors first to cater for the case when trying to do rolling joins on factors
     if (x_merge_type == i_merge_type) {
       if (verbose) catf("%s has same type (%s) as %s. No coercion needed.\n", iname, x_merge_type, xname)
       next
@@ -99,23 +116,24 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
         coerce_col(x, xcol, x_merge_type, i_merge_type, xname, iname, from_detail=gettext(" (all-NA)"), verbose=verbose)
         next
       }
-      # This 'stopf' might have been the one originally hit by the failing tests.
-      # Our custom error above now preempts this for incompatible factor joins.
       stopf("Incompatible join types: %s (%s) and %s (%s)", xname, x_merge_type, iname, i_merge_type)
     }
     if (x_merge_type=="integer64" || i_merge_type=="integer64") {
       nm = c(iname, xname)
-      if (x_merge_type=="integer64") { w=i; wc=icol; wclass=i_merge_type; } else { w=x; wc=xcol; wclass=x_merge_type; nm=rev(nm) }
+      if (x_merge_type=="integer64") { w=i; wc=icol; wclass=i_merge_type; } else { w=x; wc=xcol; wclass=x_merge_type; nm=rev(nm) }  # w is which to coerce
       if (wclass=="integer" || (wclass=="double" && fitsInInt64(w[[wc]]))) {
         from_detail = if (wclass == "double") gettext(" (which has integer64 representation, e.g. no fractions)") else ""
         coerce_col(w, wc, wclass, "integer64", nm[1L], nm[2L], from_detail, verbose=verbose)
       } else stopf("Incompatible join types: %s is type integer64 but %s is type double and cannot be coerced to integer64 (e.g. has fractions)", nm[2L], nm[1L])
     } else {
-      ic_idx = which(icol == icols)
+      # just integer and double left
+      ic_idx = which(icol == icols) # check if on is joined on multiple conditions, #6602
       if (i_merge_type=="double") {
         coerce_x = FALSE
         if (fitsInInt32(i[[icol]])) {
           coerce_x = TRUE
+          # common case of ad hoc user-typed integers missing L postfix joining to correct integer keys
+          # we've always coerced to int and returned int, for convenience.
           if (length(ic_idx)>1L) {
             xc_idx = xcols[ic_idx]
             for (xb in xc_idx[which(vapply_1c(.shallow(x, xc_idx), mergeType) == "double")]) {
@@ -128,7 +146,7 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
           if (coerce_x) {
             from_detail = gettext(" (which contains no fractions)")
             coerce_col(i, icol, "double", "integer", iname, xname, from_detail, verbose=verbose)
-            set(callersi, j=icol, value=i[[icol]])
+            set(callersi, j=icol, value=i[[icol]])       # change the shallow copy of i up in [.data.table to reflect in the result, too.
             if (length(ic_idx)>1L) {
               xc_idx = xcols[ic_idx]
               for (xb in xc_idx[which(vapply_1c(.shallow(x, xc_idx), mergeType) == "double")]) {
@@ -152,8 +170,11 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
     }
   }
 
-  non_equi = which.first(ops != 1L)
+  ## after all modifications of x, check if x has a proper key on all xcols.
+  ## If not, calculate the order. Also for non-equi joins, the order must be calculated.
+  non_equi = which.first(ops != 1L) # 1 is "==" operator
   if (is.na(non_equi)) {
+    # equi join. use existing key (#1825) or existing secondary index (#1439)
     if (identical(xcols, head(chmatch(key(x), names(x)), length(xcols)))) {
       xo = integer(0L)
       if (verbose) catf("on= matches existing key, using key\n")
@@ -167,21 +188,27 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
         if (verbose) {last.started.at=proc.time(); flush.console()}
         xo = forderv(x, by = xcols)
         if (verbose) {catf("Calculated ad hoc index in %s\n", timetaken(last.started.at)); flush.console()}
+        # TODO: use setindex() instead, so it's cached for future reuse
       }
     }
+    ## these variables are only needed for non-equi joins. Set them to default.
     nqgrp = integer(0L)
     nqmaxgrp = 1L
   } else {
+    # non-equi operators present.. investigate groups..
     nqgrp = integer(0L)
     nqmaxgrp = 1L
     if (verbose) catf("Non-equi join operators detected ... \n")
     if (roll != FALSE) stopf("roll is not implemented for non-equi joins yet.")
     if (verbose) {last.started.at=proc.time();catf("  forder took ... ");flush.console()}
+    # TODO: could check/reuse secondary indices, but we need 'starts' attribute as well!
     xo = forderv(x, xcols, retGrp=TRUE)
-    if (verbose) {cat(timetaken(last.started.at),"\n"); flush.console()}
+    if (verbose) {cat(timetaken(last.started.at),"\n"); flush.console()} # notranslate
     xg = attr(xo, 'starts', exact=TRUE)
     resetcols = head(xcols, non_equi-1L)
     if (length(resetcols)) {
+      # TODO: can we get around having to reorder twice here?
+      # or at least reuse previous order?
       if (verbose) {last.started.at=proc.time();catf("  Generating group lengths ... ");flush.console()}
       resetlen = attr(forderv(x, resetcols, retGrp=TRUE), 'starts', exact=TRUE)
       resetlen = .Call(Cuniqlengths, resetlen, nrow(x))
@@ -190,8 +217,8 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
     if (verbose) {last.started.at=proc.time();catf("  Generating non-equi group ids ... ");flush.console()}
     nqgrp = .Call(Cnestedid, x, xcols[non_equi:length(xcols)], xo, xg, resetlen, mult)
     if (verbose) {catf("done in %s\n",timetaken(last.started.at)); flush.console()}
-    if (length(nqgrp)) nqmaxgrp = max(nqgrp)
-    if (nqmaxgrp > 1L) {
+    if (length(nqgrp)) nqmaxgrp = max(nqgrp) # fix for #1986, when 'x' is 0-row table max(.) returns -Inf.
+    if (nqmaxgrp > 1L) { # got some non-equi join work to do
       if ("_nqgrp_" %in% names(x)) stopf("Column name '_nqgrp_' is reserved for non-equi joins.")
       if (verbose) {last.started.at=proc.time();catf("  Recomputing forder with non-equi ids ... ");flush.console()}
       set(nqx<-shallow(x), j="_nqgrp_", value=nqgrp)
@@ -205,7 +232,8 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
   if (verbose) {last.started.at=proc.time();catf("Starting bmerge ...\n");flush.console()}
   ans = .Call(Cbmerge, i, x, as.integer(icols), as.integer(xcols), xo, roll, rollends, nomatch, mult, ops, nqgrp, nqmaxgrp)
   if (verbose) {catf("bmerge done in %s\n",timetaken(last.started.at)); flush.console()}
+  # TO DO: xo could be moved inside Cbmerge
 
-  ans$xo = xo
+  ans$xo = xo  # for further use by [.data.table
   ans
-}
+}

R/merge.R

@@ -1,9 +1,5 @@
 merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FALSE, all.x = all,
                all.y = all, sort = TRUE, suffixes = c(".x", ".y"), no.dups = TRUE, allow.cartesian=getOption("datatable.allow.cartesian"), incomparables=NULL, ...) {
-
-  # NO user_x_name / user_y_name at the top to maintain original variable environment for most of the function
-  # They will be fetched *only* within the error handler if needed.
-
   if (!sort %in% c(TRUE, FALSE))
     stopf("Argument 'sort' should be logical TRUE/FALSE")
   if (!no.dups %in% c(TRUE, FALSE))
@@ -12,7 +8,7 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
   if (!is.data.table(y)) {
     y = as.data.table(y)
     if (missing(by) && missing(by.x)) {
-      by = key(x) # Original logic
+      by = key(x)
     }
   }
   x0 = length(x) == 0L
@@ -21,17 +17,17 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
     if (x0 && y0)
       warningf("Neither of the input data.tables to join have columns.")
     else if (x0)
-      warningf("Input data.table '%s' has no columns.", "x") # Original: literal "x"
+      warningf("Input data.table '%s' has no columns.", "x")
     else
-      warningf("Input data.table '%s' has no columns.", "y") # Original: literal "y"
+      warningf("Input data.table '%s' has no columns.", "y")
   }
   check_duplicate_names(x)
   check_duplicate_names(y)
 
-  nm_x = names(x) # Original logic
-  nm_y = names(y) # Original logic
+  nm_x = names(x)
+  nm_y = names(y)
 
-  ## set up 'by'/'by.x'/'by.y' - RETAIN ORIGINAL LOGIC EXACTLY
+  ## set up 'by'/'by.x'/'by.y'
   if ((!is.null(by.x) || !is.null(by.y)) && length(by.x) != length(by.y))
     stopf("`by.x` and `by.y` must be of same length.")
   if (!missing(by) && !missing(by.x))
@@ -50,7 +46,7 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
   } else {
     if (is.null(by))
       by = intersect(key(x), key(y))
-    if (!length(by))
+    if (!length(by))   # was is.null() before PR#5183  changed to !length()
       by = key(x)
     if (!length(by))
       by = intersect(nm_x, nm_y)
@@ -66,26 +62,33 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
     by.x = by.y = by
   }
 
+  # warn about unused arguments #2587
   if (length(list(...))) {
     ell = as.list(substitute(list(...)))[-1L]
     for (n in setdiff(names(ell), "")) warningf("Unknown argument '%s' has been passed.", n)
     unnamed_n = length(ell) - sum(nzchar(names(ell)))
     if (unnamed_n)
       warningf("Passed %d unknown and unnamed arguments.", unnamed_n)
   }
-
+  # with i. prefix in v1.9.3, this goes away. Left here for now ...
+  ## sidestep the auto-increment column number feature-leading-to-bug by
+  ## ensuring no names end in ".1", see unit test
+  ## "merge and auto-increment columns in y[x]" in test-data.frame.like.R
   start = setdiff(nm_x, by.x)
   end = setdiff(nm_y, by.y)
   dupnames = intersect(start, end)
   if (length(dupnames)) {
     start[chmatch(dupnames, start, 0L)] = paste0(dupnames, suffixes[1L])
     end[chmatch(dupnames, end, 0L)] = paste0(dupnames, suffixes[2L])
   }
+  # If no.dups = TRUE we also need to added the suffix to columns in y
+  # that share a name with by.x
   dupkeyx = intersect(by.x, end)
   if (no.dups && length(dupkeyx)) {
     end[chmatch(dupkeyx, end, 0L)] = paste0(dupkeyx, suffixes[2L])
   }
 
+  # implement incomparables argument #2587
   if (!is.null(incomparables)) {
     "%fin%" = function(x_val, table_val) if (is.character(x_val) && is.character(table_val)) x_val %chin% table_val else x_val %in% table_val
     xind = rowSums(x[, lapply(.SD, function(x_col_val) !(x_col_val %fin% incomparables)), .SDcols=by.x]) == length(by.x)
@@ -100,16 +103,16 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
     bmerge_incompatible_type_error = function(e) {
       # For merge(x=DT1, y=DT2), DT1 (user's 'x') is bmerge's 'i'
       #                            DT2 (user's 'y') is bmerge's 'x'
-      x_part_col_name <- e$c_bmerge_i_arg_bare_col_name
-      x_part_type     <- e$c_bmerge_i_arg_type
-      y_part_col_name <- e$c_bmerge_x_arg_bare_col_name
-      y_part_type     <- e$c_bmerge_x_arg_type
+      x_part_col_name <- e$bmerge_i_arg_col_name
+      x_part_type     <- e$bmerge_i_arg_type
+      y_part_col_name <- e$bmerge_x_arg_col_name
+      y_part_type     <- e$bmerge_x_arg_type
 
       # Use literal "x." and "y." prefixes referring to the arguments of merge()
       msg <- sprintf(
-        "Incompatible join types: x.%s (%s) and y.%s (%s). Factor columns must join to factor or character columns.",
-        x_part_col_name, x_part_type,  # Corresponds to merge() argument 'x'
-        y_part_col_name, y_part_type   # Corresponds to merge() argument 'y'
+        "Incompatible join types: x.%s (%s) and i.%s (%s). Factor columns must join to factor or character columns.",
+        x_part_col_name, x_part_type, 
+        y_part_col_name, y_part_type   
       )
 
       # Remove call = NULL to get "Error in merge.data.table(...): " prefix.
@@ -121,19 +124,23 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
     missingyidx = y[!x, which=TRUE, on=by, allow.cartesian=allow.cartesian]
     if (length(missingyidx)) dt = rbind(dt, y[missingyidx], use.names=FALSE, fill=TRUE, ignore.attr=TRUE)
   }
-  
+  # X[Y] syntax puts JIS i columns at the end, merge likes them alongside i.
   newend = setdiff(nm_y, by.y)
+  # fix for #1290, make sure by.y order is set properly before naming
   setcolorder(dt, c(by.y, setdiff(names(dt), c(by.y, newend)), newend))
   setnames(dt, c(by.x, start, end))
   if (nrow(dt) > 0L) {
     setkeyv(dt, if (sort) by.x else NULL)
   }
 
+  # Throw warning if there are duplicate column names in 'dt' (i.e. if
+  # `suffixes=c("","")`, to match behaviour in base:::merge.data.frame)
   resultdupnames = names(dt)[duplicated(names(dt))]
   if (length(resultdupnames)) {
     warningf("column names %s are duplicated in the result", brackify(resultdupnames))
   }
 
+  # retain custom classes of first argument that resulted in dispatch to this method, #1378
   setattr(dt, "class", class_x)
   dt
 }