-
Notifications
You must be signed in to change notification settings - Fork 707
re_query recursive clear checks are incorrect #7631
Copy link
Copy link
Open
Labels
🔍 re_queryaffects re_query itselfaffects re_query itself🔩 data modelSorbetSorbet🪳 bugSomething isn't workingSomething isn't working
Description
This code is subtly wrong:
rerun/crates/store/re_query/src/latest_at.rs
Lines 62 to 144 in 87e76f8
| // Query-time clears | |
| // ----------------- | |
| // | |
| // We need to find, at query time, whether there exist a `Clear` component that should | |
| // shadow part or all of the results that we are about to return. | |
| // | |
| // This is a two-step process. | |
| // | |
| // First, we need to find all `Clear` components that could potentially affect the returned | |
| // results, i.e. any `Clear` component on the entity itself, or any recursive `Clear` | |
| // component on any of its recursive parents. | |
| // | |
| // Then, we need to compare the index of each component result with the index of the most | |
| // recent relevant `Clear` component that was found: if there exists a `Clear` component with | |
| // both a _data time_ lesser or equal to the _query time_ and an index greater or equal | |
| // than the indexed of the returned data, then we know for sure that the `Clear` shadows | |
| // the data. | |
| let mut max_clear_index = (TimeInt::MIN, RowId::ZERO); | |
| { | |
| re_tracing::profile_scope!("clears"); | |
| let potential_clears = self.might_require_clearing.read(); | |
| let mut clear_entity_path = entity_path.clone(); | |
| loop { | |
| if !potential_clears.contains(&clear_entity_path) { | |
| // This entity does not contain any `Clear`-related data at all, there's no | |
| // point in running actual queries. | |
| let Some(parent_entity_path) = clear_entity_path.parent() else { | |
| break; | |
| }; | |
| clear_entity_path = parent_entity_path; | |
| continue; | |
| } | |
| let key = CacheKey::new( | |
| clear_entity_path.clone(), | |
| query.timeline(), | |
| ClearIsRecursive::name(), | |
| ); | |
| let cache = Arc::clone( | |
| self.latest_at_per_cache_key | |
| .write() | |
| .entry(key.clone()) | |
| .or_insert_with(|| Arc::new(RwLock::new(LatestAtCache::new(key.clone())))), | |
| ); | |
| let mut cache = cache.write(); | |
| cache.handle_pending_invalidation(); | |
| if let Some(cached) = | |
| cache.latest_at(store, query, &clear_entity_path, ClearIsRecursive::name()) | |
| { | |
| let found_recursive_clear = cached | |
| .component_mono::<ClearIsRecursive>() | |
| .and_then(Result::ok) | |
| == Some(ClearIsRecursive(true.into())); | |
| // When checking the entity itself, any kind of `Clear` component | |
| // (i.e. recursive or not) will do. | |
| // | |
| // For (recursive) parents, we need to deserialize the data to make sure the | |
| // recursive flag is set. | |
| #[allow(clippy::collapsible_if)] // readability | |
| if clear_entity_path == *entity_path || found_recursive_clear { | |
| if let Some(index) = cached.index(&query.timeline()) { | |
| if compare_indices(index, max_clear_index) | |
| == std::cmp::Ordering::Greater | |
| { | |
| max_clear_index = index; | |
| } | |
| } | |
| } | |
| } | |
| let Some(parent_entity_path) = clear_entity_path.parent() else { | |
| break; | |
| }; | |
| clear_entity_path = parent_entity_path; | |
| } | |
| } |
It should be doing an unbounded backwards latest-at walk for as long as the boolean flag of ClearIsRecursive is false.
Clearly we won't be doing that, but it is semantically wrong as it stands.
- See Proposal: split
ClearIsRecursiveintoClearFlatandClearRecursive#7630 for more context.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
🔍 re_queryaffects re_query itselfaffects re_query itself🔩 data modelSorbetSorbet🪳 bugSomething isn't workingSomething isn't working