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

custom sort functions pollute cache #186

Closed
natask opened this issue Feb 3, 2021 · 3 comments
Closed

custom sort functions pollute cache #186

natask opened this issue Feb 3, 2021 · 3 comments
Assignees
Labels
Milestone

Comments

@natask
Copy link
Contributor

natask commented Feb 3, 2021

the first call to org-ql-select with a custom sort function works correctly. however subsequent calls return missing list of results. It also affects calls that hit the cache.

This happens because (sort) modifies the list. the cache becomes polluted. future calls query the same list from the cache. but because the list is malformed, subsequent applications of the sort function return malformed resulted.

fix by replacing (sort) with (-sort), which copies the list before sorting.

natask added a commit to natask/org-ql that referenced this issue Feb 3, 2021
@alphapapa
Copy link
Owner

Thanks for reporting. I don't have time to verify this at the moment, but, assuming your analysis is correct, what you propose may not be the best solution, because it would impose a performance penalty, especially with large numbers of results.

@natask
Copy link
Contributor Author

natask commented Feb 22, 2021

I neglected to mention that org ql's default sort functions are implemented using (-sort).

org-ql/org-ql.el

Line 1893 in 208e103

(-sort (sorter pred) items)))

Although I share your reservations, aside from solving the bug, the performance is within org ql's current implementation.

@alphapapa
Copy link
Owner

You're right, of course.

I've verified the bug. I don't quite understand how it's happening, because AFAICT sort is being called on a copy of the results produced by -flatten-n, but destructive functions can be tricky like that (maybe those Clojure folks are onto something...).

Thanks for your patience. I'll apply this in the next bug-fix release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants