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

Fix row ordering flakiness when using clear APIs #3288

Merged
merged 3 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions crates/re_data_store/src/store_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,30 +123,41 @@ impl EntityDb {
fn add_path_op(&mut self, row_id: RowId, time_point: &TimePoint, path_op: &PathOp) {
let cleared_paths = self.tree.add_path_op(row_id, time_point, path_op);

// NOTE: Btree! We need a stable ordering here!
let mut cells = BTreeMap::<EntityPath, Vec<DataCell>>::default();
for component_path in cleared_paths {
if let Some(data_type) = self
.data_store
.lookup_datatype(&component_path.component_name)
{
// Create and insert an empty component into the arrow store
// TODO(jleibs): Faster empty-array creation
let cell =
DataCell::from_arrow_empty(component_path.component_name, data_type.clone());
let cells = cells
.entry(component_path.entity_path.clone())
.or_insert_with(Vec::new);

// NOTE(cmc): The fact that this inserts data to multiple entity paths using a
// single `RowId` is… interesting. Keep it in mind.
let row = DataRow::from_cells1(
row_id,
component_path.entity_path.clone(),
time_point.clone(),
cell.num_instances(),
cell,
);
self.data_store.insert_row(&row).ok();
// Also update the tree with the clear-event
cells.push(DataCell::from_arrow_empty(
component_path.component_name,
data_type.clone(),
));

// Update the tree with the clear-event.
self.tree.add_data_msg(time_point, &component_path);
}
}

// Create and insert empty components into the arrow store.
let mut row_id = row_id;
for (ent_path, cells) in cells {
// NOTE: It is important we insert all those empty components using a single row (id)!
// 1. It'll be much more efficient when querying that data back.
// 2. Otherwise we will end up with a flaky row ordering, as we have no way to tie-break
// these rows! This flaky ordering will in turn leak through the public
// API (e.g. range queries)!!
let row = DataRow::from_cells(row_id, time_point.clone(), ent_path, 0, cells);
self.data_store.insert_row(&row).ok();

// Don't reuse the same row ID for the next entity!
row_id = row_id.next();
}
}

pub fn purge(
Expand Down
9 changes: 9 additions & 0 deletions crates/re_log_types/src/data_row.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,15 @@ impl RowId {
pub fn random() -> Self {
Self(re_tuid::Tuid::random())
}

/// Returns the next logical `RowId`.
///
/// Beware: wrong usage can easily lead to conflicts.
/// Prefer [`RowId::random`] when unsure.
#[inline]
pub fn next(&self) -> Self {
Self(self.0.next())
}
}

impl SizeBytes for RowId {
Expand Down
16 changes: 16 additions & 0 deletions crates/re_tuid/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,22 @@ impl Tuid {
((self.time_ns as u128) << 64) | (self.inc as u128)
}

/// Returns the next logical `Tuid`.
///
/// Wraps the monotonically increasing back to zero on overflow.
///
/// Beware: wrong usage can easily lead to conflicts.
/// Prefer [`Tuid::random`] when unsure.
#[inline]
pub fn next(&self) -> Self {
let Self { time_ns, inc } = *self;

Self {
time_ns,
inc: inc.wrapping_add(1),
}
}

#[inline]
pub fn nanoseconds_since_epoch(&self) -> u64 {
self.time_ns
Expand Down