mirror of
https://github.com/GreptimeTeam/greptimedb.git
synced 2026-10-12 06:52:34 +00:00
fix(promql): accept evaluation times before the Unix epoch (#9480)
* fix(promql): accept evaluation times before the Unix epoch Prometheus accepts negative timestamps in query parameters; GreptimeDB rejected them while parsing and would panic converting them in the planner. Signed-off-by: Dennis Zhuang <killme2008@gmail.com> * test: update flow_tql error for a negative TQL end time A negative end now parses and fails the end >= start check instead of the timestamp conversion. Signed-off-by: Dennis Zhuang <killme2008@gmail.com> * fix(promql): reject evaluation times outside the millisecond range The signed millisecond conversion cast u128 to i64 unchecked, so a far-past time wrapped into the future. Reuse the checked conversion from label_values and return an error instead. Signed-off-by: Dennis Zhuang <killme2008@gmail.com> * chore: drop an unused import Signed-off-by: Dennis Zhuang <killme2008@gmail.com> * fix(promql): round query times to milliseconds like Prometheus Numeric times round the fraction of a second to milliseconds, and pre-epoch times floor sub-millisecond parts, matching Prometheus' parseTime and timestamp.FromTime. Signed-off-by: Dennis Zhuang <killme2008@gmail.com> * fix(promql): round the float fraction before converting to nanoseconds Signed-off-by: Dennis Zhuang <killme2008@gmail.com> * test: skip the overflow case where SystemTime cannot hold it Signed-off-by: Dennis Zhuang <killme2008@gmail.com> --------- Signed-off-by: Dennis Zhuang <killme2008@gmail.com>
This commit is contained in:
+50
-5
@@ -224,11 +224,31 @@ impl QueryLanguageParser {
|
||||
// also report rfc3339 error if float parsing fails
|
||||
.map_err(|_| rfc3339_result.unwrap_err())?;
|
||||
|
||||
let duration =
|
||||
Duration::try_from_secs_f64(secs).context(TryIntoDurationSnafu { raw: timestamp })?;
|
||||
SystemTime::UNIX_EPOCH
|
||||
.checked_add(duration)
|
||||
.context(AddSystemTimeOverflowSnafu { duration })
|
||||
// Prometheus accepts timestamps before the Unix epoch. Follow the arithmetic of its
|
||||
// `parseTime` and `timestamp.FromTime` step by step: the float fraction is rounded to
|
||||
// milliseconds before any conversion to nanoseconds, which decides boundary values
|
||||
// like 1.0005, whose fraction is just below 0.0005.
|
||||
let magnitude = Duration::try_from_secs_f64(secs.abs())
|
||||
.context(TryIntoDurationSnafu { raw: timestamp })?;
|
||||
let whole = secs.trunc();
|
||||
let fraction = ((secs - whole) * 1000.0).round() / 1000.0;
|
||||
let nanos = whole as i128 * 1_000_000_000 + (fraction * 1e9) as i128;
|
||||
let millis = nanos.div_euclid(1_000_000);
|
||||
let duration = u64::try_from(millis.unsigned_abs())
|
||||
.ok()
|
||||
.map(Duration::from_millis)
|
||||
.context(AddSystemTimeOverflowSnafu {
|
||||
duration: magnitude,
|
||||
})?;
|
||||
if millis < 0 {
|
||||
SystemTime::UNIX_EPOCH
|
||||
.checked_sub(duration)
|
||||
.context(AddSystemTimeOverflowSnafu { duration })
|
||||
} else {
|
||||
SystemTime::UNIX_EPOCH
|
||||
.checked_add(duration)
|
||||
.context(AddSystemTimeOverflowSnafu { duration })
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -380,6 +400,31 @@ mod test {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_promql_timestamp_matches_prometheus_millis() {
|
||||
// Expected values are Prometheus' `timestamp.FromTime(parseTime(input))`.
|
||||
for (input, millis) in [
|
||||
("-30000.5", -30_000_500),
|
||||
("-0.001", -1),
|
||||
("-0.0006", -1),
|
||||
("-1.2346", -1235),
|
||||
("1.2346", 1235),
|
||||
("0.0004", 0),
|
||||
("1.0005", 1000),
|
||||
("-1.0005", -1000),
|
||||
("0.0004999999", 0),
|
||||
("1969-12-31T23:59:59.9999Z", -1),
|
||||
("1970-01-01T00:00:00.0009Z", 0),
|
||||
] {
|
||||
let result = QueryLanguageParser::parse_promql_timestamp(input).unwrap();
|
||||
assert_eq!(
|
||||
crate::promql::label_values::signed_millis_since_epoch(result).unwrap(),
|
||||
millis,
|
||||
"{input}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_promql_timestamp() {
|
||||
let cases = vec![
|
||||
|
||||
@@ -33,13 +33,20 @@ use crate::promql::error::{
|
||||
/// a negative duration, and an RFC3339 query parameter can name one, so the sign
|
||||
/// is recovered here instead of unwrapping.
|
||||
fn millis_since_epoch(time: SystemTime) -> Result<Timestamp> {
|
||||
Ok(Timestamp::new_millisecond(signed_millis_since_epoch(time)?))
|
||||
}
|
||||
|
||||
/// Converts a [`SystemTime`] to signed milliseconds since the Unix epoch, rejecting instants
|
||||
/// outside the `i64` millisecond range.
|
||||
///
|
||||
/// Sub-millisecond parts round down, also before the epoch, like Prometheus'
|
||||
/// `timestamp.FromTime`.
|
||||
pub(crate) fn signed_millis_since_epoch(time: SystemTime) -> Result<i64> {
|
||||
let (millis, before_epoch) = match time.duration_since(UNIX_EPOCH) {
|
||||
Ok(duration) => (duration.as_millis(), false),
|
||||
Err(earlier) => (earlier.duration().as_millis(), true),
|
||||
Err(earlier) => (earlier.duration().as_nanos().div_ceil(1_000_000), true),
|
||||
};
|
||||
let millis =
|
||||
signed_millis(millis, before_epoch).with_context(|| SystemTimeOutOfRangeSnafu { time })?;
|
||||
Ok(Timestamp::new_millisecond(millis))
|
||||
signed_millis(millis, before_epoch).with_context(|| SystemTimeOutOfRangeSnafu { time })
|
||||
}
|
||||
|
||||
/// Converts a millisecond magnitude and epoch direction without platform time limits.
|
||||
|
||||
@@ -21,7 +21,6 @@ mod set_operator;
|
||||
|
||||
use std::collections::{BTreeSet, HashSet, VecDeque};
|
||||
use std::sync::Arc;
|
||||
use std::time::UNIX_EPOCH;
|
||||
|
||||
use arrow::datatypes::IntervalDayTime;
|
||||
use async_recursion::async_recursion;
|
||||
@@ -112,6 +111,7 @@ use crate::promql::error::{
|
||||
UnknownTableSnafu, UnsupportedExprSnafu, UnsupportedMatcherOpSnafu, ValueNotFoundSnafu,
|
||||
ZeroRangeSelectorSnafu,
|
||||
};
|
||||
use crate::promql::label_values::signed_millis_since_epoch;
|
||||
use crate::query_engine::QueryEngineState;
|
||||
|
||||
/// `time()` function in PromQL.
|
||||
@@ -241,10 +241,11 @@ impl BinaryResultLabels {
|
||||
}
|
||||
|
||||
impl PromPlannerContext {
|
||||
fn from_eval_stmt(stmt: &EvalStmt) -> Self {
|
||||
let start = stmt.start.duration_since(UNIX_EPOCH).unwrap().as_millis() as Millisecond;
|
||||
let end = stmt.end.duration_since(UNIX_EPOCH).unwrap().as_millis() as Millisecond;
|
||||
Self {
|
||||
fn from_eval_stmt(stmt: &EvalStmt) -> Result<Self> {
|
||||
// Evaluation times before the Unix epoch are valid in Prometheus.
|
||||
let start = signed_millis_since_epoch(stmt.start)?;
|
||||
let end = signed_millis_since_epoch(stmt.end)?;
|
||||
Ok(Self {
|
||||
start,
|
||||
end,
|
||||
interval: stmt.interval.as_millis() as _,
|
||||
@@ -252,7 +253,7 @@ impl PromPlannerContext {
|
||||
stmt_start: start,
|
||||
stmt_end: end,
|
||||
..Default::default()
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
/// Reset all planner states
|
||||
@@ -309,7 +310,7 @@ impl PromPlanner {
|
||||
) -> Result<LogicalPlan> {
|
||||
let mut planner = Self {
|
||||
table_provider,
|
||||
ctx: PromPlannerContext::from_eval_stmt(stmt),
|
||||
ctx: PromPlannerContext::from_eval_stmt(stmt)?,
|
||||
promql_annotations,
|
||||
};
|
||||
|
||||
|
||||
@@ -13,7 +13,7 @@
|
||||
// limitations under the License.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::time::{Duration, UNIX_EPOCH};
|
||||
use std::time::{Duration, SystemTime, UNIX_EPOCH};
|
||||
|
||||
use catalog::RegisterTableRequest;
|
||||
use catalog::memory::{MemoryCatalogManager, new_memory_catalog_manager};
|
||||
@@ -3297,7 +3297,7 @@ async fn comparison_binary_join_uses_tsid_and_keeps_it_in_filtered_result() {
|
||||
.await;
|
||||
let mut planner = PromPlanner {
|
||||
table_provider,
|
||||
ctx: PromPlannerContext::from_eval_stmt(&eval_stmt),
|
||||
ctx: PromPlannerContext::from_eval_stmt(&eval_stmt).unwrap(),
|
||||
promql_annotations: None,
|
||||
};
|
||||
let plan = planner
|
||||
@@ -6266,7 +6266,7 @@ async fn native_scan_bounds_preserve_zero_lookback_and_overflow() {
|
||||
.await;
|
||||
let mut planner = PromPlanner {
|
||||
table_provider,
|
||||
ctx: PromPlannerContext::from_eval_stmt(&build_eval_stmt("some_metric")),
|
||||
ctx: PromPlannerContext::from_eval_stmt(&build_eval_stmt("some_metric")).unwrap(),
|
||||
promql_annotations: None,
|
||||
};
|
||||
planner.ctx.time_index_column = Some("timestamp".to_string());
|
||||
@@ -8872,3 +8872,31 @@ async fn test_count_values_groups_by_formatted_value_for_bigint_input() {
|
||||
vec![("9007199254740992", 2.0)]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn eval_times_before_epoch() {
|
||||
let plan = |start: SystemTime, end: SystemTime| async move {
|
||||
let mut eval_stmt = build_eval_stmt("some_metric");
|
||||
eval_stmt.start = start;
|
||||
eval_stmt.end = end;
|
||||
let table_provider = build_test_table_provider(
|
||||
&[(DEFAULT_SCHEMA_NAME.to_string(), "some_metric".to_string())],
|
||||
1,
|
||||
1,
|
||||
)
|
||||
.await;
|
||||
PromPlanner::stmt_to_plan(table_provider, &eval_stmt, &build_query_engine_state()).await
|
||||
};
|
||||
|
||||
plan(
|
||||
UNIX_EPOCH - Duration::from_secs(30_000),
|
||||
UNIX_EPOCH - Duration::from_secs(29_000),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
// Beyond the i64 millisecond range, which must not wrap into the future. Windows cannot
|
||||
// represent this SystemTime.
|
||||
if let Some(too_early) = UNIX_EPOCH.checked_sub(Duration::from_secs(10_000_000_000_000_000)) {
|
||||
assert!(plan(too_early, too_early).await.is_err());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -155,7 +155,7 @@ Error: 2000(InvalidSyntax), Invalid TQL syntax: sql parser error: Expected expre
|
||||
CREATE FLOW calc_reqs SINK TO cnt_reqs EVAL INTERVAL '1m' AS
|
||||
TQL EVAL (now() - now(), now()-(now()+'15s'::interval), '5s') count_values("status_code", http_requests);
|
||||
|
||||
Error: 3001(EngineExecuteQuery), Failed to convert float seconds to duration, raw: -15
|
||||
Error: 3001(EngineExecuteQuery), The end time must be greater than start time, start: 0, end: -15000
|
||||
|
||||
CREATE FLOW calc_reqs SINK TO cnt_reqs EVAL INTERVAL '1m' AS
|
||||
TQL EVAL (now() - now(), now()-now()+'15s'::interval, '5s') count_values("status_code", http_requests);
|
||||
|
||||
@@ -155,7 +155,7 @@ Error: 2000(InvalidSyntax), Invalid TQL syntax: sql parser error: Expected expre
|
||||
CREATE FLOW calc_reqs SINK TO cnt_reqs EVAL INTERVAL '1m' AS
|
||||
TQL EVAL (now() - now(), now()-(now()+'15s'::interval), '5s') count_values("status_code", http_requests);
|
||||
|
||||
Error: 3001(EngineExecuteQuery), Failed to convert float seconds to duration, raw: -15: cannot convert float seconds to Duration: value is negative
|
||||
Error: 3001(EngineExecuteQuery), The end time must be greater than start time, start: 0, end: -15000
|
||||
|
||||
CREATE FLOW calc_reqs SINK TO cnt_reqs EVAL INTERVAL '1m' AS
|
||||
TQL EVAL (now() - now(), now()-now()+'15s'::interval, '5s') count_values("status_code", http_requests);
|
||||
|
||||
Reference in New Issue
Block a user