Fix incorrect keying after merge of keyed, non-alphabetic factor and character columns

NEWS.md

@@ -192,6 +192,8 @@ rowwiseDT(
 
 14. Passing functions programmatically with `env=` doesn't produce an opaque error, e.g. `DT[, f(b), env = list(f=sum)]`, [#6026](https://github.com/Rdatatable/data.table/issues/6026). Note that it's much better to pass functions like `f="sum"` instead. Thanks to @MichaelChirico for the bug report and fix.
 
+15. If the first column of a key is a factor column containing unsorted levels (`setkey()` does not order the levels) and it is joined to a character column, then an invalidly keyed result would be produced. This could lead to a subsequent error/warning or silent incorrect results. Thanks to @gbrunick for the report and Benjamin Schwendinger for the fix.
+
 ## NOTES
 
 1. `transform()` method for data.table sped up substantially when creating new columns on large tables. Thanks to @OfekShilon for the report and PR. The implemented solution was proposed by @ColeMiller1.

R/data.table.R

@@ -1337,8 +1337,22 @@ replace_dot_alias = function(e) {
           ## check key on i as well!
           ichk = is.data.table(i) && haskey(i) &&
                  identical(head(key(i), length(leftcols)), names_i[leftcols]) # i has the correct key, #3061
-          if (keylen && (ichk || is.logical(i) || (.Call(CisOrderedSubset, irows, nrow(x)) && ((roll == FALSE) || length(irows) == 1L)))) # see #1010. don't set key when i has no key, but irows is ordered and roll != FALSE
-            setattr(ans,"sorted",head(key(x),keylen))
+          dt_types = function(x, cols = seq_along(x)) vapply_1c(cols, function(j) typeof(x[[j]]))
+          if (
+            keylen
+            && (
+              ichk
+              || is.logical(i)
+              || (
+                .Call(CisOrderedSubset, irows, nrow(x))
+                && (!roll || length(irows) == 1L) # see #1010. don't set key when i has no key, but irows is ordered and roll != FALSE
+                && (  #5361 merging on keyed factor with character, check if resulting character is really sorted
+                  identical(dt_types(i, leftcols), dt_types(x, rightcols)) # can only be not identical
+                  || is.sorted(ans, by=head(key(x), keylen))
+                )
+              )
+            )
+          ) setattr(ans,"sorted",head(key(x),keylen))
         }
         setattr(ans, "class", class(x))  # retain class that inherits from data.table, #64
         setattr(ans, "row.names", .set_row_names(length(ans[[1L]])))

inst/tests/tests.Rraw

@@ -7070,6 +7070,30 @@ test(1483.3, merge(x,y,by="country",all=TRUE), data.table(country=factor(c("US",
 setkey(y)
 test(1483.4, y[x], data.table(country="US", key="country"))
 
+# 5361 merge on character and factor should only have key(x) if result is really sorted
+let = c("a", "b", "c")
+rlet = c("c", "b", "a")
+x = data.table(x=rlet)
+y = data.table(x=factor(let, levels=rlet), key="x")
+test(1483.51, x[y, on="x"], x)
+test(1483.52, y[x, on="x"], x)
+test(1483.53, merge(x, y, by="x"), data.table(x=let, key="x"))
+test(1483.54, merge(y, x, by="x"), data.table(x=let, key="x"))
+
+x = data.table(x=rlet, key="x")
+y = data.table(x=factor(let, levels=rlet))
+test(1483.61, x[y, on="x"], x)
+test(1483.62, y[x, on="x"], data.table(x=let))
+test(1483.63, merge(x, y, by="x"), data.table(x=let, key="x"))
+test(1483.64, merge(y, x, by="x"), data.table(x=let, key="x"))
+
+x = data.table(x=rlet, a=rlet)
+y = data.table(x=factor(let, levels=rlet), b=let, key="x")
+test(1483.71, x[y, on="x"], data.table(x=rlet, a=rlet, b=rlet))
+test(1483.72, y[x, on="x"], data.table(x=rlet, b=rlet, a=rlet))
+test(1483.73, merge(x, y, by="x"), data.table(x=let, a=let, b=let, key="x"))
+test(1483.74, merge(y, x, by="x"), data.table(x=let, b=let, a=let, key="x"))
+
 # NULL items should be removed when making data.table from list, #842
 # Original fix for #842 added a branch in as.data.table.list() using point()
 # Then PR#3471 moved logic from data.table() into as.data.table.list() and now removes NULL items up front, so longer need for the branch