From 15ee2b91f40be3750ceb896dd5a6b77ce2c3fcdc Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Mon, 7 Oct 2024 16:24:54 +0200 Subject: [PATCH] Dataframe API: remove control columns from public surface (#7612) * Prevents ~hard~annoying-to-solve cross-feature interactions in the new API implementation. * Fixes #7599 --- crates/store/re_chunk_store/src/dataframe.rs | 127 +-------------- crates/store/re_chunk_store/src/lib.rs | 8 +- crates/store/re_dataframe/src/latest_at.rs | 48 ++---- crates/store/re_dataframe/src/range.rs | 10 +- crates/store/re_dataframe2/src/query.rs | 147 ++---------------- .../src/dataframe_ui.rs | 33 +--- .../src/display_record_batch.rs | 116 +------------- .../src/view_query/blueprint.rs | 8 +- .../src/view_query/ui.rs | 30 ---- rerun_py/src/dataframe.rs | 65 +------- 10 files changed, 65 insertions(+), 527 deletions(-) diff --git a/crates/store/re_chunk_store/src/dataframe.rs b/crates/store/re_chunk_store/src/dataframe.rs index 3500954c917c..90bd9a5de6f7 100644 --- a/crates/store/re_chunk_store/src/dataframe.rs +++ b/crates/store/re_chunk_store/src/dataframe.rs @@ -12,7 +12,7 @@ use itertools::Itertools as _; use re_chunk::{LatestAtQuery, TimelineName}; use re_log_types::{EntityPath, TimeInt, Timeline}; use re_log_types::{EntityPathFilter, ResolvedTimeRange}; -use re_types_core::{ArchetypeName, ComponentName, Loggable as _}; +use re_types_core::{ArchetypeName, ComponentName}; use crate::ChunkStore; @@ -70,12 +70,10 @@ pub enum JoinEncoding { // Describes any kind of column. // // See: -// * [`ControlColumnDescriptor`] // * [`TimeColumnDescriptor`] // * [`ComponentColumnDescriptor`] #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum ColumnDescriptor { - Control(ControlColumnDescriptor), Time(TimeColumnDescriptor), Component(ComponentColumnDescriptor), } @@ -84,7 +82,7 @@ impl ColumnDescriptor { #[inline] pub fn entity_path(&self) -> Option<&EntityPath> { match self { - Self::Control(_) | Self::Time(_) => None, + Self::Time(_) => None, Self::Component(descr) => Some(&descr.entity_path), } } @@ -92,7 +90,6 @@ impl ColumnDescriptor { #[inline] pub fn datatype(&self) -> ArrowDatatype { match self { - Self::Control(descr) => descr.datatype.clone(), Self::Time(descr) => descr.datatype.clone(), Self::Component(descr) => descr.returned_datatype(), } @@ -101,7 +98,6 @@ impl ColumnDescriptor { #[inline] pub fn to_arrow_field(&self) -> ArrowField { match self { - Self::Control(descr) => descr.to_arrow_field(), Self::Time(descr) => descr.to_arrow_field(), Self::Component(descr) => descr.to_arrow_field(), } @@ -110,59 +106,12 @@ impl ColumnDescriptor { #[inline] pub fn short_name(&self) -> String { match self { - Self::Control(descr) => descr.component_name.short_name().to_owned(), Self::Time(descr) => descr.timeline.name().to_string(), Self::Component(descr) => descr.component_name.short_name().to_owned(), } } } -/// Describes a column used to control Rerun's behavior, such as `RowId`. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct ControlColumnDescriptor { - /// Semantic name associated with this data. - /// - /// Example: `RowId::name()`. - pub component_name: ComponentName, - - /// The Arrow datatype of the column. - pub datatype: ArrowDatatype, -} - -impl PartialOrd for ControlColumnDescriptor { - #[inline] - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for ControlColumnDescriptor { - #[inline] - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - let Self { - component_name, - datatype: _, - } = self; - component_name.cmp(&other.component_name) - } -} - -impl ControlColumnDescriptor { - #[inline] - pub fn to_arrow_field(&self) -> ArrowField { - let Self { - component_name, - datatype, - } = self; - - ArrowField::new( - component_name.to_string(), - datatype.clone(), - false, /* nullable */ - ) - } -} - /// Describes a time column, such as `log_time`. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct TimeColumnDescriptor { @@ -411,7 +360,6 @@ impl ComponentColumnDescriptor { /// Describes a column selection to return as part of a query. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum ColumnSelector { - Control(ControlColumnSelector), Time(TimeColumnSelector), Component(ComponentColumnSelector), //TODO(jleibs): Add support for archetype-based component selection. @@ -422,20 +370,12 @@ impl From for ColumnSelector { #[inline] fn from(desc: ColumnDescriptor) -> Self { match desc { - ColumnDescriptor::Control(desc) => Self::Control(desc.into()), ColumnDescriptor::Time(desc) => Self::Time(desc.into()), ColumnDescriptor::Component(desc) => Self::Component(desc.into()), } } } -impl From for ColumnSelector { - #[inline] - fn from(desc: ControlColumnSelector) -> Self { - Self::Control(desc) - } -} - impl From for ColumnSelector { #[inline] fn from(desc: TimeColumnSelector) -> Self { @@ -450,37 +390,6 @@ impl From for ColumnSelector { } } -/// Select a control column. -/// -/// The only control column currently supported is `rerun.components.RowId`. -// -// TODO(cmc): `RowId` shouldnt be a control column at this point, it should be yet another index. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] -pub struct ControlColumnSelector { - /// Name of the control column. - // - // TODO(cmc): this should be `component_name`. - pub component: ComponentName, -} - -impl ControlColumnSelector { - #[inline] - pub fn row_id() -> Self { - Self { - component: RowId::name(), - } - } -} - -impl From for ControlColumnSelector { - #[inline] - fn from(desc: ControlColumnDescriptor) -> Self { - Self { - component: desc.component_name, - } - } -} - /// Select a time column. // // TODO(cmc): This shouldn't be specific to time, this should be an `IndexColumnSelector` or smth. @@ -917,11 +826,6 @@ impl ChunkStore { pub fn schema(&self) -> Vec { re_tracing::profile_function!(); - let controls = std::iter::once(ColumnDescriptor::Control(ControlColumnDescriptor { - component_name: RowId::name(), - datatype: RowId::arrow_datatype(), - })); - let timelines = self.all_timelines().into_iter().map(|timeline| { ColumnDescriptor::Time(TimeColumnDescriptor { timeline, @@ -995,26 +899,7 @@ impl ChunkStore { .chain(temporal_components) .collect::>(); - controls.chain(timelines).chain(components).collect() - } - - /// Given a [`ControlColumnSelector`], returns the corresponding [`ControlColumnDescriptor`]. - #[allow(clippy::unused_self)] - pub fn resolve_control_selector( - &self, - selector: &ControlColumnSelector, - ) -> ControlColumnDescriptor { - if selector.component == RowId::name() { - ControlColumnDescriptor { - component_name: selector.component, - datatype: RowId::arrow_datatype(), - } - } else { - ControlColumnDescriptor { - component_name: selector.component, - datatype: ArrowDatatype::Null, - } - } + timelines.chain(components).collect() } /// Given a [`TimeColumnSelector`], returns the corresponding [`TimeColumnDescriptor`]. @@ -1076,12 +961,10 @@ impl ChunkStore { .map(|selector| { let selector = selector.into(); match selector { - ColumnSelector::Control(selector) => { - ColumnDescriptor::Control(self.resolve_control_selector(&selector)) - } ColumnSelector::Time(selector) => { ColumnDescriptor::Time(self.resolve_time_selector(&selector)) } + ColumnSelector::Component(selector) => { ColumnDescriptor::Component(self.resolve_component_selector(&selector)) } @@ -1171,7 +1054,7 @@ impl ChunkStore { self.schema() .into_iter() .filter(|column| match column { - ColumnDescriptor::Control(_) | ColumnDescriptor::Time(_) => true, + ColumnDescriptor::Time(_) => true, ColumnDescriptor::Component(column) => view_contents .get(&column.entity_path) .map_or(false, |components| { diff --git a/crates/store/re_chunk_store/src/lib.rs b/crates/store/re_chunk_store/src/lib.rs index ddeb2e7bb87e..50d3e07aa05a 100644 --- a/crates/store/re_chunk_store/src/lib.rs +++ b/crates/store/re_chunk_store/src/lib.rs @@ -25,10 +25,10 @@ mod subscribers; mod writes; pub use self::dataframe::{ - ColumnDescriptor, ColumnSelector, ComponentColumnDescriptor, ComponentColumnSelector, - ControlColumnDescriptor, ControlColumnSelector, Index, IndexRange, IndexValue, JoinEncoding, - LatestAtQueryExpression, QueryExpression, QueryExpression2, RangeQueryExpression, - SparseFillStrategy, TimeColumnDescriptor, TimeColumnSelector, ViewContentsSelector, + ColumnDescriptor, ColumnSelector, ComponentColumnDescriptor, ComponentColumnSelector, Index, + IndexRange, IndexValue, JoinEncoding, LatestAtQueryExpression, QueryExpression, + QueryExpression2, RangeQueryExpression, SparseFillStrategy, TimeColumnDescriptor, + TimeColumnSelector, ViewContentsSelector, }; pub use self::events::{ChunkStoreDiff, ChunkStoreDiffKind, ChunkStoreEvent}; pub use self::gc::{GarbageCollectionOptions, GarbageCollectionTarget}; diff --git a/crates/store/re_dataframe/src/latest_at.rs b/crates/store/re_dataframe/src/latest_at.rs index ad7bb6f103a6..afbce277db9b 100644 --- a/crates/store/re_dataframe/src/latest_at.rs +++ b/crates/store/re_dataframe/src/latest_at.rs @@ -77,9 +77,6 @@ impl LatestAtQueryHandle<'_> { .schema_for_query(&self.query.clone().into()) }) .into_iter() - // NOTE: We drop `RowId`, as it doesn't make any sense in a compound row like the - // one we are returning. - .filter(|descr| !matches!(descr, ColumnDescriptor::Control(_))) .collect() }; @@ -125,7 +122,7 @@ impl LatestAtQueryHandle<'_> { .map(|chunk| (col, chunk)) } - _ => None, + ColumnDescriptor::Time(_) => None, }) .collect() }; @@ -138,7 +135,7 @@ impl LatestAtQueryHandle<'_> { .iter() .filter_map(|descr| match descr { ColumnDescriptor::Time(descr) => Some(descr.timeline), - _ => None, + ColumnDescriptor::Component(_) => None, }) .collect_vec(); @@ -169,22 +166,13 @@ impl LatestAtQueryHandle<'_> { columns .iter() - .filter_map(|col| match col { - ColumnDescriptor::Control(_) => { - if cfg!(debug_assertions) { - unreachable!("filtered out during schema computation"); - } else { - // NOTE: Technically cannot ever happen, but I'd rather that than an uwnrap. - None - } - } - + .map(|col| match col { ColumnDescriptor::Time(descr) => { let time_column = max_time_per_timeline .remove(&descr.timeline) .and_then(|(_, chunk)| chunk.timelines().get(&descr.timeline).cloned()); - Some(time_column.map_or_else( + time_column.map_or_else( || { arrow2::array::new_null_array( descr.datatype.clone(), @@ -192,23 +180,21 @@ impl LatestAtQueryHandle<'_> { ) }, |time_column| time_column.times_array().to_boxed(), - )) + ) } - ColumnDescriptor::Component(descr) => Some( - all_units - .get(col) - .and_then(|chunk| chunk.components().get(&descr.component_name)) - .map_or_else( - || { - arrow2::array::new_null_array( - descr.returned_datatype(), - null_array_length, - ) - }, - |list_array| list_array.to_boxed(), - ), - ), + ColumnDescriptor::Component(descr) => all_units + .get(col) + .and_then(|chunk| chunk.components().get(&descr.component_name)) + .map_or_else( + || { + arrow2::array::new_null_array( + descr.returned_datatype(), + null_array_length, + ) + }, + |list_array| list_array.to_boxed(), + ), }) .collect_vec() }; diff --git a/crates/store/re_dataframe/src/range.rs b/crates/store/re_dataframe/src/range.rs index a6940a787f49..977c691702e1 100644 --- a/crates/store/re_dataframe/src/range.rs +++ b/crates/store/re_dataframe/src/range.rs @@ -350,7 +350,7 @@ impl RangeQueryHandle<'_> { JoinEncoding::OverlappingSlice => None, JoinEncoding::DictionaryEncode => Some(descr), }, - _ => None, + ColumnDescriptor::Time(_) => None, }) .filter_map(|descr| { let arrays = pov_time_column @@ -426,7 +426,7 @@ impl RangeQueryHandle<'_> { } JoinEncoding::DictionaryEncode => None, }, - _ => None, + ColumnDescriptor::Time(_) => None, }) .map(|descr| { let arrays = pov_time_column @@ -472,8 +472,6 @@ impl RangeQueryHandle<'_> { columns .iter() .map(|descr| match descr { - ColumnDescriptor::Control(_descr) => pov_chunk.row_ids_array().to_boxed(), - ColumnDescriptor::Time(descr) => { let time_column = pov_chunk.timelines().get(&descr.timeline).cloned(); time_column.map_or_else( @@ -530,10 +528,6 @@ impl RangeQueryHandle<'_> { let packed_arrays = columns .iter() .map(|descr| match descr { - ColumnDescriptor::Control(_descr) => { - pov_chunk.row_ids_array().sliced(row, 1).to_boxed() - } - ColumnDescriptor::Time(descr) => { let time_column = pov_chunk.timelines().get(&descr.timeline); time_column.map_or_else( diff --git a/crates/store/re_dataframe2/src/query.rs b/crates/store/re_dataframe2/src/query.rs index 13b1ba4586bb..3441c348d7b7 100644 --- a/crates/store/re_dataframe2/src/query.rs +++ b/crates/store/re_dataframe2/src/query.rs @@ -13,8 +13,8 @@ use nohash_hasher::IntMap; use re_chunk::{Chunk, RangeQuery, RowId, TimeInt, Timeline, UnitChunkShared}; use re_chunk_store::{ ColumnDescriptor, ColumnSelector, ComponentColumnDescriptor, ComponentColumnSelector, - ControlColumnDescriptor, ControlColumnSelector, IndexValue, JoinEncoding, QueryExpression2, - SparseFillStrategy, TimeColumnDescriptor, TimeColumnSelector, + IndexValue, JoinEncoding, QueryExpression2, SparseFillStrategy, TimeColumnDescriptor, + TimeColumnSelector, }; use re_log_types::ResolvedTimeRange; @@ -152,39 +152,6 @@ impl QueryHandle<'_> { .iter() .map(|column| { match column { - ColumnSelector::Control(selected_column) => { - let ControlColumnSelector { - component: selected_component_name, - } = selected_column; - - view_contents - .iter() - .enumerate() - .filter_map(|(idx, view_column)| match view_column { - ColumnDescriptor::Control(view_descr) => { - Some((idx, view_descr)) - } - _ => None, - }) - .find(|(_idx, view_descr)| { - view_descr.component_name == *selected_component_name - }) - .map_or_else( - || { - ( - usize::MAX, - ColumnDescriptor::Control(ControlColumnDescriptor { - component_name: *selected_component_name, - datatype: arrow2::datatypes::DataType::Null, - }), - ) - }, - |(idx, view_descr)| { - (idx, ColumnDescriptor::Control(view_descr.clone())) - }, - ) - } - ColumnSelector::Time(selected_column) => { let TimeColumnSelector { timeline: selected_timeline, @@ -195,7 +162,7 @@ impl QueryHandle<'_> { .enumerate() .filter_map(|(idx, view_column)| match view_column { ColumnDescriptor::Time(view_descr) => Some((idx, view_descr)), - _ => None, + ColumnDescriptor::Component(_) => None, }) .find(|(_idx, view_descr)| { *view_descr.timeline.name() == *selected_timeline @@ -234,7 +201,7 @@ impl QueryHandle<'_> { ColumnDescriptor::Component(view_descr) => { Some((idx, view_descr)) } - _ => None, + ColumnDescriptor::Time(_) => None, }) .find(|(_idx, view_descr)| { view_descr.entity_path == *selected_entity_path @@ -301,7 +268,7 @@ impl QueryHandle<'_> { .iter() .enumerate() .map(|(idx, selected_column)| match selected_column { - ColumnDescriptor::Control(_) | ColumnDescriptor::Time(_) => Vec::new(), + ColumnDescriptor::Time(_) => Vec::new(), ColumnDescriptor::Component(column) => { // NOTE: Keep in mind that the range APIs natively make sure that we will @@ -758,7 +725,7 @@ impl QueryHandle<'_> { StreamingJoinState::Retrofilled(unit) => { let component_name = state.view_contents.get(view_idx).and_then(|col| match col { ColumnDescriptor::Component(descr) => Some(descr.component_name), - _ => None, + ColumnDescriptor::Time(_) => None, })?; unit.components().get(&component_name).map(|list_array| list_array.to_boxed()) } @@ -783,17 +750,6 @@ impl QueryHandle<'_> { .selected_contents .iter() .map(|(view_idx, column)| match column { - ColumnDescriptor::Control(descr) => { - if descr.component_name == "rerun.controls.RowId" { - cur_most_recent_row - .chunk - .row_ids_array() - .sliced(cur_most_recent_row.cursor as usize, 1) - } else { - arrow2::array::new_null_array(column.datatype(), 1) - } - } - ColumnDescriptor::Time(descr) => { max_value_per_index.get(&descr.timeline).map_or_else( || arrow2::array::new_null_array(column.datatype(), 1), @@ -936,10 +892,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = unindent::unindent( "\ [ @@ -981,10 +934,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = unindent::unindent( "\ [ @@ -1026,10 +976,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = unindent::unindent( "\ [ @@ -1077,10 +1024,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = unindent::unindent( "\ [ @@ -1130,10 +1074,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = "[]"; similar_asserts::assert_eq!(expected, got); @@ -1156,10 +1097,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = "[]"; similar_asserts::assert_eq!(expected, got); @@ -1182,10 +1120,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = unindent::unindent( "\ [ @@ -1218,10 +1153,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = unindent::unindent( "\ [ @@ -1272,10 +1204,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = "[]"; similar_asserts::assert_eq!(expected, got); @@ -1308,10 +1237,7 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = unindent::unindent( "\ [ @@ -1357,51 +1283,12 @@ mod tests { ); eprintln!("{dataframe}"); - let got = format!( - "{:#?}", - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); + let got = format!("{:#?}", dataframe.data.iter().collect_vec()); let expected = "[]"; similar_asserts::assert_eq!(expected, got); } - // only controls - { - let mut query = QueryExpression2::new(timeline); - query.selection = Some(vec![ - ColumnSelector::Control(ControlColumnSelector { - component: "rerun.controls.RowId".into(), - }), - ColumnSelector::Control(ControlColumnSelector { - component: "AControlColumnThatDoesntExist".into(), - }), - ]); - eprintln!("{query:#?}:"); - - let query_handle = query_engine.query(query.clone()); - let dataframe = concatenate_record_batches( - query_handle.schema().clone(), - &query_handle.into_batch_iter().collect_vec(), - ); - eprintln!("{dataframe}"); - - let got = format!( - "{:#?}", - // NOTE: comparing the rowids themselves is gonna be way too annoying. - dataframe.data.iter().skip(1 /* RowId */).collect_vec() - ); - let expected = unindent::unindent( - "\ - [ - NullArray(8), - ]\ - ", - ); - - similar_asserts::assert_eq!(expected, got); - } - // only indices (+ duplication) { let mut query = QueryExpression2::new(timeline); diff --git a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs index 370005a1bc90..3399d84f99a0 100644 --- a/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/dataframe_ui.rs @@ -6,10 +6,10 @@ use egui::NumExt as _; use itertools::Itertools; use re_chunk_store::external::re_chunk::ArrowArray; -use re_chunk_store::{ColumnDescriptor, LatestAtQuery, RowId}; +use re_chunk_store::{ColumnDescriptor, LatestAtQuery}; use re_dataframe2::QueryHandle; use re_log_types::{EntityPath, TimeInt, Timeline, TimelineName}; -use re_types_core::{ComponentName, Loggable as _}; +use re_types_core::ComponentName; use re_ui::UiExt as _; use re_viewer_context::ViewerContext; @@ -80,7 +80,7 @@ pub(crate) fn dataframe_ui( let num_sticky_cols = selected_columns .iter() - .take_while(|cd| matches!(cd, ColumnDescriptor::Control(_) | ColumnDescriptor::Time(_))) + .take_while(|cd| matches!(cd, ColumnDescriptor::Time(_))) .count(); egui::Frame::none().inner_margin(5.0).show(ui, |ui| { @@ -135,9 +135,6 @@ struct RowsDisplayData { /// The index of the time column corresponding to the query timeline. query_time_column_index: Option, - - /// The index of the time column corresponding the row IDs. - row_id_column_index: Option, } impl RowsDisplayData { @@ -169,18 +166,7 @@ impl RowsDisplayData { ColumnDescriptor::Time(time_column_desc) => { &time_column_desc.timeline == query_timeline } - _ => false, - }) - .map(|(pos, _)| pos); - - // find the row id column - let row_id_column_index = selected_columns - .iter() - .find_position(|desc| match desc { - ColumnDescriptor::Control(control_column_desc) => { - control_column_desc.component_name == RowId::name() - } - _ => false, + ColumnDescriptor::Component(_) => false, }) .map(|(pos, _)| pos); @@ -188,7 +174,6 @@ impl RowsDisplayData { display_record_batches, batch_ref_from_row, query_time_column_index, - row_id_column_index, }) } } @@ -270,12 +255,12 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { // if this column can actually be hidden, then that's the corresponding action let hide_action = match column { - ColumnDescriptor::Control(_) => None, ColumnDescriptor::Time(desc) => (desc.timeline != self.query_handle.query().filtered_index) .then(|| HideColumnAction::HideTimeColumn { timeline_name: *desc.timeline.name(), }), + ColumnDescriptor::Component(desc) => { Some(HideColumnAction::HideComponentColumn { entity_path: desc.entity_path.clone(), @@ -348,13 +333,6 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { .unwrap_or(TimeInt::MAX); let latest_at_query = LatestAtQuery::new(self.query_handle.query().filtered_index, timestamp); - let row_id = display_data - .row_id_column_index - .and_then(|col_idx| { - display_data.display_record_batches[batch_idx].columns()[col_idx] - .try_decode_row_id(batch_row_idx) - }) - .unwrap_or(RowId::ZERO); if ui.is_sizing_pass() { ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); @@ -393,7 +371,6 @@ impl<'a> egui_table::TableDelegate for DataframeTableDelegate<'a> { column.data_ui( self.ctx, ui, - row_id, &latest_at_query, batch_row_idx, instance_index, diff --git a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs index 4c1a92468919..a81def252f97 100644 --- a/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs +++ b/crates/viewer/re_space_view_dataframe/src/display_record_batch.rs @@ -6,23 +6,20 @@ use thiserror::Error; use re_chunk_store::external::arrow2::{ array::{ Array as ArrowArray, DictionaryArray as ArrowDictionaryArray, ListArray as ArrowListArray, - PrimitiveArray as ArrowPrimitiveArray, StructArray as ArrowStructArray, + PrimitiveArray as ArrowPrimitiveArray, }, datatypes::DataType, datatypes::DataType as ArrowDataType, }; -use re_chunk_store::{ColumnDescriptor, ComponentColumnDescriptor, LatestAtQuery, RowId}; -use re_log_types::{EntityPath, TimeInt, TimeType, Timeline}; +use re_chunk_store::{ColumnDescriptor, ComponentColumnDescriptor, LatestAtQuery}; +use re_log_types::{EntityPath, TimeInt, Timeline}; use re_types::external::arrow2::datatypes::IntegerType; -use re_types_core::{ComponentName, Loggable as _}; +use re_types_core::ComponentName; use re_ui::UiExt; use re_viewer_context::{UiLayout, ViewerContext}; #[derive(Error, Debug)] pub(crate) enum DisplayRecordBatchError { - #[error("Unknown control column: {0}")] - UnknownControlColumn(String), - #[error("Unexpected column data type for timeline '{0}': {1:?}")] UnexpectedTimeColumnDataType(String, ArrowDataType), @@ -117,7 +114,6 @@ impl ComponentData { &self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - row_id: RowId, latest_at_query: &LatestAtQuery, entity_path: &EntityPath, component_name: ComponentName, @@ -157,7 +153,7 @@ impl ComponentData { ctx.recording(), entity_path, component_name, - Some(row_id), + None, &*data_to_display, ); } else { @@ -169,10 +165,6 @@ impl ComponentData { /// A single column of data in a record batch. #[derive(Debug)] pub(crate) enum DisplayColumn { - RowId { - row_id_times: ArrowPrimitiveArray, - row_id_counters: ArrowPrimitiveArray, - }, Timeline { timeline: Timeline, time_data: ArrowPrimitiveArray, @@ -191,41 +183,6 @@ impl DisplayColumn { column_data: &Box, ) -> Result { match column_descriptor { - ColumnDescriptor::Control(desc) => { - if desc.component_name == RowId::name() { - let row_ids = column_data - .as_any() - .downcast_ref::() - .expect("expected format for RowId, failure is a bug in re_dataframe"); - let [times, counters] = row_ids.values() else { - panic!("RowIds are corrupt -- this should be impossible (sanity checked)"); - }; - - #[allow(clippy::unwrap_used)] - let row_id_times = times - .as_any() - .downcast_ref::>() - .expect("expected format for RowId, failure is a bug in re_dataframe") - .clone(); - - #[allow(clippy::unwrap_used)] - let row_id_counters = counters - .as_any() - .downcast_ref::>() - .expect("expected format for RowId, failure is a bug in re_dataframe") - .clone(); - - Ok(Self::RowId { - //descriptor: desc, - row_id_times, - row_id_counters, - }) - } else { - Err(DisplayRecordBatchError::UnknownControlColumn( - desc.component_name.to_string(), - )) - } - } ColumnDescriptor::Time(desc) => { let time_data = column_data .as_any() @@ -243,6 +200,7 @@ impl DisplayColumn { time_data, }) } + ColumnDescriptor::Component(desc) => Ok(Self::Component { entity_path: desc.entity_path.clone(), component_name: desc.component_name, @@ -253,7 +211,7 @@ impl DisplayColumn { pub(crate) fn instance_count(&self, row_index: usize) -> u64 { match self { - Self::RowId { .. } | Self::Timeline { .. } => 1, + Self::Timeline { .. } => 1, Self::Component { component_data, .. } => component_data.instance_count(row_index), } } @@ -268,7 +226,6 @@ impl DisplayColumn { &self, ctx: &ViewerContext<'_>, ui: &mut egui::Ui, - row_id: RowId, latest_at_query: &LatestAtQuery, row_index: usize, instance_index: Option, @@ -281,22 +238,6 @@ impl DisplayColumn { } match self { - Self::RowId { - row_id_times, - row_id_counters, - .. - } => { - if instance_index.is_some() { - // we only ever display the row id on the summary line - return; - } - - let row_id = RowId::from_u128( - (row_id_times.value(row_index) as u128) << 64 - | (row_id_counters.value(row_index) as u128), - ); - row_id_ui(ctx, ui, &row_id); - } Self::Timeline { timeline, time_data, @@ -324,7 +265,6 @@ impl DisplayColumn { component_data.data_ui( ctx, ui, - row_id, latest_at_query, entity_path, *component_name, @@ -335,23 +275,6 @@ impl DisplayColumn { } } - /// Try to decode the row ID from the given row index. - /// - /// Succeeds only if the column is a `RowId` column. - pub(crate) fn try_decode_row_id(&self, row_index: usize) -> Option { - match self { - Self::RowId { - row_id_times, - row_id_counters, - } => { - let time = row_id_times.value(row_index); - let counter = row_id_counters.value(row_index); - Some(RowId::from_u128((time as u128) << 64 | (counter as u128))) - } - _ => None, - } - } - /// Try to decode the time from the given row index. /// /// Succeeds only if the column is a `Timeline` column. @@ -361,7 +284,7 @@ impl DisplayColumn { let timestamp = time_data.value(row_index); TimeInt::try_from(timestamp).ok() } - _ => None, + Self::Component { .. } => None, } } } @@ -405,26 +328,3 @@ impl DisplayRecordBatch { &self.columns } } - -fn row_id_ui(ctx: &ViewerContext<'_>, ui: &mut egui::Ui, row_id: &RowId) { - let s = row_id.to_string(); - let split_pos = s.char_indices().nth_back(5); - - ui.label(match split_pos { - Some((pos, _)) => &s[pos..], - None => &s, - }) - .on_hover_ui(|ui| { - let text = format!( - "{}\n\nTimestamp: {}\nIncrement: {}", - s, - (row_id.nanoseconds_since_epoch() as i64) - .try_into() - .map(|t| TimeType::Time.format(TimeInt::from_nanos(t), ctx.app_options.time_zone)) - .unwrap_or("error decoding timestamp".to_owned()), - row_id.inc() - ); - - ui.label(text); - }); -} diff --git a/crates/viewer/re_space_view_dataframe/src/view_query/blueprint.rs b/crates/viewer/re_space_view_dataframe/src/view_query/blueprint.rs index 27065df42cfc..4d4b2d6ace2d 100644 --- a/crates/viewer/re_space_view_dataframe/src/view_query/blueprint.rs +++ b/crates/viewer/re_space_view_dataframe/src/view_query/blueprint.rs @@ -111,12 +111,12 @@ impl Query { let mut selected_columns = datatypes::SelectedColumns::default(); for column in columns { match column { - ColumnSelector::Control(_) => {} ColumnSelector::Time(desc) => { selected_columns .time_columns .push(desc.timeline.as_str().into()); } + ColumnSelector::Component(desc) => { let blueprint_component_descriptor = datatypes::ComponentColumnSelector::new(&desc.entity_path, desc.component); @@ -186,12 +186,12 @@ impl Query { let result = view_columns .iter() .filter(|column| match column { - ColumnDescriptor::Control(_) => true, ColumnDescriptor::Time(desc) => { // we always include the query timeline column because we need it for the dataframe ui desc.timeline.name() == &query_timeline_name || selected_time_columns.contains(desc.timeline.name()) } + ColumnDescriptor::Component(desc) => { // Check against both the full name and short name, as the user might have used // the latter in the blueprint API. @@ -232,7 +232,7 @@ impl Query { HideColumnAction::HideTimeColumn { timeline_name } => { selected_columns.retain(|column| match column { ColumnSelector::Time(desc) => desc.timeline != timeline_name, - _ => true, + ColumnSelector::Component(_) => true, }); } @@ -244,7 +244,7 @@ impl Query { ColumnSelector::Component(desc) => { desc.entity_path != entity_path || desc.component != component_name } - _ => true, + ColumnSelector::Time(_) => true, }); } } diff --git a/crates/viewer/re_space_view_dataframe/src/view_query/ui.rs b/crates/viewer/re_space_view_dataframe/src/view_query/ui.rs index 95ee9f6604c7..f606bf432411 100644 --- a/crates/viewer/re_space_view_dataframe/src/view_query/ui.rs +++ b/crates/viewer/re_space_view_dataframe/src/view_query/ui.rs @@ -293,36 +293,6 @@ impl Query { ui.add_space(12.0); - // - // Control columns (always selected) - // - - let mut first = true; - for column in view_columns { - let ColumnDescriptor::Control(_) = column else { - continue; - }; - - if first { - ui.label("Control"); - first = false; - } - - // Control columns are always shown because: - // - now it's just `RowId` - // - the dataframe UI requires the `RowId` column to be present (for tooltips) - let is_enabled = false; - let mut is_visible = true; - - ui.add_enabled_ui(is_enabled, |ui| { - if ui - .re_checkbox(&mut is_visible, column.short_name()) - .on_disabled_hover_text("The query timeline must always be visible") - .changed() - { /* cannot happen for now */ } - }); - } - // // Time columns // diff --git a/rerun_py/src/dataframe.rs b/rerun_py/src/dataframe.rs index a4ecc9fbd68e..b38ce40c7839 100644 --- a/rerun_py/src/dataframe.rs +++ b/rerun_py/src/dataframe.rs @@ -16,13 +16,12 @@ use pyo3::{ use re_chunk_store::{ ChunkStore, ChunkStoreConfig, ColumnDescriptor, ColumnSelector, ComponentColumnDescriptor, - ComponentColumnSelector, ControlColumnDescriptor, ControlColumnSelector, QueryExpression2, - SparseFillStrategy, TimeColumnDescriptor, TimeColumnSelector, VersionPolicy, - ViewContentsSelector, + ComponentColumnSelector, QueryExpression2, SparseFillStrategy, TimeColumnDescriptor, + TimeColumnSelector, VersionPolicy, ViewContentsSelector, }; use re_dataframe2::QueryEngine; use re_log_types::{EntityPathFilter, ResolvedTimeRange, TimeType}; -use re_sdk::{ComponentName, EntityPath, Loggable as _, StoreId, StoreKind}; +use re_sdk::{ComponentName, EntityPath, StoreId, StoreKind}; /// Register the `rerun.dataframe` module. pub(crate) fn register(m: &Bound<'_, PyModule>) -> PyResult<()> { @@ -30,8 +29,6 @@ pub(crate) fn register(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; - m.add_class::()?; - m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_class::()?; @@ -44,43 +41,6 @@ pub(crate) fn register(m: &Bound<'_, PyModule>) -> PyResult<()> { Ok(()) } -/// Python binding for [`ControlColumnDescriptor`] -#[pyclass(frozen, name = "ControlColumnDescriptor")] -#[derive(Clone)] -struct PyControlColumnDescriptor(ControlColumnDescriptor); - -#[pymethods] -impl PyControlColumnDescriptor { - fn __repr__(&self) -> String { - format!("Ctrl({})", self.0.component_name.short_name()) - } -} - -impl From for PyControlColumnDescriptor { - fn from(desc: ControlColumnDescriptor) -> Self { - Self(desc) - } -} - -/// Python binding for [`ControlColumnSelector`] -#[pyclass(frozen, name = "ControlColumnSelector")] -#[derive(Clone)] -struct PyControlColumnSelector(ControlColumnSelector); - -#[pymethods] -impl PyControlColumnSelector { - #[staticmethod] - fn row_id() -> Self { - Self(ControlColumnSelector { - component: re_chunk::RowId::name(), - }) - } - - fn __repr__(&self) -> String { - format!("Ctrl({})", self.0.component.short_name()) - } -} - /// Python binding for [`TimeColumnDescriptor`] #[pyclass(frozen, name = "TimeColumnDescriptor")] #[derive(Clone)] @@ -195,10 +155,6 @@ impl PyComponentColumnSelector { /// Python binding for [`AnyColumn`] type-alias. #[derive(FromPyObject)] enum AnyColumn { - #[pyo3(transparent, annotation = "control_descriptor")] - ControlDescriptor(PyControlColumnDescriptor), - #[pyo3(transparent, annotation = "control_selector")] - ControlSelector(PyControlColumnSelector), #[pyo3(transparent, annotation = "time_descriptor")] TimeDescriptor(PyIndexColumnDescriptor), #[pyo3(transparent, annotation = "time_selector")] @@ -212,8 +168,6 @@ enum AnyColumn { impl AnyColumn { fn into_selector(self) -> ColumnSelector { match self { - Self::ControlDescriptor(desc) => ColumnDescriptor::Control(desc.0).into(), - Self::ControlSelector(selector) => selector.0.into(), Self::TimeDescriptor(desc) => ColumnDescriptor::Time(desc.0).into(), Self::TimeSelector(selector) => selector.0.into(), Self::ComponentDescriptor(desc) => ColumnDescriptor::Component(desc.0).into(), @@ -269,19 +223,6 @@ pub struct PySchema { #[pymethods] impl PySchema { - fn control_columns(&self) -> Vec { - self.schema - .iter() - .filter_map(|column| { - if let ColumnDescriptor::Control(col) = column { - Some(col.clone().into()) - } else { - None - } - }) - .collect() - } - fn index_columns(&self) -> Vec { self.schema .iter()