Skip to content

Commit a3ddd26

Browse files
authored
Add DataFusionTableWidget::column_visibility and use it to improve visibility defaults for the partition table (#9936)
### Related * part of #9741 ### What Title. Also fixes the column ordering, which was wrongly based on their id. And rename a bunch of things for consistency. Current default view: <img width="1550" alt="image" src="https://github.com/user-attachments/assets/1e7863fc-8bd3-4e81-85ea-0f7850761af6" />
1 parent 4eed8e1 commit a3ddd26

File tree

6 files changed

+81
-25
lines changed

6 files changed

+81
-25
lines changed

crates/store/re_sorbet/src/column_descriptor_ref.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use re_log_types::EntityPath;
2+
13
use crate::{
24
BatchType, ColumnDescriptor, ComponentColumnDescriptor, IndexColumnDescriptor,
35
RowIdColumnDescriptor,
@@ -31,6 +33,13 @@ impl ColumnDescriptorRef<'_> {
3133
Self::Component(descr) => descr.component_name.short_name(),
3234
}
3335
}
36+
37+
pub fn entity_path(&self) -> Option<&EntityPath> {
38+
match self {
39+
Self::RowId(_) | Self::Time(_) => None,
40+
Self::Component(descr) => Some(&descr.entity_path),
41+
}
42+
}
3443
}
3544

3645
impl<'a> From<&'a ColumnDescriptor> for ColumnDescriptorRef<'a> {

crates/store/re_sorbet/src/sorbet_columns.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ use nohash_hasher::IntSet;
44
use re_log_types::{EntityPath, TimelineName};
55

66
use crate::{
7-
ColumnDescriptor, ColumnKind, ComponentColumnDescriptor, ComponentColumnSelector,
8-
IndexColumnDescriptor, RowIdColumnDescriptor, SorbetError, TimeColumnSelector,
7+
ColumnDescriptor, ColumnDescriptorRef, ColumnKind, ComponentColumnDescriptor,
8+
ComponentColumnSelector, IndexColumnDescriptor, RowIdColumnDescriptor, SorbetError,
9+
TimeColumnSelector,
910
};
1011

1112
#[derive(thiserror::Error, Debug, PartialEq, Eq)]
@@ -72,6 +73,10 @@ impl SorbetColumnDescriptors {
7273
.collect()
7374
}
7475

76+
pub fn iter_ref(&self) -> impl Iterator<Item = ColumnDescriptorRef<'_>> {
77+
self.columns.iter().map(ColumnDescriptorRef::from)
78+
}
79+
7580
pub fn row_id_columns(&self) -> impl Iterator<Item = &RowIdColumnDescriptor> {
7681
self.columns.iter().filter_map(|descr| {
7782
if let ColumnDescriptor::RowId(descr) = descr {

crates/viewer/re_dataframe_ui/src/datafusion_table_widget.rs

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,6 @@ impl<'a> Columns<'a> {
4141
}
4242

4343
impl Columns<'_> {
44-
fn descriptors(&self) -> impl Iterator<Item = &ColumnDescriptorRef<'_>> {
45-
self.inner.values().map(|(_, desc)| desc)
46-
}
47-
4844
fn index_from_id(&self, id: Option<egui::Id>) -> Option<usize> {
4945
id.and_then(|id| self.inner.get(&id).map(|(index, _)| *index))
5046
}
@@ -57,7 +53,9 @@ impl Columns<'_> {
5753
}
5854
}
5955

60-
type ColumnRenamerFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>>;
56+
type ColumnNameFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>>;
57+
58+
type ColumnVisibilityFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> bool + 'a>>;
6159

6260
pub struct DataFusionTableWidget<'a> {
6361
session_ctx: Arc<SessionContext>,
@@ -73,7 +71,10 @@ pub struct DataFusionTableWidget<'a> {
7371
/// Closure used to determine the display name of the column.
7472
///
7573
/// Defaults to using [`ColumnDescriptorRef::name`].
76-
column_renamer: ColumnRenamerFn<'a>,
74+
column_name_fn: ColumnNameFn<'a>,
75+
76+
/// Closure used to determine the default visibility of the column
77+
default_column_visibility_fn: ColumnVisibilityFn<'a>,
7778

7879
/// The blueprint used the first time the table is queried.
7980
initial_blueprint: TableBlueprint,
@@ -99,7 +100,8 @@ impl<'a> DataFusionTableWidget<'a> {
99100

100101
title: None,
101102
title_button: None,
102-
column_renamer: None,
103+
column_name_fn: None,
104+
default_column_visibility_fn: None,
103105
initial_blueprint: Default::default(),
104106
}
105107
}
@@ -116,11 +118,22 @@ impl<'a> DataFusionTableWidget<'a> {
116118
self
117119
}
118120

