Skip to content

Commit b60fd71

Browse files
AdamGSAdamGS
authored
TUI bug fixes (#2822)
We got stats tables back!
1 parent c878d66 commit b60fd71

File tree

9 files changed

+52
-46
lines changed

9 files changed

+52
-46
lines changed

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎vortex-layout/src/data.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ impl Layout {
244244
(0..self.nsegments()).map(move |i| self.segment_id(i).vortex_expect("segment bounds"))
245245
}
246246

247-
/// Returns the layout metadata
247+
/// Returns the bytes of the metadata stored in the layout's flatbuffer.
248248
pub fn metadata(&self) -> Option<Bytes> {
249249
match &self.0 {
250250
Inner::Owned(owned) => owned.metadata.clone(),

‎vortex-layout/src/layouts/stats/mod.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ impl LayoutVTable for StatsLayout {
5353
row_offset: u64,
5454
filter_field_mask: &[FieldMask],
5555
projection_field_mask: &[FieldMask],
56-
segments: &mut SegmentCollector,
56+
segments_collector: &mut SegmentCollector,
5757
) -> VortexResult<()> {
5858
if !filter_field_mask.is_empty() {
5959
layout
@@ -62,7 +62,7 @@ impl LayoutVTable for StatsLayout {
6262
row_offset,
6363
filter_field_mask,
6464
projection_field_mask,
65-
&mut segments.with_priority_hint(RequiredSegmentKind::PRUNING),
65+
&mut segments_collector.with_priority_hint(RequiredSegmentKind::Pruning),
6666
)?;
6767
}
6868
layout
@@ -71,7 +71,7 @@ impl LayoutVTable for StatsLayout {
7171
row_offset,
7272
filter_field_mask,
7373
projection_field_mask,
74-
segments,
74+
segments_collector,
7575
)
7676
}
7777
}

‎vortex-layout/src/layouts/struct_/mod.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,15 @@ impl LayoutVTable for StructLayout {
5959
row_offset,
6060
&[field_mask],
6161
&[],
62-
&mut segments.with_priority_hint(RequiredSegmentKind::FILTER),
62+
&mut segments.with_priority_hint(RequiredSegmentKind::Filter),
6363
)
6464
})?;
6565
for_all_matching_children(layout, projection_field_mask, |field_mask, child| {
6666
child.required_segments(
6767
row_offset,
6868
&[],
6969
&[field_mask],
70-
&mut segments.with_priority_hint(RequiredSegmentKind::PROJECTION),
70+
&mut segments.with_priority_hint(RequiredSegmentKind::Projection),
7171
)
7272
})?;
7373
Ok(())

