r-lib / cachem

Key-value caches for R
https://cachem.r-lib.org
Other
57 stars 16 forks source link

With custom missing keys layered cache never gets objects beyond first layer #26

Open vspinu opened 2 years ago

vspinu commented 2 years ago
dcache <- cachem::cache_disk("/tmp/dircache", missing = NULL)
mcache <- cachem::cache_mem(missing = NULL)
lcache <- cachem::cache_layered(mcache, dcache)

dcache$set("foo", 100)
dcache$get("foo")
#[1] 100
lcache$get("foo")
# NULL

This is because layerd cache is using is.key_missing instead of using the internal missingness value.

An easy fix would be to check for the corresponding missing_ but it requires an indirection. Alternatively export a new function missing or something:

1 file changed, 2 insertions(+), 2 deletions(-)
R/cache-layered.R | 4 ++--

modified   R/cache-layered.R
@@ -34,8 +34,8 @@ cache_layered <- function(..., logfile = NULL) {
     # Search down the caches for the object
     for (i in seq_along(caches)) {
       value <- caches[[i]]$get(key)
-
-      if (!is.key_missing(value)) {
+      missing <- eval_tidy(caches[[i]]$info()$missing)
+      if (!identical(value, missing)) {
         log_(paste0("Get from ", class(caches[[i]])[1], "... hit"))
         # Set the value in any caches where we searched and missed.
         for (j in seq_len(i-1)) {
wch commented 2 years ago

It's hard to have a fully general solution for this, because missing is an expression that's evaluated each time there's a miss, so a get() call could, for example, throw an error every time there's a missing key, or it could increment a counter of misses (and in the proposed change, it would increment the counter unnecessarily), and so on.

Is there a particular reason you prefer NULL over the sentinel value?

vspinu commented 2 years ago

I prefer NULL because of the nil-puning in the code (things like %||%) but it's not a big deal. I already switched to the default sentinel.

An even easier solution would be to add missing argument to cache_layered. Then users could figure out by themselves what to do. The constructor can throw an error if missing keys are not the same. The status quo is not ok. It took me a while to figure out that something wasn't quite right under the hood.

wch commented 2 years ago

I think it makes sense to add a missing arg to cache_layered.