Skip to content

Add DataFusionTableWidget::column_visibility and use it to improve visibility defaults for the partition table #9936

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion crates/store/re_log_types/src/path/entity_path_part.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use re_string_interner::InternedString;
use crate::PathParseError;

pub const RESERVED_NAMESPACE_PREFIX: &str = "__";
const PROPERTIES_PART: &str = "__properties";
pub const PROPERTIES_PART: &str = "__properties";
const RECORDING_PART: &str = "recording";

/// The different parts that make up an [`EntityPath`][crate::EntityPath].
Expand Down
2 changes: 1 addition & 1 deletion crates/store/re_log_types/src/path/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub use entity_path_filter::{
EntityPathFilter, EntityPathFilterError, EntityPathRule, EntityPathSubs,
ResolvedEntityPathFilter, ResolvedEntityPathRule, RuleEffect,
};
pub use entity_path_part::EntityPathPart;
pub use entity_path_part::{EntityPathPart, PROPERTIES_PART};
pub use parse_path::{PathParseError, tokenize_by};

// ----------------------------------------------------------------------------
Expand Down
9 changes: 9 additions & 0 deletions crates/store/re_sorbet/src/column_descriptor_ref.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use re_log_types::EntityPath;

use crate::{
BatchType, ColumnDescriptor, ComponentColumnDescriptor, IndexColumnDescriptor,
RowIdColumnDescriptor,
Expand Down Expand Up @@ -31,6 +33,13 @@ impl ColumnDescriptorRef<'_> {
Self::Component(descr) => descr.component_name.short_name(),
}
}

pub fn entity_path(&self) -> Option<EntityPath> {
match self {
Self::RowId(_) | Self::Time(_) => None,
Self::Component(descr) => Some(descr.entity_path.clone()),
}
}
}

impl<'a> From<&'a ColumnDescriptor> for ColumnDescriptorRef<'a> {
Expand Down
9 changes: 7 additions & 2 deletions crates/store/re_sorbet/src/sorbet_columns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ use nohash_hasher::IntSet;
use re_log_types::{EntityPath, TimelineName};

use crate::{
ColumnDescriptor, ColumnKind, ComponentColumnDescriptor, ComponentColumnSelector,
IndexColumnDescriptor, RowIdColumnDescriptor, SorbetError, TimeColumnSelector,
ColumnDescriptor, ColumnDescriptorRef, ColumnKind, ComponentColumnDescriptor,
ComponentColumnSelector, IndexColumnDescriptor, RowIdColumnDescriptor, SorbetError,
TimeColumnSelector,
};

#[derive(thiserror::Error, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -72,6 +73,10 @@ impl SorbetColumnDescriptors {
.collect()
}

pub fn iter_ref(&self) -> impl Iterator<Item = ColumnDescriptorRef<'_>> {
self.columns.iter().map(ColumnDescriptorRef::from)
}

pub fn row_id_columns(&self) -> impl Iterator<Item = &RowIdColumnDescriptor> {
self.columns.iter().filter_map(|descr| {
if let ColumnDescriptor::RowId(descr) = descr {
Expand Down
56 changes: 38 additions & 18 deletions crates/viewer/re_dataframe_ui/src/datafusion_table_widget.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@ impl<'a> Columns<'a> {
}

impl Columns<'_> {
fn descriptors(&self) -> impl Iterator<Item = &ColumnDescriptorRef<'_>> {
self.inner.values().map(|(_, desc)| desc)
}

fn index_from_id(&self, id: Option<egui::Id>) -> Option<usize> {
id.and_then(|id| self.inner.get(&id).map(|(index, _)| *index))
}
Expand All @@ -57,7 +53,9 @@ impl Columns<'_> {
}
}

type ColumnRenamerFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>>;
pub type ColumnNameFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>>;

pub type ColumnVisibilityFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> bool + 'a>>;

pub struct DataFusionTableWidget<'a> {
session_ctx: Arc<SessionContext>,
Expand All @@ -73,7 +71,10 @@ pub struct DataFusionTableWidget<'a> {
/// Closure used to determine the display name of the column.
///
/// Defaults to using [`ColumnDescriptorRef::name`].
column_renamer: ColumnRenamerFn<'a>,
column_name_fn: ColumnNameFn<'a>,

/// Closure used to determine the visibility of the column
column_visibility_fn: ColumnVisibilityFn<'a>,

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

title: None,
title_button: None,
column_renamer: None,
column_name_fn: None,
column_visibility_fn: None,
initial_blueprint: Default::default(),
}
}
Expand All @@ -116,11 +118,22 @@ impl<'a> DataFusionTableWidget<'a> {
self
}

