From d85db09325aa5dd6a490dd54327d32ace7d05e79 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 14 Jan 2025 16:37:09 +0100 Subject: [PATCH] Revert "Refactor `TimeColumnDescriptor` (#8658)" This reverts commit 1ffe5862a912fea313d420f97d421ab23cf9c77a. --- crates/store/re_chunk_store/src/dataframe.rs | 59 ++----- crates/store/re_dataframe/src/query.rs | 149 ++++++++++-------- .../re_view_dataframe/src/dataframe_ui.rs | 36 +++-- .../src/display_record_batch.rs | 2 +- .../src/view_query/blueprint.rs | 4 +- .../re_view_dataframe/src/view_query/ui.rs | 2 +- rerun_py/src/dataframe.rs | 6 +- 7 files changed, 123 insertions(+), 135 deletions(-) diff --git a/crates/store/re_chunk_store/src/dataframe.rs b/crates/store/re_chunk_store/src/dataframe.rs index 95b70a779ecc..0297f77225ed 100644 --- a/crates/store/re_chunk_store/src/dataframe.rs +++ b/crates/store/re_chunk_store/src/dataframe.rs @@ -4,7 +4,6 @@ use std::collections::{BTreeMap, BTreeSet}; use std::ops::Deref; use std::ops::DerefMut; -use arrow::datatypes::DataType as ArrowDatatype; use arrow2::{ array::ListArray as ArrowListArray, datatypes::{DataType as Arrow2Datatype, Field as Arrow2Field}, @@ -46,7 +45,7 @@ impl ColumnDescriptor { #[inline] pub fn datatype(&self) -> Arrow2Datatype { match self { - Self::Time(descr) => descr.datatype().into(), + Self::Time(descr) => descr.datatype.clone(), Self::Component(descr) => descr.returned_datatype(), } } @@ -80,9 +79,10 @@ impl ColumnDescriptor { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct TimeColumnDescriptor { /// The timeline this column is associated with. - timeline: Timeline, + pub timeline: Timeline, - is_null: bool, + /// The Arrow datatype of the column. + pub datatype: Arrow2Datatype, } impl PartialOrd for TimeColumnDescriptor { @@ -97,49 +97,17 @@ impl Ord for TimeColumnDescriptor { fn cmp(&self, other: &Self) -> std::cmp::Ordering { let Self { timeline, - is_null: _, + datatype: _, } = self; timeline.cmp(&other.timeline) } } impl TimeColumnDescriptor { - /// Used when returning a null column, i.e. when a lookup failed. - pub fn new_null(name: TimelineName) -> Self { - Self { - // TODO(cmc): I picked a sequence here because I have to pick something. - // It doesn't matter, only the name will remain in the Arrow schema anyhow. - timeline: Timeline::new_sequence(name), - is_null: true, - } - } - - pub fn timeline(&self) -> Timeline { - self.timeline - } - - pub fn name(&self) -> &TimelineName { - self.timeline.name() - } - - pub fn typ(&self) -> re_log_types::TimeType { - self.timeline.typ() - } - - #[inline] - pub fn datatype(&self) -> ArrowDatatype { - let Self { timeline, is_null } = self; - if *is_null { - ArrowDatatype::Null - } else { - timeline.typ().datatype() - } - } - fn metadata(&self) -> arrow2::datatypes::Metadata { let Self { timeline, - is_null: _, + datatype: _, } = self; std::iter::once(Some(( @@ -151,10 +119,15 @@ impl TimeColumnDescriptor { } #[inline] + // Time column must be nullable since static data doesn't have a time. pub fn to_arrow_field(&self) -> Arrow2Field { - let nullable = true; // Time column must be nullable since static data doesn't have a time. - Arrow2Field::new(self.name().to_string(), self.datatype().into(), nullable) - .with_metadata(self.metadata()) + let Self { timeline, datatype } = self; + Arrow2Field::new( + timeline.name().to_string(), + datatype.clone(), + true, /* nullable */ + ) + .with_metadata(self.metadata()) } } @@ -735,7 +708,7 @@ impl ChunkStore { let timelines = self.all_timelines_sorted().into_iter().map(|timeline| { ColumnDescriptor::Time(TimeColumnDescriptor { timeline, - is_null: false, + datatype: timeline.datatype().into(), }) }); @@ -809,7 +782,7 @@ impl ChunkStore { TimeColumnDescriptor { timeline, - is_null: false, + datatype: timeline.datatype().into(), } } diff --git a/crates/store/re_dataframe/src/query.rs b/crates/store/re_dataframe/src/query.rs index 16801c116489..55670f12295a 100644 --- a/crates/store/re_dataframe/src/query.rs +++ b/crates/store/re_dataframe/src/query.rs @@ -373,73 +373,82 @@ impl QueryHandle { ) -> Vec<(usize, ColumnDescriptor)> { selection .iter() - .map(|column| match column { - ColumnSelector::Time(selected_column) => { - let TimeColumnSelector { - timeline: selected_timeline, - } = selected_column; - - view_contents - .iter() - .enumerate() - .filter_map(|(idx, view_column)| match view_column { - ColumnDescriptor::Time(view_descr) => Some((idx, view_descr)), - ColumnDescriptor::Component(_) => None, - }) - .find(|(_idx, view_descr)| *view_descr.name() == *selected_timeline) - .map_or_else( - || { - ( - usize::MAX, - ColumnDescriptor::Time(TimeColumnDescriptor::new_null( - *selected_timeline, - )), - ) - }, - |(idx, view_descr)| (idx, ColumnDescriptor::Time(view_descr.clone())), - ) - } + .map(|column| { + match column { + ColumnSelector::Time(selected_column) => { + let TimeColumnSelector { + timeline: selected_timeline, + } = selected_column; + + view_contents + .iter() + .enumerate() + .filter_map(|(idx, view_column)| match view_column { + ColumnDescriptor::Time(view_descr) => Some((idx, view_descr)), + ColumnDescriptor::Component(_) => None, + }) + .find(|(_idx, view_descr)| { + *view_descr.timeline.name() == *selected_timeline + }) + .map_or_else( + || { + ( + usize::MAX, + ColumnDescriptor::Time(TimeColumnDescriptor { + // TODO(cmc): I picked a sequence here because I have to pick something. + // It doesn't matter, only the name will remain in the Arrow schema anyhow. + timeline: Timeline::new_sequence(*selected_timeline), + datatype: arrow2::datatypes::DataType::Null, + }), + ) + }, + |(idx, view_descr)| { + (idx, ColumnDescriptor::Time(view_descr.clone())) + }, + ) + } - ColumnSelector::Component(selected_column) => { - let ComponentColumnSelector { - entity_path: selected_entity_path, - component_name: selected_component_name, - } = selected_column; - - view_contents - .iter() - .enumerate() - .filter_map(|(idx, view_column)| match view_column { - ColumnDescriptor::Component(view_descr) => Some((idx, view_descr)), - ColumnDescriptor::Time(_) => None, - }) - .find(|(_idx, view_descr)| { - view_descr.entity_path == *selected_entity_path - && view_descr.component_name.matches(selected_component_name) - }) - .map_or_else( - || { - ( - usize::MAX, - ColumnDescriptor::Component(ComponentColumnDescriptor { - entity_path: selected_entity_path.clone(), - archetype_name: None, - archetype_field_name: None, - component_name: ComponentName::from( - selected_component_name.clone(), - ), - store_datatype: arrow2::datatypes::DataType::Null, - is_static: false, - is_indicator: false, - is_tombstone: false, - is_semantically_empty: false, - }), - ) - }, - |(idx, view_descr)| { - (idx, ColumnDescriptor::Component(view_descr.clone())) - }, - ) + ColumnSelector::Component(selected_column) => { + let ComponentColumnSelector { + entity_path: selected_entity_path, + component_name: selected_component_name, + } = selected_column; + + view_contents + .iter() + .enumerate() + .filter_map(|(idx, view_column)| match view_column { + ColumnDescriptor::Component(view_descr) => Some((idx, view_descr)), + ColumnDescriptor::Time(_) => None, + }) + .find(|(_idx, view_descr)| { + view_descr.entity_path == *selected_entity_path + && view_descr.component_name.matches(selected_component_name) + }) + .map_or_else( + || { + ( + usize::MAX, + ColumnDescriptor::Component(ComponentColumnDescriptor { + entity_path: selected_entity_path.clone(), + archetype_name: None, + archetype_field_name: None, + component_name: ComponentName::from( + selected_component_name.clone(), + ), + store_datatype: arrow2::datatypes::DataType::Null, + is_static: false, + is_indicator: false, + is_tombstone: false, + is_semantically_empty: false, + }), + ) + }, + |(idx, view_descr)| { + (idx, ColumnDescriptor::Component(view_descr.clone())) + }, + ) + } } }) .collect_vec() @@ -1239,10 +1248,14 @@ impl QueryHandle { .iter() .map(|(view_idx, column)| match column { ColumnDescriptor::Time(descr) => { - max_value_per_index.get(&descr.timeline()).map_or_else( + max_value_per_index.get(&descr.timeline).map_or_else( || arrow2::array::new_null_array(column.datatype(), 1), |(_time, time_sliced)| { - descr.typ().make_arrow_array(time_sliced.clone()).into() + descr + .timeline + .typ() + .make_arrow_array(time_sliced.clone()) + .into() }, ) } diff --git a/crates/viewer/re_view_dataframe/src/dataframe_ui.rs b/crates/viewer/re_view_dataframe/src/dataframe_ui.rs index ecc7cffc835b..79c060191eb7 100644 --- a/crates/viewer/re_view_dataframe/src/dataframe_ui.rs +++ b/crates/viewer/re_view_dataframe/src/dataframe_ui.rs @@ -166,7 +166,7 @@ impl RowsDisplayData { .iter() .find_position(|desc| match desc { ColumnDescriptor::Time(time_column_desc) => { - &time_column_desc.timeline() == query_timeline + &time_column_desc.timeline == query_timeline } ColumnDescriptor::Component(_) => false, }) @@ -283,27 +283,29 @@ impl egui_table::TableDelegate for DataframeTableDelegate<'_> { .unwrap_or_else(|| Timeline::new("", TimeType::Sequence)); // if this column can actually be hidden, then that's the corresponding action - let hide_action = - match column { - ColumnDescriptor::Time(desc) => (desc.timeline() != filtered_index) - .then(|| HideColumnAction::HideTimeColumn { - timeline_name: *desc.name(), - }), - - ColumnDescriptor::Component(desc) => { - Some(HideColumnAction::HideComponentColumn { - entity_path: desc.entity_path.clone(), - component_name: desc.component_name, - }) - } - }; + let hide_action = match column { + ColumnDescriptor::Time(desc) => { + (desc.timeline != filtered_index).then(|| { + HideColumnAction::HideTimeColumn { + timeline_name: *desc.timeline.name(), + } + }) + } + + ColumnDescriptor::Component(desc) => { + Some(HideColumnAction::HideComponentColumn { + entity_path: desc.entity_path.clone(), + component_name: desc.component_name, + }) + } + }; let header_ui = |ui: &mut egui::Ui| { let text = egui::RichText::new(column.short_name()).strong(); let is_selected = match column { ColumnDescriptor::Time(descr) => { - &descr.timeline() == self.ctx.rec_cfg.time_ctrl.read().timeline() + &descr.timeline == self.ctx.rec_cfg.time_ctrl.read().timeline() } ColumnDescriptor::Component(component_column_descriptor) => self .ctx @@ -321,7 +323,7 @@ impl egui_table::TableDelegate for DataframeTableDelegate<'_> { self.ctx.command_sender.send_system( re_viewer_context::SystemCommand::SetActiveTimeline { rec_id: self.ctx.recording_id().clone(), - timeline: descr.timeline(), + timeline: descr.timeline, }, ); } diff --git a/crates/viewer/re_view_dataframe/src/display_record_batch.rs b/crates/viewer/re_view_dataframe/src/display_record_batch.rs index b774c5f80f90..a1eddc1c3dd5 100644 --- a/crates/viewer/re_view_dataframe/src/display_record_batch.rs +++ b/crates/viewer/re_view_dataframe/src/display_record_batch.rs @@ -185,7 +185,7 @@ impl DisplayColumn { ) -> Result { match column_descriptor { ColumnDescriptor::Time(desc) => { - let timeline = desc.timeline(); + let timeline = desc.timeline; let time_data = TimeColumn::read_array(column_data).map_err(|err| { DisplayRecordBatchError::BadTimeColumn { diff --git a/crates/viewer/re_view_dataframe/src/view_query/blueprint.rs b/crates/viewer/re_view_dataframe/src/view_query/blueprint.rs index 5f229cfc58e7..abb1f508fd43 100644 --- a/crates/viewer/re_view_dataframe/src/view_query/blueprint.rs +++ b/crates/viewer/re_view_dataframe/src/view_query/blueprint.rs @@ -204,8 +204,8 @@ impl Query { .filter(|column| match column { ColumnDescriptor::Time(desc) => { // we always include the query timeline column because we need it for the dataframe ui - desc.name() == &query_timeline_name - || selected_time_columns.contains(desc.name()) + desc.timeline.name() == &query_timeline_name + || selected_time_columns.contains(desc.timeline.name()) } ColumnDescriptor::Component(desc) => { diff --git a/crates/viewer/re_view_dataframe/src/view_query/ui.rs b/crates/viewer/re_view_dataframe/src/view_query/ui.rs index 8880c179cd67..795e8506067c 100644 --- a/crates/viewer/re_view_dataframe/src/view_query/ui.rs +++ b/crates/viewer/re_view_dataframe/src/view_query/ui.rs @@ -314,7 +314,7 @@ impl Query { // The query timeline is always active because it's necessary for the dataframe ui // (for tooltips). - let is_query_timeline = time_column_descriptor.name() == timeline.name(); + let is_query_timeline = time_column_descriptor.timeline.name() == timeline.name(); let is_enabled = !is_query_timeline; let mut is_visible = is_query_timeline || selected_columns.contains(&column_selector); diff --git a/rerun_py/src/dataframe.rs b/rerun_py/src/dataframe.rs index fd7772b3a4be..98fb4e18419b 100644 --- a/rerun_py/src/dataframe.rs +++ b/rerun_py/src/dataframe.rs @@ -74,7 +74,7 @@ struct PyIndexColumnDescriptor(TimeColumnDescriptor); #[pymethods] impl PyIndexColumnDescriptor { fn __repr__(&self) -> String { - format!("Index(timeline:{})", self.0.name()) + format!("Index(timeline:{})", self.0.timeline.name()) } /// The name of the index. @@ -82,7 +82,7 @@ impl PyIndexColumnDescriptor { /// This property is read-only. #[getter] fn name(&self) -> &str { - self.0.name() + self.0.timeline.name() } /// Part of generic ColumnDescriptor interface: always False for Index. @@ -1371,7 +1371,7 @@ impl PyRecording { include_semantically_empty_columns, include_indicator_columns, include_tombstone_columns, - filtered_index: Some(timeline.timeline()), + filtered_index: Some(timeline.timeline), filtered_index_range: None, filtered_index_values: None, using_index_values: None,