| cmollekopf |
Notes:
Tested in querytest
| Lint Skipped |
| Unit Tests Skipped |
Some general notes first; Good job, I like the approach =) Using the buckets makes the whole index smaller, which is great. The tradeoff is probably the added complexity of storing start and end date directly in the value and filtering accordingly.
The alternative would have been to just store the samples as key, and then relying on a post filtering step that reads the relevant dates from the entities directly. I don't know what would perform better either (depends on the dataset I suppose), so I'm fine with this approach as well (If you did make any estimations I'd be interested).
| tests/querytest.cpp | ||
|---|---|---|
| 1620 | You'll have to test the live query case (a running live query, to which events are added) as well. So far we never use the indexes in the incremental update case, but always filter directly on the properties (the assumption is that this is cheaper for small updates). I think your code handles the case though with the datastorequery changes. | |
Looks overall good to me. I'll apply the patch locally, and will merge it if not problems surface.
Wait before merging it, storing the dates alongside the reference in the index is a "bad optimization" (changed my mind over the WE):
It was to optimize memory when fetching with a period not aligned with an entire week
So in the general case it's better, but since we use it for a calendar view, we will most likely only fetch events in a whole week (ie: week view and month view), so it should be better to remove them