Skip to content

Commit

Permalink
Forbid compaction of tagged+untagged data for a single component (#8782)
Browse files Browse the repository at this point in the history
This implements the second solution described in
#8760 (comment):
> I knew I was forgetting yet another subtle complication: maintaining
an untagged index is not enough, you can still end up in a situation
where a single Chunk has both untagged and tagged data for a component,
and no index is gonna save you there.
> This is what's happening here (see attached screenshot). if i had to
guess, this is because a runtime blueprint write ends up compacted in a
pre-existing, tagged blueprint chunk, and now the resulting chunk is
both tagged and untagged for that component.
> 
> Once we're done with all the API updates on the SDK side, it shouldn't
ever be possible for a user to end up in that situation *when working in
new recording*, so that end is covered.
> That leaves A) runtime blueprint writes and B) user writes to a
pre-existing, legacy recording. Obviously the correct fix for blueprint
writes is to port all of them to tagged APIs, but A) that will not
happen for 0.22 and B) that doesn't take care of the other problem.
> 
> I see two possible avenues here:
> * Modify the compaction logic so that when tagged data is compacted
with untagged data of the same component, we merge them together and
keep the tags going forward. This should be well-specified with the
current data model, and helps with propagating tags going forward.
> * Modify the compaction logic so that tagged and untagged data of the
same component is never compacted together.


I can confirm this fixes the "changing view origin crashes in debug"
issue.

* Part of #8760
  • Loading branch information
teh-cmc authored Jan 23, 2025
1 parent 1c51788 commit c5925bb
Showing 1 changed file with 33 additions and 1 deletion.
34 changes: 33 additions & 1 deletion crates/store/re_chunk/src/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,35 @@ impl Chunk {
})
}

/// Returns `true` if both chunks share the same descriptors for the components that
/// _they have in common_.
#[inline]
pub fn same_descriptors(&self, rhs: &Self) -> bool {
self.components.values().all(|lhs_per_desc| {
if lhs_per_desc.len() > 1 {
// If it's already in UB land, we might as well give up immediately.
return false;
}

lhs_per_desc.keys().all(|lhs_desc| {
let Some(rhs_per_desc) = rhs.components.get(&lhs_desc.component_name) else {
return true;
};

if rhs_per_desc.len() > 1 {
// If it's already in UB land, we might as well give up immediately.
return false;
}

if let Some(rhs_desc) = rhs_per_desc.keys().next() {
lhs_desc == rhs_desc
} else {
true
}
})
})
}

/// Returns true if two chunks are concatenable.
///
/// To be concatenable, two chunks must:
Expand All @@ -258,7 +287,10 @@ impl Chunk {
/// * Use the same datatypes for the components they have in common.
#[inline]
pub fn concatenable(&self, rhs: &Self) -> bool {
self.same_entity_paths(rhs) && self.same_timelines(rhs) && self.same_datatypes(rhs)
self.same_entity_paths(rhs)
&& self.same_timelines(rhs)
&& self.same_datatypes(rhs)
&& self.same_descriptors(rhs)
}
}

Expand Down

0 comments on commit c5925bb

Please sign in to comment.