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

Also fixes the column ordering, which was wrongly based on their id.

Author: abey79

crates/store/re_log_types/src/path/entity_path_part.rs

@@ -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].

crates/store/re_log_types/src/path/mod.rs

@@ -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::{tokenize_by, PathParseError};
 
 // ----------------------------------------------------------------------------

crates/store/re_sorbet/src/column_descriptor_ref.rs

@@ -1,3 +1,5 @@
+use re_log_types::EntityPath;
+
 use crate::{
     BatchType, ColumnDescriptor, ComponentColumnDescriptor, IndexColumnDescriptor,
     RowIdColumnDescriptor,
@@ -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> {

crates/viewer/re_dataframe_ui/src/datafusion_table_widget.rs

@@ -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))
     }
@@ -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>,
@@ -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,
@@ -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(),
         }
     }
@@ -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
     }
@@ -153,7 +166,8 @@ impl<'a> DataFusionTableWidget<'a> {
             table_ref,
             title,
             title_button,
-            column_renamer,
+            column_name_fn,
+            column_visibility_fn,
             initial_blueprint,
         } = self;
 
@@ -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.descriptors().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)
             }),
         );
 
@@ -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,
@@ -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,
@@ -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)

crates/viewer/re_dataframe_ui/src/table_utils.rs

@@ -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

crates/viewer/re_redap_browser/src/servers.rs

@@ -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::{icons, list_item, UiExt as _};
 use re_viewer_context::{
@@ -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(),
@@ -125,16 +129,30 @@ impl Server {
                 .command_sender
                 .send(Command::RefreshCollection(self.origin.clone()));
         }))
-        .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(),

crates/viewer/re_viewer/src/app_state.rs

@@ -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()
             }