pub fn column_renamer(
pub fn column_name(
mut self,
renamer: impl Fn(&ColumnDescriptorRef<'_>) -> String + 'a,
column_name_fn: impl Fn(&ColumnDescriptorRef<'_>) -> String + 'a,
) -> Self {
self.column_renamer = Some(Box::new(renamer));
self.column_name_fn = Some(Box::new(column_name_fn));

self
}

// TODO(ab): this should best be expressed as part of the `TableBlueprint`, but we need better
// column selector first.
pub fn column_visibility(
mut self,
column_visibility_fn: impl Fn(&ColumnDescriptorRef<'_>) -> bool + 'a,
) -> Self {
self.column_visibility_fn = Some(Box::new(column_visibility_fn));

self
}
Expand Down Expand Up @@ -153,7 +166,8 @@ impl<'a> DataFusionTableWidget<'a> {
table_ref,
title,
title_button,
column_renamer,
column_name_fn,
column_visibility_fn,
initial_blueprint,
} = self;

Expand Down Expand Up @@ -256,14 +270,20 @@ impl<'a> DataFusionTableWidget<'a> {
let mut table_config = TableConfig::get_with_columns(
ui.ctx(),
id,
columns.descriptors().map(|c| {
let name = if let Some(renamer) = &column_renamer {
renamer(c)
sorbet_schema.columns.iter_ref().map(|c| {
let name = if let Some(column_name_fn) = &column_name_fn {
column_name_fn(&c)
} else {
c.name(BatchType::Dataframe)
};

ColumnConfig::new(Id::new(c), name)
let visible = if let Some(column_visibility_fn) = &column_visibility_fn {
column_visibility_fn(&c)
} else {
true
};

ColumnConfig::new_with_visible(Id::new(c), name, visible)
}),
);

Expand All @@ -280,7 +300,7 @@ impl<'a> DataFusionTableWidget<'a> {
fields,
display_record_batches: &display_record_batches,
columns: &columns,
column_renamer: &column_renamer,
column_name_fn: &column_name_fn,
blueprint: table_state.blueprint(),
new_blueprint: &mut new_blueprint,
table_config,
Expand Down Expand Up @@ -348,7 +368,7 @@ struct DataFusionTableDelegate<'a> {
fields: &'a Fields,
display_record_batches: &'a Vec<DisplayRecordBatch>,
columns: &'a Columns<'a>,
column_renamer: &'a ColumnRenamerFn<'a>,
column_name_fn: &'a ColumnNameFn<'a>,
blueprint: &'a TableBlueprint,
new_blueprint: &'a mut TableBlueprint,
table_config: TableConfig,
Expand All @@ -362,7 +382,7 @@ impl egui_table::TableDelegate for DataFusionTableDelegate<'_> {

if let Some((index, desc)) = self.columns.index_and_descriptor_from_id(id) {
let column_name = self.fields[index].name();
let name = if let Some(renamer) = self.column_renamer {
let name = if let Some(renamer) = self.column_name_fn {
renamer(desc)
} else {
desc.name(BatchType::Dataframe)
Expand Down
4 changes: 4 additions & 0 deletions crates/viewer/re_dataframe_ui/src/table_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ impl ColumnConfig {
visible: true,
}
}

pub fn new_with_visible(id: Id, name: String, visible: bool) -> Self {
Self { id, name, visible }
}
}

// TODO(lucasmerlin): It would be nice to have this in egui_table, so egui_table could do the work
Expand Down
24 changes: 21 additions & 3 deletions crates/viewer/re_redap_browser/src/servers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use std::sync::mpsc::{Receiver, Sender};
use egui::{Frame, Margin, RichText};

use re_log_types::EntryId;
use re_protos::manifest_registry::v1alpha1::DATASET_MANIFEST_ID_FIELD_NAME;
use re_protos::manifest_registry::v1alpha1::{
DATASET_MANIFEST_ID_FIELD_NAME, DATASET_MANIFEST_REGISTRATION_TIME_FIELD_NAME,
};
use re_ui::list_item::ItemActionButton;
use re_ui::{UiExt as _, icons, list_item};
use re_viewer_context::{
Expand Down Expand Up @@ -115,6 +117,8 @@ impl Server {
ui: &mut egui::Ui,
dataset: &Dataset,
) {
const RECORDING_LINK_FIELD_NAME: &str = "recording link";

re_dataframe_ui::DataFusionTableWidget::new(
self.tables_session_ctx.ctx.clone(),
dataset.name(),
Expand All @@ -125,16 +129,30 @@ impl Server {
.send(Command::RefreshCollection(self.origin.clone()))
.ok();
}))
.column_renamer(|desc| {
.column_name(|desc| {
//TODO(ab): with this strategy, we do not display relevant entity path if any.
let name = desc.short_name();

name.strip_prefix("rerun_")
.unwrap_or(name)
.replace('_', " ")
})
.column_visibility(|desc| {
if desc.entity_path().is_some_and(|entity_path| {
entity_path.starts_with(&re_log_types::PROPERTIES_PART.into())
}) {
true
} else {
matches!(
desc.short_name(),
RECORDING_LINK_FIELD_NAME
| DATASET_MANIFEST_ID_FIELD_NAME
| DATASET_MANIFEST_REGISTRATION_TIME_FIELD_NAME
)
}
})
.generate_partition_links(
"recording link",
RECORDING_LINK_FIELD_NAME,
DATASET_MANIFEST_ID_FIELD_NAME,
self.origin.clone(),
dataset.id(),
Expand Down
2 changes: 1 addition & 1 deletion crates/viewer/re_viewer/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ fn table_ui(
) {
re_dataframe_ui::DataFusionTableWidget::new(store.session_context(), TableStore::TABLE_NAME)
.title(table_id.as_str())
.column_renamer(|desc| match desc {
.column_name(|desc| match desc {
ColumnDescriptorRef::RowId(_) | ColumnDescriptorRef::Time(_) => {
desc.short_name().to_owned()
}
Expand Down
Loading