Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache is not invalidated for relative date/time queries #223

Open
yantar92 opened this issue Jun 20, 2021 · 9 comments
Open

Cache is not invalidated for relative date/time queries #223

yantar92 opened this issue Jun 20, 2021 · 9 comments
Assignees
Labels

Comments

@yantar92
Copy link
Contributor

Consider a simple query like (ts-active :to today) and the following headline:

* test
SCHEDULED: <2021-06-21 Mon>

If today is Sunday, 20th, the query will correctly return nil. However, if the file containing the headline is unchanged, and we run the query again next day (Monday, 21st), the return value will still be nil. It should not be.

@alphapapa
Copy link
Owner

Thanks, that's a good catch.

For my own understanding: It's happening because the today argument is replaced in the --query-predicate function, which only applies inside the byte-compiled predicate, which is not part of the cache key (IIRC, there are some issues with hashing lambdas that makes them unsuitable for keys, at least before 27.1). Probably, the argument replacing should be done in the query normalizers. That seems like too large of a change for a bugfix release, so I'll do this in 0.6.

@alphapapa alphapapa self-assigned this Jun 20, 2021
@alphapapa alphapapa added the bug label Jun 20, 2021
@alphapapa alphapapa added this to the 0.6 milestone Jun 20, 2021
@yantar92
Copy link
Contributor Author

yantar92 commented Jun 21, 2021 via email

@alphapapa
Copy link
Owner

Consider a custom predicate matching all the tasks scheduled today, but no later than 1 hour after current time. If we simply put current date+time as one of the arguments in the normalised query, the cache will always be invalid.

IIUC, the cache should never used used for such a query--or more specifically, the cache would only be valid for up to one second, anyway, depending on the timestamp resolution--so running a new query every time would be correct.

@alphapapa
Copy link
Owner

@yantar92 That required a lot more work than I expected, due to moving some of the argument processing into query normalization and changing some internal macros (as well as unexpected, inexplicable test failures only on CI, but I digress). All the tests pass (including new ones I added to check for this problem). Please let me know how it works for you.

Regarding the custom predicate issue: Again, if I understand that specific example properly, I think that won't be a problem. But if I misunderstood it, or if there are other issues with custom predicates and cache invalidation, please open another issue and we can work on it.

Thanks for your help. I'm surprised it took this long for this issue to be noticed.

@alphapapa
Copy link
Owner

P.S. If you haven't seen #143, please take a look, as I'd like your input on the proposed changes.

@yantar92
Copy link
Contributor Author

yantar92 commented Jul 2, 2021 via email

@alphapapa
Copy link
Owner

I suspect that such kinds of queries are more common for agenda-like views. However, caching never works when refreshing agenda on master without #203. That's why the issue was never captured.

I'm not exactly sure what you mean by "when refreshing agenda." Do you mean your own, agenda-like custom query views? Do you mean that, without merging #203, those queries' results are never cached? Or do you mean that something else in your config modifies the text properties and causes the org-ql cache to be invalidated?

(I'm reopening this issue to ensure that I'll see your response; notifications for closed issues tend to get lost in my notifications list.)

@alphapapa alphapapa reopened this Jul 2, 2021
@alphapapa alphapapa modified the milestones: 0.6, 0.7 Sep 22, 2021
@alphapapa
Copy link
Owner

Retargeting this for 0.7. 0.6 has been delayed for too long.

@alphapapa
Copy link
Owner

AFAICT this issue is fixed, but I'll take another look for 0.8.

@alphapapa alphapapa modified the milestones: 0.7, 0.8 Mar 10, 2023
@alphapapa alphapapa removed this from the 0.8 milestone Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants