Skip to content

Commit d6d81d5

Browse files
committed
fix(timeseries): repair numeric aggregates and time_bucket() after nodedb-sql migration
Fixes #15. The aggregate executor receives (op, field) pairs but all three PhysicalTask construction sites were passing the alias instead of the source field name, so the executor could never locate the column and returned null for every MIN/MAX/SUM/AVG result. parse_interval_to_ms only handled compact suffixes ("1h", "15m") and rejected the word-form intervals that DataFusion normalises SQL INTERVAL literals into ("1 hour", "15 minutes"), causing time_bucket() to bucket everything into epoch-zero and return an empty result set. Changes: - sql_plan_convert: extract agg_expr_to_pair() helper that reads the first argument expression (Column name or Wildcard) and use it at all three PhysicalTask construction sites instead of a.alias - aggregate: extend parse_interval_to_ms to accept singular and plural word-form units (hour/hours, minute/minutes, second/seconds, day/days) alongside the existing compact forms; bare numbers are treated as seconds; unrecognised units return 0 instead of silently mis-routing - aggregate: add unit tests covering word-form parsing
1 parent 8588cf8 commit d6d81d5

2 files changed

Lines changed: 50 additions & 23 deletions

File tree

nodedb-sql/src/planner/aggregate.rs

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -133,23 +133,33 @@ fn extract_bucket_interval(func: &ast::Function) -> Result<i64> {
133133
Ok(parse_interval_to_ms(&interval_str))
134134
}
135135

136-
/// Parse an interval string like "1h", "15m", "30s", "1d" to milliseconds.
136+
/// Parse an interval string to milliseconds.
137+
///
138+
/// Accepted forms: `"1h"`, `"15m"`, `"30s"`, `"1d"`, `"1 hour"`, `"15 minutes"`,
139+
/// `"30 seconds"`, `"7 days"`. Plural and singular word forms both work.
137140
fn parse_interval_to_ms(s: &str) -> i64 {
138141
let s = s.trim();
139142
if s.is_empty() {
140143
return 0;
141144
}
142-
let (num_str, suffix) = s.split_at(s.len() - 1);
143-
let num: i64 = num_str.trim().parse().unwrap_or(0);
144-
match suffix {
145-
"s" => num * 1_000,
146-
"m" => num * 60_000,
147-
"h" => num * 3_600_000,
148-
"d" => num * 86_400_000,
149-
_ => {
150-
// Try full string as seconds.
151-
s.parse::<i64>().unwrap_or(0) * 1_000
145+
146+
// Split into numeric part and unit part (handles both "1h" and "1 hour").
147+
let num_end = s
148+
.find(|c: char| !c.is_ascii_digit() && c != '.')
149+
.unwrap_or(s.len());
150+
let num: i64 = s[..num_end].trim().parse().unwrap_or(0);
151+
let unit = s[num_end..].trim();
152+
153+
match unit {
154+
"s" | "sec" | "second" | "seconds" => num * 1_000,
155+
"m" | "min" | "minute" | "minutes" => num * 60_000,
156+
"h" | "hr" | "hour" | "hours" => num * 3_600_000,
157+
"d" | "day" | "days" => num * 86_400_000,
158+
"" => {
159+
// Bare number — treat as seconds.
160+
num * 1_000
152161
}
162+
_ => 0,
153163
}
154164
}
155165

@@ -260,5 +270,12 @@ mod tests {
260270
assert_eq!(parse_interval_to_ms("15m"), 900_000);
261271
assert_eq!(parse_interval_to_ms("30s"), 30_000);
262272
assert_eq!(parse_interval_to_ms("7d"), 604_800_000);
273+
// Word-form intervals.
274+
assert_eq!(parse_interval_to_ms("1 hour"), 3_600_000);
275+
assert_eq!(parse_interval_to_ms("2 hours"), 7_200_000);
276+
assert_eq!(parse_interval_to_ms("15 minutes"), 900_000);
277+
assert_eq!(parse_interval_to_ms("30 seconds"), 30_000);
278+
assert_eq!(parse_interval_to_ms("1 day"), 86_400_000);
279+
assert_eq!(parse_interval_to_ms("5 min"), 300_000);
263280
}
264281
}

nodedb/src/control/planner/sql_plan_convert.rs

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -249,10 +249,8 @@ fn convert_one(
249249
tiered,
250250
} => {
251251
let filter_bytes = serialize_filters(filters)?;
252-
let agg_pairs: Vec<(String, String)> = aggregates
253-
.iter()
254-
.map(|a| (a.function.clone(), a.alias.clone()))
255-
.collect();
252+
let agg_pairs: Vec<(String, String)> =
253+
aggregates.iter().map(agg_expr_to_pair).collect();
256254

257255
// AUTO_TIER: split query across retention tiers if enabled.
258256
if *tiered
@@ -667,10 +665,7 @@ fn convert_aggregate(
667665
let vshard = VShardId::from_collection(&left_collection);
668666

669667
let group_strs = group_by_to_strings(group_by);
670-
let agg_pairs = aggregates
671-
.iter()
672-
.map(|a| (a.function.clone(), a.alias.clone()))
673-
.collect();
668+
let agg_pairs = aggregates.iter().map(agg_expr_to_pair).collect();
674669

675670
return Ok(vec![PhysicalTask {
676671
tenant_id,
@@ -700,10 +695,7 @@ fn convert_aggregate(
700695
let having_bytes = serialize_filters(having)?;
701696

702697
let group_strs = group_by_to_strings(group_by);
703-
let agg_pairs = aggregates
704-
.iter()
705-
.map(|a| (a.function.clone(), a.alias.clone()))
706-
.collect();
698+
let agg_pairs = aggregates.iter().map(agg_expr_to_pair).collect();
707699

708700
Ok(vec![PhysicalTask {
709701
tenant_id,
@@ -734,6 +726,24 @@ fn extract_collection_name(plan: &SqlPlan) -> String {
734726
}
735727
}
736728

729+
/// Convert an `AggregateExpr` to the `(op, field)` pair the executor expects.
730+
///
731+
/// The field is extracted from the first argument (e.g. `elapsed_ms` from
732+
/// `AVG(elapsed_ms)`). Wildcard args produce `"*"`. Falls back to `"*"` when
733+
/// there are no arguments (bare `COUNT`).
734+
fn agg_expr_to_pair(a: &AggregateExpr) -> (String, String) {
735+
let field = a
736+
.args
737+
.first()
738+
.map(|arg| match arg {
739+
SqlExpr::Column { name, .. } => name.clone(),
740+
SqlExpr::Wildcard => "*".into(),
741+
_ => format!("{arg:?}"),
742+
})
743+
.unwrap_or_else(|| "*".into());
744+
(a.function.clone(), field)
745+
}
746+
737747
fn group_by_to_strings(exprs: &[SqlExpr]) -> Vec<String> {
738748
exprs
739749
.iter()

0 commit comments

Comments
 (0)