Fix missing data for "bridged" range queries #6177
Conversation
teh-cmc
added
🪳 bug
| let hole_start = time_range.max(); | ||
| let hole_end = | ||
| TimeInt::new_temporal(query.range().min().as_i64().saturating_sub(1)); | ||
| if hole_start < hole_end { |
There was a problem hiding this comment.
I'm having a hard time mentally matching this to the description, picture above.
This description talks about an ordering between queries. This implementation seems to be in terms of a relationship between a new query and the current state of the cache.
It looks like what's going on is that is that any time a new query comes before or after the existing range cache, then we clear the cache to avoid ever ending up with a hole in the first place?
There was a problem hiding this comment.
It looks like what's going on is that is that any time a new query comes before or after the existing range cache, then we clear the cache to avoid ever ending up with a hole in the first place?
Yes, we prematurely clear the cache anytime two non-contiguous queries (as defined by the checks below) come in one after another, to keep it simple.
crates/re_query/src/range/query.rs
Outdated
| if let Some((data_time, _, _)) = store.latest_at( | ||
| &LatestAtQuery::new(query.timeline(), hole_end), | ||
| entity_path, | ||
| component_name, | ||
| &[component_name], | ||
| ) { | ||
| if data_time != hole_start { |
There was a problem hiding this comment.
Again a little confused by what's happening here. I suspect this is to avoid invalidating in cases where the hole otherwise contains no data.
However, shouldn't it be data_time > hole_start.
Any situation where data_time < hole_start doesn't seem like it should require an invalidation since the values pulled in by the new query should be correct?
There was a problem hiding this comment.
Yeah you're right, nice catch
A cheap fix for #5686, both in terms of added code and runtime performance, as it only kicks in in very particular circumstances.
The test added in this PR explains the situation better than words ever could.
The proper solution here would to implement proper bucketing in the range cache (#4810), but that's a much bigger change that I don't want to get into until after the upcoming drastic changes to the datastore.
Checklist
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerTo run all checks from
main, comment on the PR with@rerun-bot full-check.