119-
pub fn column_renamer(
121+
pub fn column_name(
120122
mut self,
121-
renamer: impl Fn(&ColumnDescriptorRef<'_>) -> String + 'a,
123+
column_name_fn: impl Fn(&ColumnDescriptorRef<'_>) -> String + 'a,
122124
) -> Self {
123-
self.column_renamer = Some(Box::new(renamer));
125+
self.column_name_fn = Some(Box::new(column_name_fn));
126+
127+
self
128+
}
129+
130+
// TODO(ab): this should best be expressed as part of the `TableBlueprint`, but we need better
131+
// column selector first.
132+
pub fn default_column_visibility(
133+
mut self,
134+
column_visibility_fn: impl Fn(&ColumnDescriptorRef<'_>) -> bool + 'a,
135+
) -> Self {
136+
self.default_column_visibility_fn = Some(Box::new(column_visibility_fn));
124137

125138
self
126139
}
@@ -153,7 +166,8 @@ impl<'a> DataFusionTableWidget<'a> {
153166
table_ref,
154167
title,
155168
title_button,
156-
column_renamer,
169+
column_name_fn,
170+
default_column_visibility_fn,
157171
initial_blueprint,
158172
} = self;
159173

@@ -256,14 +270,20 @@ impl<'a> DataFusionTableWidget<'a> {
256270
let mut table_config = TableConfig::get_with_columns(
257271
ui.ctx(),
258272
id,
259-
columns.descriptors().map(|c| {
260-
let name = if let Some(renamer) = &column_renamer {
261-
renamer(c)
273+
sorbet_schema.columns.iter_ref().map(|c| {
274+
let name = if let Some(column_name_fn) = &column_name_fn {
275+
column_name_fn(&c)
262276
} else {
263277
c.name(BatchType::Dataframe)
264278
};
265279

266-
ColumnConfig::new(Id::new(c), name)
280+
let visible = if let Some(column_visibility_fn) = &default_column_visibility_fn {
281+
column_visibility_fn(&c)
282+
} else {
283+
true
284+
};
285+
286+
ColumnConfig::new_with_visible(Id::new(c), name, visible)
267287
}),
268288
);
269289

@@ -280,7 +300,7 @@ impl<'a> DataFusionTableWidget<'a> {
280300
fields,
281301
display_record_batches: &display_record_batches,
282302
columns: &columns,
283-
column_renamer: &column_renamer,
303+
column_name_fn: &column_name_fn,
284304
blueprint: table_state.blueprint(),
285305
new_blueprint: &mut new_blueprint,
286306
table_config,
@@ -348,7 +368,7 @@ struct DataFusionTableDelegate<'a> {
348368
fields: &'a Fields,
349369
display_record_batches: &'a Vec<DisplayRecordBatch>,
350370
columns: &'a Columns<'a>,
351-
column_renamer: &'a ColumnRenamerFn<'a>,
371+
column_name_fn: &'a ColumnNameFn<'a>,
352372
blueprint: &'a TableBlueprint,
353373
new_blueprint: &'a mut TableBlueprint,
354374
table_config: TableConfig,
@@ -362,7 +382,7 @@ impl egui_table::TableDelegate for DataFusionTableDelegate<'_> {
362382

363383
if let Some((index, desc)) = self.columns.index_and_descriptor_from_id(id) {
364384
let column_name = self.fields[index].name();
365-
let name = if let Some(renamer) = self.column_renamer {
385+
let name = if let Some(renamer) = self.column_name_fn {
366386
renamer(desc)
367387
} else {
368388
desc.name(BatchType::Dataframe)

crates/viewer/re_dataframe_ui/src/table_utils.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,10 @@ impl ColumnConfig {
9696
visible: true,
9797
}
9898
}
99+
100+
pub fn new_with_visible(id: Id, name: String, visible: bool) -> Self {
101+
Self { id, name, visible }
102+
}
99103
}
100104

101105
// TODO(lucasmerlin): It would be nice to have this in egui_table, so egui_table could do the work

crates/viewer/re_redap_browser/src/servers.rs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@ use std::sync::mpsc::{Receiver, Sender};
33

44
use egui::{Frame, Margin, RichText};
55

6-
use re_log_types::EntryId;
7-
use re_protos::manifest_registry::v1alpha1::DATASET_MANIFEST_ID_FIELD_NAME;
6+
use re_log_types::{EntityPathPart, EntryId};
7+
use re_protos::manifest_registry::v1alpha1::{
8+
DATASET_MANIFEST_ID_FIELD_NAME, DATASET_MANIFEST_REGISTRATION_TIME_FIELD_NAME,
9+
};
810
use re_ui::list_item::ItemActionButton;
911
use re_ui::{UiExt as _, icons, list_item};
1012
use re_viewer_context::{
@@ -115,6 +117,8 @@ impl Server {
115117
ui: &mut egui::Ui,
116118
dataset: &Dataset,
117119
) {
120+
const RECORDING_LINK_FIELD_NAME: &str = "recording link";
121+
118122
re_dataframe_ui::DataFusionTableWidget::new(
119123
self.tables_session_ctx.ctx.clone(),
120124
dataset.name(),
@@ -125,16 +129,30 @@ impl Server {
125129
.send(Command::RefreshCollection(self.origin.clone()))
126130
.ok();
127131
}))
128-
.column_renamer(|desc| {
132+
.column_name(|desc| {
129133
//TODO(ab): with this strategy, we do not display relevant entity path if any.
130134
let name = desc.short_name();
131135

132136
name.strip_prefix("rerun_")
133137
.unwrap_or(name)
134138
.replace('_', " ")
135139
})
140+
.default_column_visibility(|desc| {
141+
if desc.entity_path().is_some_and(|entity_path| {
142+
entity_path.starts_with(&std::iter::once(EntityPathPart::properties()).collect())
143+
}) {
144+
true
145+
} else {
146+
matches!(
147+
desc.short_name(),
148+
RECORDING_LINK_FIELD_NAME
149+
| DATASET_MANIFEST_ID_FIELD_NAME
150+
| DATASET_MANIFEST_REGISTRATION_TIME_FIELD_NAME
151+
)
152+
}
153+
})
136154
.generate_partition_links(
137-
"recording link",
155+
RECORDING_LINK_FIELD_NAME,
138156
DATASET_MANIFEST_ID_FIELD_NAME,
139157
self.origin.clone(),
140158
dataset.id(),

crates/viewer/re_viewer/src/app_state.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -747,7 +747,7 @@ fn table_ui(
747747
) {
748748
re_dataframe_ui::DataFusionTableWidget::new(store.session_context(), TableStore::TABLE_NAME)
749749
.title(table_id.as_str())
750-
.column_renamer(|desc| match desc {
750+
.column_name(|desc| match desc {
751751
ColumnDescriptorRef::RowId(_) | ColumnDescriptorRef::Time(_) => {
752752
desc.short_name().to_owned()
753753
}

0 commit comments

Comments
 (0)