Joins do not warn user when using POSc and Date comparisons

[al-obrien]

Issue

There is a possible silent warning/error that occurs when joining on POSc and Date columns. It is unclear if this is expected behaviour based on my readings.

The example below shows how the same tables (dt1 and dt2) either join or fail to (left) join based upon the format of the date/time columns in dt1 (either POSc or Date). Although the simple <= and >= comparisons on vectors pass a warning, there is no warning passed when performing a join, which led to confusion as to the cause of join difference.

Example

library(data.table)

# POSc
dt1_posc <- data.table(id = 1,
		date_time1 = as.POSIXct('2017-01-01'),
		date_time2 = as.POSIXct('2020-01-01'), 
		infocol = 'test')
dt1 <- data.table(id = 1,
		date_time1 = as.Date('2017-01-01'),
		date_time2 = as.Date('2020-01-01'), 
		infocol = 'test')

dt2 <- data.table(id = c(1, 1, 2),
		date_1=as.Date(c('2018-01-01', '2019-01-01', '2010-01-01')),
		date_2=as.Date(c('2018-06-01', '2020-11-01', '2010-01-01')))

# Works
dt1[dt2, on = .(id, date_time1 <= date_2, date_time2 >= date_1)]
#>      id date_time1 date_time2 infocol
#>   <num>     <Date>     <Date>  <char>
#>1:     1 2018-06-01 2018-01-01    test
#>2:     1 2020-11-01 2019-01-01    test
#>3:     2 2010-01-01 2010-01-01    <NA>

# Doesnt work
dt1_posc[dt2, on = .(id, date_time1 <= date_2, date_time2 >= date_1)]
#>      id date_time1 date_time2 infocol
#>   <num>     <Date>     <Date>  <char>
#>1:     1 2018-06-01 2018-01-01    <NA>
#>2:     1 2020-11-01 2019-01-01    <NA>
#>3:     2 2010-01-01 2010-01-01    <NA>

# Check simple comparison
dt1_posc$date_time1 <= dt2$date_2 & dt1_posc$date_time2 >= dt2$date_1
#> [1] FALSE FALSE FALSE
#> Warning message:
#> Incompatible methods ("Ops.POSIXt", "Ops.Date") for "<=" 
dt1$date_time1 <= dt2$date_2 & dt1$date_time2 >= dt2$date_1
#> [1]  TRUE  TRUE FALSE

Session info

I used the latest version of data.table on CRAN (1.16.2) in the following session

R version 4.3.1 (2023-06-16 ucrt)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19045)

Matrix products: default


locale:
[1] LC_COLLATE=English_Canada.utf8  LC_CTYPE=English_Canada.utf8    LC_MONETARY=English_Canada.utf8 LC_NUMERIC=C                    LC_TIME=English_Canada.utf8    

time zone: America/Edmonton
tzcode source: internal

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] data.table_1.16.2

loaded via a namespace (and not attached):
[1] compiler_4.3.1    tools_4.3.1       rstudioapi_0.15.0 

[r2evans]

I think your issue is not that it does or doesn't join, it's that base R's warning is either bypassed or suppressed, is that right? Unfortunately in R, the units for Date (day) and POSIXt (second) are not automatically adjusted, requiring user interaction to perform accurately. (Fixing this is on my wishlist-for-R.)

The lack of a warning does make me wonder (not quite enough yet to look under the hood at the source) how data.table is implementing a number-like join.

(Similarly, I'd really like data.table to stop warning about difference in "tzone" when comparing just times, since it is not about units, and they compare quite accurately. But I don't want to hijack this thread for that wishlist item :-)

[al-obrien]

I think your issue is not that it does or doesn't join, it's that base R's warning is either bypassed or suppressed, is that right?

That's correct. Its understandable that joins on a POSc and Date column would behave differently but the warning to notify the user on the type difference, in this instance, seems to be bypassed (compared to when using those operators outside of a join context).

[r2evans]

