Skip to content

Commit 920de91

Browse files
emilkemilk
authored
Make IndexColumnDescriptor members private (#9070)
### Related * Sibling PR: rerun-io/dataplatform#259 * Part of #8744 * Part of #9082 ### What A few steps on the road to unifying timelines and RowId ### TODO * [x] dataplatform PR
1 parent d717148 commit 920de91

File tree

11 files changed

+100
-94
lines changed

11 files changed

+100
-94
lines changed

‎crates/store/re_chunk/src/transport.rs

+8-8
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,11 @@ impl Chunk {
6363
} = info;
6464

6565
let array = info.times_array();
66-
let schema = re_sorbet::IndexColumnDescriptor {
67-
timeline: *timeline,
68-
datatype: array.data_type().clone(),
69-
is_sorted: *is_sorted,
70-
};
66+
67+
debug_assert_eq!(&timeline.datatype(), array.data_type());
68+
69+
let schema =
70+
re_sorbet::IndexColumnDescriptor::from_timeline(*timeline, *is_sorted);
7171

7272
(schema, into_arrow_ref(array))
7373
})
@@ -165,17 +165,17 @@ impl Chunk {
165165
let times =
166166
TimeColumn::read_array(&as_array_ref(column.clone())).map_err(|err| {
167167
ChunkError::Malformed {
168-
reason: format!("Bad time column '{}': {err}", schema.name()),
168+
reason: format!("Bad time column '{}': {err}", schema.column_name()),
169169
}
170170
})?;
171171

172172
let time_column =
173-
TimeColumn::new(schema.is_sorted.then_some(true), timeline, times);
173+
TimeColumn::new(schema.is_sorted().then_some(true), timeline, times);
174174
if timelines.insert(timeline, time_column).is_some() {
175175
return Err(ChunkError::Malformed {
176176
reason: format!(
177177
"time column '{}' was specified more than once",
178-
schema.name(),
178+
timeline.name()
179179
),
180180
});
181181
}

‎crates/store/re_chunk_store/src/dataframe.rs

+35-1
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,45 @@ pub struct TimeColumnSelector {
6666
pub timeline: TimelineName,
6767
}
6868

69+
impl From<TimelineName> for TimeColumnSelector {
70+
#[inline]
71+
fn from(timeline: TimelineName) -> Self {
72+
Self { timeline }
73+
}
74+
}
75+
76+
impl From<Timeline> for TimeColumnSelector {
77+
#[inline]
78+
fn from(timeline: Timeline) -> Self {
79+
Self {
80+
timeline: *timeline.name(),
81+
}
82+
}
83+
}
84+
85+
impl From<&str> for TimeColumnSelector {
86+
#[inline]
87+
fn from(timeline: &str) -> Self {
88+
Self {
89+
timeline: timeline.into(),
90+
}
91+
}
92+
}
93+
94+
impl From<String> for TimeColumnSelector {
95+
#[inline]
96+
fn from(timeline: String) -> Self {
97+
Self {
98+
timeline: timeline.into(),
99+
}
100+
}
101+
}
102+
69103
impl From<IndexColumnDescriptor> for TimeColumnSelector {
70104
#[inline]
71105
fn from(desc: IndexColumnDescriptor) -> Self {
72106
Self {
73-
timeline: *desc.timeline.name(),
107+
timeline: *desc.timeline().name(),
74108
}
75109
}
76110
}

‎crates/store/re_dataframe/src/query.rs

+20-50
Original file line numberDiff line numberDiff line change
@@ -393,7 +393,7 @@ impl<E: StorageEngineLike> QueryHandle<E> {
393393
ColumnDescriptor::Time(view_descr) => Some((idx, view_descr)),
394394
ColumnDescriptor::Component(_) => None,
395395
})
396-
.find(|(_idx, view_descr)| *view_descr.name() == *selected_timeline)
396+
.find(|(_idx, view_descr)| *view_descr.timeline().name() == *selected_timeline)
397397
.map_or_else(
398398
|| {
399399
(
@@ -1204,7 +1204,9 @@ impl<E: StorageEngineLike> QueryHandle<E> {
12041204
ColumnDescriptor::Time(descr) => {
12051205
max_value_per_index.get(&descr.timeline()).map_or_else(
12061206
|| arrow::array::new_null_array(&column.arrow_datatype(), 1),
1207-
|(_time, time_sliced)| descr.typ().make_arrow_array(time_sliced.clone()),
1207+
|(_time, time_sliced)| {
1208+
descr.timeline().typ().make_arrow_array(time_sliced.clone())
1209+
},
12081210
)
12091211
}
12101212

@@ -1829,15 +1831,9 @@ mod tests {
18291831
let query = QueryExpression {
18301832
filtered_index: Some(filtered_index),
18311833
selection: Some(vec![
1832-
ColumnSelector::Time(TimeColumnSelector {
1833-
timeline: *filtered_index.name(),
1834-
}),
1835-
ColumnSelector::Time(TimeColumnSelector {
1836-
timeline: *filtered_index.name(),
1837-
}),
1838-
ColumnSelector::Time(TimeColumnSelector {
1839-
timeline: "ATimeColumnThatDoesntExist".into(),
1840-
}),
1834+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1835+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1836+
ColumnSelector::Time(TimeColumnSelector::from("ATimeColumnThatDoesntExist")),
18411837
]),
18421838
..Default::default()
18431839
};
@@ -1904,36 +1900,16 @@ mod tests {
19041900
selection: Some(vec![
19051901
// NOTE: This will force a crash if the selected indexes vs. view indexes are
19061902
// improperly handled.
1907-
ColumnSelector::Time(TimeColumnSelector {
1908-
timeline: *filtered_index.name(),
1909-
}),
1910-
ColumnSelector::Time(TimeColumnSelector {
1911-
timeline: *filtered_index.name(),
1912-
}),
1913-
ColumnSelector::Time(TimeColumnSelector {
1914-
timeline: *filtered_index.name(),
1915-
}),
1916-
ColumnSelector::Time(TimeColumnSelector {
1917-
timeline: *filtered_index.name(),
1918-
}),
1919-
ColumnSelector::Time(TimeColumnSelector {
1920-
timeline: *filtered_index.name(),
1921-
}),
1922-
ColumnSelector::Time(TimeColumnSelector {
1923-
timeline: *filtered_index.name(),
1924-
}),
1925-
ColumnSelector::Time(TimeColumnSelector {
1926-
timeline: *filtered_index.name(),
1927-
}),
1928-
ColumnSelector::Time(TimeColumnSelector {
1929-
timeline: *filtered_index.name(),
1930-
}),
1931-
ColumnSelector::Time(TimeColumnSelector {
1932-
timeline: *filtered_index.name(),
1933-
}),
1934-
ColumnSelector::Time(TimeColumnSelector {
1935-
timeline: *filtered_index.name(),
1936-
}),
1903+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1904+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1905+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1906+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1907+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1908+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1909+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1910+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1911+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1912+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
19371913
//
19381914
ColumnSelector::Component(ComponentColumnSelector {
19391915
entity_path: entity_path.clone(),
@@ -1986,15 +1962,9 @@ mod tests {
19861962
.collect(),
19871963
),
19881964
selection: Some(vec![
1989-
ColumnSelector::Time(TimeColumnSelector {
1990-
timeline: *filtered_index.name(),
1991-
}),
1992-
ColumnSelector::Time(TimeColumnSelector {
1993-
timeline: *Timeline::log_time().name(),
1994-
}),
1995-
ColumnSelector::Time(TimeColumnSelector {
1996-
timeline: *Timeline::log_tick().name(),
1997-
}),
1965+
ColumnSelector::Time(TimeColumnSelector::from(*filtered_index.name())),
1966+
ColumnSelector::Time(TimeColumnSelector::from(*Timeline::log_time().name())),
1967+
ColumnSelector::Time(TimeColumnSelector::from(*Timeline::log_tick().name())),
19981968
//
19991969
ColumnSelector::Component(ComponentColumnSelector {
20001970
entity_path: entity_path.clone(),

‎crates/store/re_sorbet/src/column_descriptor.rs

+6-10
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ impl ColumnDescriptor {
5757
#[inline]
5858
pub fn short_name(&self) -> String {
5959
match self {
60-
Self::Time(descr) => descr.timeline.name().to_string(),
60+
Self::Time(descr) => descr.column_name().to_owned(),
6161
Self::Component(descr) => descr.component_name.short_name().to_owned(),
6262
}
6363
}
@@ -73,7 +73,7 @@ impl ColumnDescriptor {
7373
#[inline]
7474
pub fn arrow_datatype(&self) -> ArrowDatatype {
7575
match self {
76-
Self::Time(descr) => descr.datatype.clone(),
76+
Self::Time(descr) => descr.datatype().clone(),
7777
Self::Component(descr) => descr.returned_datatype(),
7878
}
7979
}
@@ -134,14 +134,10 @@ fn test_schema_over_ipc() {
134134
#![expect(clippy::disallowed_methods)] // Schema::new
135135

136136
let original_columns = [
137-
ColumnDescriptor::Time(IndexColumnDescriptor {
138-
timeline: re_log_types::Timeline::log_time(),
139-
datatype: arrow::datatypes::DataType::Timestamp(
140-
arrow::datatypes::TimeUnit::Nanosecond,
141-
None,
142-
),
143-
is_sorted: true,
144-
}),
137+
ColumnDescriptor::Time(IndexColumnDescriptor::from_timeline(
138+
re_log_types::Timeline::log_time(),
139+
true,
140+
)),
145141
ColumnDescriptor::Component(ComponentColumnDescriptor {
146142
entity_path: re_log_types::EntityPath::from("/some/path"),
147143
archetype_name: Some("archetype".to_owned().into()),

‎crates/store/re_sorbet/src/column_descriptor_ref.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ impl ColumnDescriptorRef<'_> {
1515
pub fn name(&self) -> String {
1616
match self {
1717
Self::RowId(descr) => descr.name().to_owned(),
18-
Self::Time(descr) => descr.timeline.name().to_string(),
18+
Self::Time(descr) => descr.column_name().to_owned(),
1919
Self::Component(descr) => descr.component_name.short_name().to_owned(),
2020
}
2121
}

‎crates/store/re_sorbet/src/index_column_descriptor.rs

+20-8
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,15 @@ pub struct UnsupportedTimeType {
1414
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
1515
pub struct IndexColumnDescriptor {
1616
/// The timeline this column is associated with.
17-
pub timeline: Timeline,
17+
timeline: Timeline,
1818

1919
/// The Arrow datatype of the column.
20-
pub datatype: ArrowDatatype,
20+
datatype: ArrowDatatype,
2121

2222
/// Are the indices in this column sorted?
2323
///
2424
/// `false` means either "unsorted" or "unknown".
25-
pub is_sorted: bool,
25+
is_sorted: bool,
2626
}
2727

2828
impl PartialOrd for IndexColumnDescriptor {
@@ -57,24 +57,36 @@ impl IndexColumnDescriptor {
5757
}
5858
}
5959

60+
#[inline]
61+
pub fn from_timeline(timeline: Timeline, is_sorted: bool) -> Self {
62+
Self {
63+
timeline,
64+
datatype: timeline.datatype(),
65+
is_sorted,
66+
}
67+
}
68+
6069
#[inline]
6170
pub fn timeline(&self) -> Timeline {
6271
self.timeline
6372
}
6473

6574
#[inline]
66-
pub fn name(&self) -> &TimelineName {
75+
pub fn column_name(&self) -> &str {
6776
self.timeline.name()
6877
}
6978

7079
#[inline]
71-
pub fn typ(&self) -> re_log_types::TimeType {
72-
self.timeline.typ()
80+
pub fn datatype(&self) -> &ArrowDatatype {
81+
&self.datatype
7382
}
7483

84+
/// Are the indices in this column sorted?
85+
///
86+
/// `false` means either "unsorted" or "unknown".
7587
#[inline]
76-
pub fn datatype(&self) -> &ArrowDatatype {
77-
&self.datatype
88+
pub fn is_sorted(&self) -> bool {
89+
self.is_sorted
7890
}
7991

8092
#[inline]

‎crates/top/rerun/src/commands/rrd/filter.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -213,5 +213,5 @@ impl FilterCommand {
213213
fn is_field_timeline_of(field: &ArrowField, dropped_timelines: &HashSet<String>) -> bool {
214214
re_sorbet::IndexColumnDescriptor::try_from(field)
215215
.ok()
216-
.is_some_and(|schema| dropped_timelines.contains(schema.name().as_str()))
216+
.is_some_and(|schema| dropped_timelines.contains(schema.column_name()))
217217
}

‎crates/viewer/re_view_dataframe/src/dataframe_ui.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ impl egui_table::TableDelegate for DataframeTableDelegate<'_> {
287287
match column {
288288
ColumnDescriptor::Time(desc) => (desc.timeline() != filtered_index)
289289
.then(|| HideColumnAction::HideTimeColumn {
290-
timeline_name: *desc.name(),
290+
timeline_name: *desc.timeline().name(),
291291
}),
292292

293293
ColumnDescriptor::Component(desc) => {

‎crates/viewer/re_view_dataframe/src/view_query/blueprint.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,8 @@ impl Query {
213213
.filter(|column| match column {
214214
ColumnDescriptor::Time(desc) => {
215215
// we always include the query timeline column because we need it for the dataframe ui
216-
desc.name() == &query_timeline_name
217-
|| selected_time_columns.contains(desc.name())
216+
desc.timeline().name() == &query_timeline_name
217+
|| selected_time_columns.contains(desc.timeline().name())
218218
}
219219

220220
ColumnDescriptor::Component(desc) => {

‎crates/viewer/re_view_dataframe/src/view_query/ui.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ impl Query {
379379

380380
// The query timeline is always active because it's necessary for the dataframe ui
381381
// (for tooltips).
382-
let is_query_timeline = time_column_descriptor.name() == timeline.name();
382+
let is_query_timeline = time_column_descriptor.timeline() == *timeline;
383383
let is_enabled = !is_query_timeline;
384384
let mut is_visible =
385385
is_query_timeline || selected_columns.contains(&column_selector);

‎rerun_py/src/dataframe.rs

+5-11
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,15 @@ struct PyIndexColumnDescriptor(IndexColumnDescriptor);
7676
#[pymethods]
7777
impl PyIndexColumnDescriptor {
7878
fn __repr__(&self) -> String {
79-
format!("Index(timeline:{})", self.0.name())
79+
format!("Index(timeline:{})", self.0.column_name())
8080
}
8181

8282
/// The name of the index.
8383
///
8484
/// This property is read-only.
8585
#[getter]
8686
fn name(&self) -> &str {
87-
self.0.name()
87+
self.0.column_name()
8888
}
8989

9090
/// Part of generic ColumnDescriptor interface: always False for Index.
@@ -121,9 +121,7 @@ impl PyIndexColumnSelector {
121121
#[new]
122122
#[pyo3(text_signature = "(self, index)")]
123123
fn new(index: &str) -> Self {
124-
Self(TimeColumnSelector {
125-
timeline: index.into(),
126-
})
124+
Self(TimeColumnSelector::from(index))
127125
}
128126

129127
fn __repr__(&self) -> String {
@@ -284,9 +282,7 @@ impl AnyColumn {
284282
match self {
285283
Self::Name(name) => {
286284
if !name.contains(':') && !name.contains('/') {
287-
Ok(ColumnSelector::Time(TimeColumnSelector {
288-
timeline: name.into(),
289-
}))
285+
Ok(ColumnSelector::Time(TimeColumnSelector::from(name)))
290286
} else {
291287
let component_path =
292288
re_log_types::ComponentPath::from_str(&name).map_err(|err| {
@@ -1340,9 +1336,7 @@ impl PyRecording {
13401336
let borrowed_self = slf.borrow();
13411337

13421338
// Look up the type of the timeline
1343-
let selector = TimeColumnSelector {
1344-
timeline: index.into(),
1345-
};
1339+
let selector = TimeColumnSelector::from(index);
13461340

13471341
let timeline = borrowed_self.store.read().resolve_time_selector(&selector);
13481342

0 commit comments

Comments
 (0)