Adding an gp_lttb to handle lttb on gappy data. #517
Conversation
extension/src/lttb.rs
Outdated
| Some(state) => state, | ||
| }; | ||
| state.series.sort_by_key(|point| point.ts); | ||
| let series = Cow::from(&state.series); |
There was a problem hiding this comment.
Why copy-on-write? It looks like we never write, so we never copy, so we could reference state.series directly. I looked at lttb_final_inner and it looks like that doesn't need it either.
There was a problem hiding this comment.
Good point, fixed in both places.
extension/src/lttb.rs
Outdated
| num_points: downsampled.len() as u32, | ||
| flags: time_vector::FLAG_IS_SORTED, | ||
| internal_padding: [0; 3], | ||
| points: (&*downsampled).into(), |
There was a problem hiding this comment.
Just downsampled.into() ought to work.
There was a problem hiding this comment.
It does, but it does move downsampled now. Luckily I just have to move the following line before this to keep this from being an issue.
WireBaron
force-pushed
the
br/irregular_lttb
branch
from
2d6031d to
21ee5fc
Compare
WireBaron
force-pushed
the
br/irregular_lttb
branch
from
21ee5fc to
aa1df04
Compare
| let state = unsafe { state.to_inner() }; | ||
| let needs_interval = state.is_none(); | ||
|
|
||
| // Don't love this code, but need to compute gap_val if needed before time is moved |
There was a problem hiding this comment.
Oh wow. That's just a silly omission on TimestampTz, which is a simple wrapper around Datum, which is just a usize. We should derive Copy on it. Not right now, just FYI :)
There was a problem hiding this comment.
I can see why they were reluctant to define copy on datum, since a datum may actually be a pointer pretending to be a usize. However it might be nice to implement a utility to give us a shallow copy, that we can use for things like timestamptz (which is legitimately a usize).
This change adds a new gp_lttb aggregate to the toolkit_experimental schema. This aggregate will identify any large gaps in the data and will perform a piecemeal lttb on the data between the gaps.
WireBaron
force-pushed
the
br/irregular_lttb
branch
from
d77b84a to
3341d91
Compare
WireBaron
changed the title
|
bors r+ |
|
Build succeeded: |
This change adds a new gp_lttb aggregate to the toolkit_experimental
schema. This aggregate will identify any large gaps in the data and
will perform a piecemeal lttb on the data between the gaps.