Port TimeColumn to arrow-rs
#8638
Conversation
emilk
added
🏹 arrow
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
| if *times.data_type() != timeline.datatype() { | ||
| return Err(ChunkError::Malformed { | ||
| reason: format!( | ||
| "Time data for timeline {} has the wrong datatype: expected {:?} but got {:?} instead", | ||
| timeline.name(), | ||
| timeline.datatype(), | ||
| *times.data_type(), | ||
| ), | ||
| }); | ||
| } |
| let times = self | ||
| .times_raw() | ||
| .iter() | ||
| .chain(rhs.times_raw()) | ||
| .copied() | ||
| .collect_vec(); | ||
| let times = ArrowScalarBuffer::from(times); |
There was a problem hiding this comment.
Using Rust to do the concat instead of using a computation kernel.
There was a problem hiding this comment.
Any particular reason? Maybe worth a comment and/or TODO?
There was a problem hiding this comment.
To use the computational kernel I would have to convert the buffers into dynamic array, and then dynamic-cast them back again. A lot of work, but little to gain
There was a problem hiding this comment.
Ahh, I forgot the compute functions like concat don't support generics. Annoying.
| --- | ||
| [ | ||
| "1048576 scalars", | ||
| "37.1 MiB in total", | ||
| "37.0 MiB in total", |
| impl<T: ArrowNativeType> SizeBytes for ScalarBuffer<T> { | ||
| #[inline] | ||
| fn heap_size_bytes(&self) -> u64 { | ||
| self.inner().capacity() as _ |
There was a problem hiding this comment.
In most places we take .len() instead of .capacity(), but this isn't codified. What is the preference here @teh-cmc? I would think .capacity() is the more interesting thing, as that will catch failure to call .shrink_to_fit(), for instance 🤔
|
@rerun-bot full-check |
|
Started a full build: https://github.com/rerun-io/rerun/actions/runs/12710803994 |
|
@rerun-bot full-check |
|
Started a full build: https://github.com/rerun-io/rerun/actions/runs/12714439344 |
crates/store/re_chunk/src/chunk.rs
Outdated
| #[derive(Debug, thiserror::Error)] | ||
| pub enum TimeColumnError { | ||
| #[error("Time columns had nulls, but should be dense")] | ||
| Nulls, |
There was a problem hiding this comment.
Nit: ContainsNulls would be slightly more descriptive
| let times = self | ||
| .times_raw() | ||
| .iter() | ||
| .chain(rhs.times_raw()) | ||
| .copied() | ||
| .collect_vec(); | ||
| let times = ArrowScalarBuffer::from(times); |
There was a problem hiding this comment.
Any particular reason? Maybe worth a comment and/or TODO?
Related
re_arrow2toarrow#3741