My guess is that something internal "knows" that the underlying join keys are both numeric under the hood, and it just works regardless of other attributes of the numbers. (That's one way I might have approached it.)

[ben-schwen]

This is somewhat related to #6602 and seems to be the same as #1200. Under the hood joins in data.table check if the join keys check are compatible and coerces, if needed (as @r2evans suspected). This can be seen nicely when turning on verbosity options(datatable.verbose=TRUE).

[r2evans]

This question seems to be a better structure for the "issue", suggest it might be useful to either keep this issue and close 1200 as a dupe, or at least update 1200 with a better reprex (either from the SO question it references or from this issue).

Further, do you (@ben-schwen) think it's a good idea and likely that having the data.table-join mechanics check for date/posix attributes to see if this is necessary? It can't have a "zero" effect on join performance, though I don't think it'd be much if even perceivable to mortals.

[ben-schwen]

Yes, we should do some prechecks on compatible types. Ofc, this will never catch everything but it should at least catch common pitfalls (similar as we already do in rbindlist).

[r2evans]

@ben-schwen , I haven't studied the data.table-join source, but I can try taking a look at it ...

Some questions:

• other than this issue, are there other class-checks that have been discussed elsewhere? it makes sense to roll all such prechecks into the same PR
• for Date-vs-POSIXt, two options in order of simplicity and straight-forward-ness:
  1. Warn, coerce the date to Date 00:00:00 (early midnight), include a hint to that in the warning; or
  2. Based on LHS-vs-RHS, try to infer whether it should be 00:00:00 or 24:00:00 - 1e-9; I haven't thought fully about this yet, not sure if there are gotchas

Offhand, do you know exactly where this precheck would go in the code?

[r2evans]

@ben-schwen, here is one possible patch (I'm happy to provide as a PR). It works, but it is doing precisely one bespoke precheck: if both columns to join are in c("Date", "POSIXt", "ITime", "times") (the latter is from chron::) but they are not the same, then it warns and continues with the join.

dt1_posc[dt2, on = .(id, date_time1 <= date_2, date_time2 >= date_1)]
# Warning in bmerge(i, x, leftcols, rightcols, roll, rollends, nomatch, mult,  :
#   Attempting to join column x.date_time1 (POSIXt) with column i.date_2 (Date), they are unlikely to be compatible numbers. Suggest you convert one to the other's class.
# Warning in bmerge(i, x, leftcols, rightcols, roll, rollends, nomatch, mult,  :
#   Attempting to join column x.date_time2 (POSIXt) with column i.date_1 (Date), they are unlikely to be compatible numbers. Suggest you convert one to the other's class.
#       id date_time1 date_time2 infocol
#    <num>     <Date>     <Date>  <char>
# 1:     1 2018-06-01 2018-01-01    <NA>
# 2:     1 2020-11-01 2019-01-01    <NA>
# 3:     2 2010-01-01 2010-01-01    <NA>

With this patch:

--- a/R/bmerge.R
+++ b/R/bmerge.R
@@ -70,6 +70,14 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
       }
       stopf("Incompatible join types: %s (%s) and %s (%s). Factor columns must join to factor or character columns.", xname, xclass, iname, iclass)
     }
+    timeclasses = c("Date", "POSIXt", "ITime", "times")
+    xclass_time = intersect(class(x[[xc]]), timeclasses)
+    iclass_time = intersect(class(i[[ic]]), timeclasses)
+    if (length(xclass_time) > 0L && length(iclass_time) > 0L &&
+          !identical(sort(xclass_time), sort(iclass_time))) {
+      warningf("Attempting to join column %s (%s) with column %s (%s), they are unlikely to be compatible numbers. Suggest you convert one to the other's class.",
+               xname, toString(xclass_time), iname, toString(iclass_time))
+    }
     if (xclass == iclass) {
       if (verbose) catf("%s has same type (%s) as %s. No coercion needed.\n", iname, xclass, xname)
       next

I'm making the assumption that the follow-on checks (using getClass(..)) will find issues if a time-class is joined against something else (that is not number-like). If you can think of other class incompatibilities that must be checked, I wonder if it would be better to define a function (perhaps check_merge_compatibility(class(x), class(i))), though that only really seems to scratch the itch of "declarative programming" and not adding much to efficiency/speed, and reduces the clutter only a little.

It could alternatively stopf instead of warn, I'll leave that decision up to the core devs here.

[ben-schwen]

Looks good. I would only add nanotime to commonly used timeclasses.

stopf seems to hard and would probably break a lot of code.