‎vortex-layout/src/segments.rs‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,10 @@ pub trait SegmentWriter {
5959

6060
#[derive(Debug, Default, PartialEq, Eq, Hash, Clone, Copy, PartialOrd, Ord)]
6161
pub enum RequiredSegmentKind {
62-
PRUNING = 1,
63-
FILTER = 2,
62+
Pruning = 1,
63+
Filter = 2,
6464
#[default]
65-
PROJECTION = 3,
65+
Projection = 3,
6666
}
6767

6868
#[derive(Debug, PartialEq, Eq, Hash, Clone, Copy, PartialOrd, Ord)]
@@ -84,7 +84,7 @@ impl SegmentPriority {
8484

8585
const TOP_PRIORITY: SegmentPriority = SegmentPriority {
8686
row_end: 0,
87-
kind: RequiredSegmentKind::PRUNING,
87+
kind: RequiredSegmentKind::Pruning,
8888
row_start: 0,
8989
};
9090

@@ -106,7 +106,7 @@ impl SegmentCollector {
106106
}
107107

108108
pub fn with_priority_hint(&self, kind: RequiredSegmentKind) -> Self {
109-
SegmentCollector {
109+
Self {
110110
store: self.store.clone(),
111111
// highest priority wins
112112
kind: kind.min(self.kind),
@@ -117,7 +117,7 @@ impl SegmentCollector {
117117
pub fn push(&mut self, row_start: u64, row_end: u64, segment: SegmentId) {
118118
let (start, end) = match self.kind {
119119
// row offset inside the stats table is not our concern
120-
RequiredSegmentKind::PRUNING => (0, 0),
120+
RequiredSegmentKind::Pruning => (0, 0),
121121
_ => (row_start, row_end),
122122
};
123123
self.increment_metrics();
@@ -195,7 +195,7 @@ impl RowRangePruner {
195195
let mut store = self.store.write()?;
196196
let to_remove: Vec<_> = store
197197
.keys()
198-
.filter(|key| key.kind != RequiredSegmentKind::PRUNING)
198+
.filter(|key| key.kind != RequiredSegmentKind::Pruning)
199199
.skip_while(|key| key.row_end < first_row)
200200
.take_while(|key| key.row_end <= last_row)
201201
.filter(|key| first_row <= key.row_start)
@@ -229,7 +229,7 @@ impl RowRangePruner {
229229
.write()
230230
.vortex_expect("poisoned lock")
231231
.retain(|key, _| {
232-
if key.kind == RequiredSegmentKind::PRUNING {
232+
if key.kind == RequiredSegmentKind::Pruning {
233233
return true; // keep segments required for pruning
234234
}
235235
let keep =
@@ -346,23 +346,23 @@ pub mod test {
346346

347347
// Add segments that span different ranges
348348
store.insert(
349-
SegmentPriority::new(0, 100, RequiredSegmentKind::PROJECTION),
349+
SegmentPriority::new(0, 100, RequiredSegmentKind::Projection),
350350
vec![SegmentId(1)],
351351
);
352352
store.insert(
353-
SegmentPriority::new(50, 150, RequiredSegmentKind::PROJECTION),
353+
SegmentPriority::new(50, 150, RequiredSegmentKind::Projection),
354354
vec![SegmentId(2)],
355355
);
356356
store.insert(
357-
SegmentPriority::new(150, 250, RequiredSegmentKind::FILTER),
357+
SegmentPriority::new(150, 250, RequiredSegmentKind::Filter),
358358
vec![SegmentId(3)],
359359
);
360360
store.insert(
361-
SegmentPriority::new(200, 300, RequiredSegmentKind::PROJECTION),
361+
SegmentPriority::new(200, 300, RequiredSegmentKind::Projection),
362362
vec![SegmentId(4)],
363363
);
364364
store.insert(
365-
SegmentPriority::new(0, 0, RequiredSegmentKind::PRUNING),
365+
SegmentPriority::new(0, 0, RequiredSegmentKind::Pruning),
366366
vec![SegmentId(5)],
367367
);
368368

@@ -391,27 +391,27 @@ pub mod test {
391391
assert!(!store_lock.contains_key(&SegmentPriority::new(
392392
0,
393393
100,
394-
RequiredSegmentKind::PROJECTION
394+
RequiredSegmentKind::Projection
395395
)));
396396
assert!(!store_lock.contains_key(&SegmentPriority::new(
397397
50,
398398
150,
399-
RequiredSegmentKind::PROJECTION
399+
RequiredSegmentKind::Projection
400400
)));
401401
assert!(store_lock.contains_key(&SegmentPriority::new(
402402
150,
403403
250,
404-
RequiredSegmentKind::FILTER
404+
RequiredSegmentKind::Filter
405405
))); // Not fully encompassed
406406
assert!(store_lock.contains_key(&SegmentPriority::new(
407407
200,
408408
300,
409-
RequiredSegmentKind::PROJECTION
409+
RequiredSegmentKind::Projection
410410
)));
411411
assert!(store_lock.contains_key(&SegmentPriority::new(
412412
0,
413413
0,
414-
RequiredSegmentKind::PRUNING
414+
RequiredSegmentKind::Pruning
415415
)));
416416

417417
// Check that the correct cancellation messages were sent
@@ -502,17 +502,17 @@ pub mod test {
502502
assert!(!store_lock.contains_key(&SegmentPriority::new(
503503
0,
504504
100,
505-
RequiredSegmentKind::PROJECTION
505+
RequiredSegmentKind::Projection
506506
)));
507507
assert!(!store_lock.contains_key(&SegmentPriority::new(
508508
50,
509509
150,
510-
RequiredSegmentKind::PROJECTION
510+
RequiredSegmentKind::Projection
511511
)));
512512
assert!(store_lock.contains_key(&SegmentPriority::new(
513513
150,
514514
250,
515-
RequiredSegmentKind::FILTER
515+
RequiredSegmentKind::Filter
516516
))); // Not fully encompassed
517517
})
518518
}
@@ -543,29 +543,29 @@ pub mod test {
543543
assert!(store_lock.contains_key(&SegmentPriority::new(
544544
0,
545545
100,
546-
RequiredSegmentKind::PROJECTION
546+
RequiredSegmentKind::Projection
547547
))); // Contains 75
548548
assert!(store_lock.contains_key(&SegmentPriority::new(
549549
50,
550550
150,
551-
RequiredSegmentKind::PROJECTION
551+
RequiredSegmentKind::Projection
552552
))); // Contains 75, 125
553553
assert!(store_lock.contains_key(&SegmentPriority::new(
554554
150,
555555
250,
556-
RequiredSegmentKind::FILTER
556+
RequiredSegmentKind::Filter
557557
))); // Contains 175, 225
558558
assert!(store_lock.contains_key(&SegmentPriority::new(
559559
200,
560560
300,
561-
RequiredSegmentKind::PROJECTION
561+
RequiredSegmentKind::Projection
562562
))); // Contains 225
563563

564564
// PRUNING segments should always be kept
565565
assert!(store_lock.contains_key(&SegmentPriority::new(
566566
0,
567567
0,
568-
RequiredSegmentKind::PRUNING
568+
RequiredSegmentKind::Pruning
569569
)));
570570
})
571571
}

‎vortex-tui/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ rust-version = { workspace = true }
1414
version = { workspace = true }
1515

1616
[dependencies]
17+
anyhow = { workspace = true }
1718
async-trait = { workspace = true }
1819
clap = { workspace = true, features = ["derive"] }
1920
crossterm = { workspace = true }

‎vortex-tui/src/browse/app.rs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,8 @@ impl LayoutCursor {
5959
dtype = if layout.id() == CHUNKED_LAYOUT_ID {
6060
// If metadata is present, last child is stats table
6161
if layout.metadata().is_some() && component == (layout.nchildren() - 1) {
62-
let present_stats = stats_from_bitset_bytes(
63-
layout.metadata().expect("extracting stats").as_ref(),
64-
);
62+
let metadata_bytes = layout.metadata().expect("extracting stats");
63+
let present_stats = stats_from_bitset_bytes(&metadata_bytes[4..]);
6564

6665
StatsTable::dtype_for_stats_table(&dtype, &present_stats)
6766
} else {
@@ -148,7 +147,7 @@ impl LayoutCursor {
148147
/// Predicate true when the cursor is currently activated over a stats table
149148
pub fn is_stats_table(&self) -> bool {
150149
let parent = self.parent();
151-
parent.encoding().id() == CHUNKED_LAYOUT_ID
150+
parent.encoding().id() == STATS_LAYOUT_ID
152151
&& parent.layout().metadata().is_some()
153152
&& self.path.last().copied().unwrap_or_default() == (parent.layout().nchildren() - 1)
154153
}

‎vortex-tui/src/browse/ui/layouts.rs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,13 @@ fn render_layout_header(cursor: &LayoutCursor, area: Rect, buf: &mut Buffer) {
6262
)));
6363
}
6464

65-
if cursor.encoding().id() == CHUNKED_LAYOUT_ID {
65+
if cursor.encoding().id() == STATS_LAYOUT_ID {
6666
// Push any columnar stats.
67-
if let Some(metadata) = cursor.layout().metadata() {
68-
let available_stats = stats_from_bitset_bytes(&metadata);
67+
if let Some(available_stats) = cursor
68+
.layout()
69+
.metadata()
70+
.map(|metadata| stats_from_bitset_bytes(&metadata[4..]))
71+
{
6972
let mut line = String::new();
7073
line.push_str("Statistics: ");
7174
for stat in available_stats {

‎vortex-tui/src/main.rs‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,15 @@ enum Commands {
3939
Browse { file: PathBuf },
4040
}
4141

42-
fn main() {
42+
fn main() -> anyhow::Result<()> {
4343
let cli = Cli::parse();
4444
match cli.command {
45-
Commands::Tree { file } => TOKIO_RUNTIME.block_on(exec_tree(file)).expect("exec_tree"),
46-
Commands::Convert { file, quiet } => TOKIO_RUNTIME
47-
.block_on(exec_convert(file, Flags { quiet }))
48-
.expect("exec_convert"),
49-
Commands::Browse { file } => exec_tui(file).expect("exec_tui"),
50-
}
45+
Commands::Tree { file } => TOKIO_RUNTIME.block_on(exec_tree(file))?,
46+
Commands::Convert { file, quiet } => {
47+
TOKIO_RUNTIME.block_on(exec_convert(file, Flags { quiet }))?
48+
}
49+
Commands::Browse { file } => exec_tui(file)?,
50+
};
51+
52+
Ok(())
5153
}

0 commit comments

Comments
 (0)