Skip to content

Commit f697f0b

Browse files
bors[bot]syvb
andauthored
Merge #715
715: Fix errors in state_agg rollup r=Smittyvb a=Smittyvb This PR should fix out-of-bounds indexing in `rollup`: - before, `last_state` in the merged aggregate was computed incorrectly; it is now determined correctly - merging aggregates now sorts them first It also improves the various error messages that could arise when using rollup. Co-authored-by: Smitty <[email protected]> Co-authored-by: syvb <[email protected]> Co-authored-by: Smittyvb <[email protected]>
2 parents 96678c5 + 5e081e0 commit f697f0b

File tree

3 files changed

+194
-33
lines changed

3 files changed

+194
-33
lines changed

Changelog.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ This changelog should be updated as part of a PR if the work is worth noting (mo
99
#### New experimental features
1010

1111
#### Bug fixes
12+
- [#715](https://github.com/timescale/timescaledb-toolkit/pull/715): Fix out-of-bounds indexing error in `state_agg` rollup
1213

1314
#### Other notable changes
1415

extension/src/state_aggregate.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,13 +104,20 @@ impl StateEntry {
104104
if self.a == i64::MAX {
105105
MaterializedState::Integer(self.b)
106106
} else {
107-
MaterializedState::String(states[self.a as usize..self.b as usize].to_string())
107+
MaterializedState::String(
108+
states
109+
.get(self.a as usize..self.b as usize)
110+
.expect("tried to materialize out-of-bounds state")
111+
.to_string(),
112+
)
108113
}
109114
}
110115

111116
fn as_str(self, states: &str) -> &str {
112117
assert!(self.a != i64::MAX, "Tried to get non-string state");
113-
&states[self.a as usize..self.b as usize]
118+
states
119+
.get(self.a as usize..self.b as usize)
120+
.expect("tried to stringify out-of-bounds state")
114121
}
115122

116123
fn as_integer(self) -> i64 {

extension/src/state_aggregate/rollup.rs

Lines changed: 184 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ pub struct RollupTransState {
5656
compact: bool,
5757
}
5858

59-
#[derive(Debug, Clone, Serialize, Deserialize)]
59+
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
6060
struct OwnedCompactStateAgg {
6161
durations: Vec<DurationInState>,
6262
combined_durations: Vec<TimeInState>,
@@ -80,14 +80,22 @@ impl OwnedCompactStateAgg {
8080
"can't merge aggs with different state types"
8181
);
8282

83-
let (earlier, later) = match self.first_time.cmp(&other.first_time) {
83+
let (earlier, later) = match self.cmp(&other) {
8484
Ordering::Less => (self, other),
8585
Ordering::Greater => (other, self),
86-
Ordering::Equal => panic!("can't merge overlapping aggregates (same start time)"),
86+
Ordering::Equal => panic!(
87+
"can't merge overlapping aggregates (same start time: {})",
88+
self.first_time
89+
),
8790
};
91+
8892
assert!(
8993
earlier.last_time <= later.first_time,
90-
"can't merge overlapping aggregates"
94+
"can't merge overlapping aggregates (earlier={}-{}, later={}-{})",
95+
earlier.first_time,
96+
earlier.last_time,
97+
later.first_time,
98+
later.last_time,
9199
);
92100
assert_ne!(
93101
later.durations.len(),
@@ -97,7 +105,7 @@ impl OwnedCompactStateAgg {
97105
assert_ne!(
98106
earlier.durations.len(),
99107
0,
100-
"later aggregate must be non-empty"
108+
"earlier aggregate must be non-empty"
101109
);
102110

103111
let later_states =
@@ -108,25 +116,37 @@ impl OwnedCompactStateAgg {
108116

109117
let earlier_len = earlier.combined_durations.len();
110118

111-
let mut added_entries = 0;
112-
for dis in later.durations.iter() {
113-
let merged_duration_to_update = merged_durations.iter_mut().find(|merged_dis| {
114-
merged_dis.state.materialize(&merged_states) == dis.state.materialize(&later_states)
115-
});
116-
if let Some(merged_duration_to_update) = merged_duration_to_update {
117-
merged_duration_to_update.duration += dis.duration;
118-
} else {
119-
let state = dis
120-
.state
121-
.materialize(&later_states)
122-
.entry(&mut merged_states);
123-
merged_durations.push(DurationInState {
124-
state,
125-
duration: dis.duration,
126-
});
127-
added_entries += 1;
119+
let mut merged_last_state = None;
120+
for (later_idx, dis) in later.durations.iter().enumerate() {
121+
let materialized_dis = dis.state.materialize(&later_states);
122+
let merged_duration_info =
123+
merged_durations
124+
.iter_mut()
125+
.enumerate()
126+
.find(|(_, merged_dis)| {
127+
merged_dis.state.materialize(&merged_states) == materialized_dis
128+
});
129+
130+
let merged_idx =
131+
if let Some((merged_idx, merged_duration_to_update)) = merged_duration_info {
132+
merged_duration_to_update.duration += dis.duration;
133+
merged_idx
134+
} else {
135+
let state = materialized_dis.entry(&mut merged_states);
136+
merged_durations.push(DurationInState {
137+
state,
138+
duration: dis.duration,
139+
});
140+
merged_durations.len() - 1
141+
};
142+
143+
if later_idx == later.last_state as usize {
144+
// this is the last state
145+
merged_last_state = Some(merged_idx);
128146
};
129147
}
148+
let merged_last_state =
149+
merged_last_state.expect("later last_state not in later.durations") as u32;
130150

131151
let mut combined_durations = earlier
132152
.combined_durations
@@ -142,22 +162,36 @@ impl OwnedCompactStateAgg {
142162

143163
let gap = later.first_time - earlier.last_time;
144164
assert!(gap >= 0);
145-
merged_durations[earlier.last_state as usize].duration += gap;
165+
merged_durations
166+
.get_mut(earlier.last_state as usize)
167+
.expect("earlier.last_state doesn't point to a state")
168+
.duration += gap;
146169

147170
// ensure combined_durations covers the whole range of time
148171
if !earlier.compact {
149-
if combined_durations[earlier_len - 1]
172+
if combined_durations
173+
.get_mut(earlier_len - 1)
174+
.expect("invalid combined_durations: nothing at end of earlier")
150175
.state
151176
.materialize(&merged_states)
152-
== combined_durations[earlier_len]
177+
== combined_durations
178+
.get(earlier_len)
179+
.expect("invalid combined_durations: nothing at start of earlier")
153180
.state
154181
.materialize(&merged_states)
155182
{
156-
combined_durations[earlier_len - 1].end_time =
157-
combined_durations.remove(earlier_len).end_time;
183+
combined_durations
184+
.get_mut(earlier_len - 1)
185+
.expect("invalid combined_durations (nothing at earlier_len - 1, equal)")
186+
.end_time = combined_durations.remove(earlier_len).end_time;
158187
} else {
159-
combined_durations[earlier_len - 1].end_time =
160-
combined_durations[earlier_len].start_time;
188+
combined_durations
189+
.get_mut(earlier_len - 1)
190+
.expect("invalid combined_durations (nothing at earlier_len - 1, not equal)")
191+
.end_time = combined_durations
192+
.get(earlier_len)
193+
.expect("invalid combined_durations (nothing at earlier_len, not equal)")
194+
.start_time;
161195
}
162196
}
163197

@@ -169,8 +203,8 @@ impl OwnedCompactStateAgg {
169203

170204
first_time: earlier.first_time,
171205
last_time: later.last_time,
172-
first_state: earlier.first_state,
173-
last_state: added_entries + later.last_state,
206+
first_state: earlier.first_state, // indexes into earlier durations are same for merged_durations
207+
last_state: merged_last_state,
174208

175209
// these values are always the same for both
176210
compact: earlier.compact,
@@ -216,8 +250,23 @@ impl<'a> From<CompactStateAgg<'a>> for OwnedCompactStateAgg {
216250
}
217251
}
218252

253+
impl PartialOrd for OwnedCompactStateAgg {
254+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
255+
Some(self.cmp(other))
256+
}
257+
}
258+
259+
impl Ord for OwnedCompactStateAgg {
260+
fn cmp(&self, other: &Self) -> Ordering {
261+
// compare using first time (OwnedCompactStateAgg::merge will handle any overlap)
262+
self.first_time.cmp(&other.first_time)
263+
}
264+
}
265+
219266
impl RollupTransState {
220267
fn merge(&mut self) {
268+
// OwnedCompactStateAgg::merge can't merge overlapping aggregates
269+
self.values.sort();
221270
self.values = self
222271
.values
223272
.drain(..)
@@ -417,4 +466,108 @@ mod tests {
417466

418467
r2.merge(r1);
419468
}
469+
470+
#[test]
471+
fn merges_compact_aggs_correctly() {
472+
let s1 = OwnedCompactStateAgg {
473+
durations: vec![
474+
DurationInState {
475+
duration: 500,
476+
state: StateEntry::from_integer(555_2),
477+
},
478+
DurationInState {
479+
duration: 400,
480+
state: StateEntry::from_integer(555_1),
481+
},
482+
],
483+
combined_durations: vec![],
484+
first_time: 100,
485+
last_time: 1000,
486+
first_state: 1,
487+
last_state: 0,
488+
states: vec![],
489+
compact: true,
490+
integer_states: true,
491+
};
492+
let s2 = OwnedCompactStateAgg {
493+
durations: vec![
494+
DurationInState {
495+
duration: 500,
496+
state: StateEntry::from_integer(555_2),
497+
},
498+
DurationInState {
499+
duration: 400,
500+
state: StateEntry::from_integer(555_1),
501+
},
502+
],
503+
combined_durations: vec![],
504+
first_time: 1000 + 12345,
505+
last_time: 1900 + 12345,
506+
first_state: 1,
507+
last_state: 0,
508+
states: vec![],
509+
compact: true,
510+
integer_states: true,
511+
};
512+
let s3 = OwnedCompactStateAgg {
513+
durations: vec![
514+
DurationInState {
515+
duration: 500,
516+
state: StateEntry::from_integer(555_2),
517+
},
518+
DurationInState {
519+
duration: 400,
520+
state: StateEntry::from_integer(555_1),
521+
},
522+
],
523+
combined_durations: vec![],
524+
first_time: 1900 + 12345,
525+
last_time: 1900 + 12345 + 900,
526+
first_state: 1,
527+
last_state: 0,
528+
states: vec![],
529+
compact: true,
530+
integer_states: true,
531+
};
532+
let expected = OwnedCompactStateAgg {
533+
durations: vec![
534+
DurationInState {
535+
duration: 500 * 3 + 12345,
536+
state: StateEntry::from_integer(555_2),
537+
},
538+
DurationInState {
539+
duration: 400 * 3,
540+
state: StateEntry::from_integer(555_1),
541+
},
542+
],
543+
combined_durations: vec![],
544+
first_time: 100,
545+
last_time: 1900 + 12345 + 900,
546+
first_state: 1,
547+
last_state: 0,
548+
states: vec![],
549+
compact: true,
550+
integer_states: true,
551+
};
552+
let merged = s1.clone().merge(s2.clone().merge(s3.clone()));
553+
assert_eq!(merged, expected);
554+
let merged = s3.clone().merge(s2.clone().merge(s1.clone()));
555+
assert_eq!(merged, expected);
556+
557+
let mut trans_state = RollupTransState {
558+
values: vec![s1.clone(), s2.clone(), s3.clone()],
559+
compact: true,
560+
};
561+
trans_state.merge();
562+
assert_eq!(trans_state.values.len(), 1);
563+
assert_eq!(trans_state.values[0], expected.clone());
564+
565+
let mut trans_state = RollupTransState {
566+
values: vec![s3.clone(), s1.clone(), s2.clone()],
567+
compact: true,
568+
};
569+
trans_state.merge();
570+
assert_eq!(trans_state.values.len(), 1);
571+
assert_eq!(trans_state.values[0], expected.clone());
572+
}
420573
}

0 commit comments

Comments
 (0)