Skip to content

Commit

Permalink
Revert "Refactor TimeColumnDescriptor (#8658)"
Browse files Browse the repository at this point in the history 
This reverts commit 1ffe586.
  • Loading branch information
@emilk
emilk authored Jan 14, 2025
1 parent 3906792 commit d85db09
Show file tree
Hide file tree
Showing 7 changed files with 123 additions and 135 deletions.
59 changes: 16 additions & 43 deletions crates/store/re_chunk_store/src/dataframe.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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(),
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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((
Expand All @@ -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())
}
}

Expand Down Expand Up @@ -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(),
})
});

Expand Down Expand Up @@ -809,7 +782,7 @@ impl ChunkStore {

TimeColumnDescriptor {
timeline,
is_null: false,
datatype: timeline.datatype().into(),
}
}

Expand Down
149 changes: 81 additions & 68 deletions crates/store/re_dataframe/src/query.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,73 +373,82 @@ impl<E: StorageEngineLike> QueryHandle<E> {
) -> 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()
Expand Down Expand Up @@ -1239,10 +1248,14 @@ impl<E: StorageEngineLike> QueryHandle<E> {
.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()
},
)
}
Expand Down
36 changes: 19 additions & 17 deletions crates/viewer/re_view_dataframe/src/dataframe_ui.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Expand Down Expand Up @@ -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
Expand All @@ -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,
},
);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/viewer/re_view_dataframe/src/display_record_batch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ impl DisplayColumn {
) -> Result<Self, DisplayRecordBatchError> {
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 {
Expand Down
4 changes: 2 additions & 2 deletions crates/viewer/re_view_dataframe/src/view_query/blueprint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
2 changes: 1 addition & 1 deletion crates/viewer/re_view_dataframe/src/view_query/ui.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading

0 comments on commit d85db09

Please sign in to comment.