From 819531393ffb434f751f063ebaa181421ca84de3 Mon Sep 17 00:00:00 2001 From: fys <40801205+fengys1996@users.noreply.github.com> Date: Wed, 20 Aug 2025 15:21:39 +0800 Subject: [PATCH] feat: disable month in trigger interval expr (#6774) * feat: disable month in trigger interval expr * fix: cargo clippy * fix: cargo clippy * add unit test * remove unused comment --- src/sql/src/parsers/create_parser.rs | 40 ---------------- src/sql/src/parsers/create_parser/trigger.rs | 49 +++++++++++++++++++- src/sql/src/parsers/utils.rs | 36 ++++++++++++++ 3 files changed, 84 insertions(+), 41 deletions(-) diff --git a/src/sql/src/parsers/create_parser.rs b/src/sql/src/parsers/create_parser.rs index 280e357f6d..b06ff1a0c7 100644 --- a/src/sql/src/parsers/create_parser.rs +++ b/src/sql/src/parsers/create_parser.rs @@ -16,7 +16,6 @@ pub mod trigger; use std::collections::HashMap; -use std::time::Duration; use arrow_buffer::IntervalMonthDayNano; use common_catalog::consts::default_engine; @@ -362,45 +361,6 @@ impl<'a> ParserContext<'a> { ) } - /// Parses an interval expression and converts it to a standard Rust [`Duration`] - /// and a raw interval expression string. - pub fn parse_interval_to_duration(&mut self) -> Result<(Duration, RawIntervalExpr)> { - let (interval, raw_interval_expr) = self.parse_interval_month_day_nano()?; - - let months: i64 = interval.months.into(); - let days: i64 = interval.days.into(); - let months_in_seconds: i64 = months * 60 * 60 * 24 * 3044 / 1000; - let days_in_seconds: i64 = days * 60 * 60 * 24; - let seconds_from_nanos = interval.nanoseconds / 1_000_000_000; - let total_seconds = months_in_seconds + days_in_seconds + seconds_from_nanos; - - let mut nanos_remainder = interval.nanoseconds % 1_000_000_000; - let mut adjusted_seconds = total_seconds; - - if nanos_remainder < 0 { - nanos_remainder += 1_000_000_000; - adjusted_seconds -= 1; - } - - ensure!( - adjusted_seconds >= 0, - InvalidIntervalSnafu { - reason: "must be a positive interval", - } - ); - - // Cast safety: `adjusted_seconds` is guaranteed to be non-negative before. - let adjusted_seconds = adjusted_seconds as u64; - // Cast safety: `nanos_remainder` is smaller than 1_000_000_000 which - // is checked above. - let nanos_remainder = nanos_remainder as u32; - - Ok(( - Duration::new(adjusted_seconds, nanos_remainder), - raw_interval_expr, - )) - } - /// Parse interval expr to [`IntervalMonthDayNano`]. fn parse_interval_month_day_nano(&mut self) -> Result<(IntervalMonthDayNano, RawIntervalExpr)> { let interval_expr = self.parser.parse_expr().context(error::SyntaxSnafu)?; diff --git a/src/sql/src/parsers/create_parser/trigger.rs b/src/sql/src/parsers/create_parser/trigger.rs index 7c6c89f4c4..a0b3ce587b 100644 --- a/src/sql/src/parsers/create_parser/trigger.rs +++ b/src/sql/src/parsers/create_parser/trigger.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::time::Duration; use snafu::{ensure, OptionExt, ResultExt}; use sqlparser::keywords::Keyword; @@ -8,6 +9,7 @@ use sqlparser::tokenizer::Token; use crate::error; use crate::error::Result; use crate::parser::ParserContext; +use crate::parsers::utils::convert_month_day_nano_to_duration; use crate::statements::create::trigger::{ AlertManagerWebhook, ChannelType, CreateTrigger, NotifyChannel, TriggerOn, }; @@ -120,6 +122,12 @@ impl<'a> ParserContext<'a> { /// /// - `is_first_keyword_matched`: indicates whether the first keyword `ON` /// has been matched. + /// + /// ## Notes + /// + /// - The months in the interval expression is prohibited. + /// - The interval must be at least 1 second. If the parsed interval is less + /// than 1 second, **it will be adjusted to 1 second**. pub(crate) fn parse_trigger_on(&mut self, is_first_keyword_matched: bool) -> Result { if !is_first_keyword_matched { if let Token::Word(w) = self.parser.peek_token().token @@ -141,7 +149,25 @@ impl<'a> ParserContext<'a> { return self.expected("`EVERY` keyword", self.parser.peek_token()); } - let (interval, raw_interval_expr) = self.parse_interval_to_duration()?; + let (month_day_nano, raw_interval_expr) = self.parse_interval_month_day_nano()?; + + // Trigger Interval (month_day_nano): the months field is prohibited, + // as the length of a month is ambiguous. + ensure!( + month_day_nano.months == 0, + error::InvalidIntervalSnafu { + reason: "year and month is not supported in trigger interval".to_string() + } + ); + + let interval = convert_month_day_nano_to_duration(month_day_nano)?; + + // Ensure the interval is at least 1 second. + let interval = if interval < Duration::from_secs(1) { + Duration::from_secs(1) + } else { + interval + }; Ok(TriggerOn { query, @@ -515,6 +541,27 @@ IF NOT EXISTS cpu_monitor let sql = "ON (SELECT * cpu_usage) EVERY '5 minute'::INTERVAL"; let mut ctx = ParserContext::new(&GreptimeDbDialect {}, sql).unwrap(); assert!(ctx.parse_trigger_on(false).is_err()); + + // Invalid, since year is not allowed in trigger interval. + let sql = "ON (SELECT * FROM cpu_usage) EVERY '1 year'::INTERVAL"; + let mut ctx = ParserContext::new(&GreptimeDbDialect {}, sql).unwrap(); + assert!(ctx.parse_trigger_on(false).is_err()); + + // Invalid, since month is not allowed in trigger interval. + let sql = "ON (SELECT * FROM cpu_usage) EVERY '1 month'::INTERVAL"; + let mut ctx = ParserContext::new(&GreptimeDbDialect {}, sql).unwrap(); + assert!(ctx.parse_trigger_on(false).is_err()); + + // Invalid, since the year and month are not allowed in trigger interval. + let sql = "ON (SELECT * FROM cpu_usage) EVERY '1 year 1 month'::INTERVAL"; + let mut ctx = ParserContext::new(&GreptimeDbDialect {}, sql).unwrap(); + assert!(ctx.parse_trigger_on(false).is_err()); + + // Valid, but the interval is less than 1 second, it will be adjusted to 1 second. + let sql = "ON (SELECT * FROM cpu_usage) EVERY '1ms'::INTERVAL"; + let mut ctx = ParserContext::new(&GreptimeDbDialect {}, sql).unwrap(); + let trigger_on = ctx.parse_trigger_on(false).unwrap(); + assert_eq!(trigger_on.interval, Duration::from_secs(1)); } #[test] diff --git a/src/sql/src/parsers/utils.rs b/src/sql/src/parsers/utils.rs index 84d9e65142..949261a416 100644 --- a/src/sql/src/parsers/utils.rs +++ b/src/sql/src/parsers/utils.rs @@ -141,3 +141,39 @@ pub fn validate_column_skipping_index_create_option(key: &str) -> bool { ] .contains(&key) } + +/// Convert an [`IntervalMonthDayNano`] to a [`Duration`]. +#[cfg(feature = "enterprise")] +pub fn convert_month_day_nano_to_duration( + interval: arrow_buffer::IntervalMonthDayNano, +) -> Result { + let months: i64 = interval.months.into(); + let days: i64 = interval.days.into(); + let months_in_seconds: i64 = months * 60 * 60 * 24 * 3044 / 1000; + let days_in_seconds: i64 = days * 60 * 60 * 24; + let seconds_from_nanos = interval.nanoseconds / 1_000_000_000; + let total_seconds = months_in_seconds + days_in_seconds + seconds_from_nanos; + + let mut nanos_remainder = interval.nanoseconds % 1_000_000_000; + let mut adjusted_seconds = total_seconds; + + if nanos_remainder < 0 { + nanos_remainder += 1_000_000_000; + adjusted_seconds -= 1; + } + + snafu::ensure!( + adjusted_seconds >= 0, + crate::error::InvalidIntervalSnafu { + reason: "must be a positive interval", + } + ); + + // Cast safety: `adjusted_seconds` is guaranteed to be non-negative before. + let adjusted_seconds = adjusted_seconds as u64; + // Cast safety: `nanos_remainder` is smaller than 1_000_000_000 which + // is checked above. + let nanos_remainder = nanos_remainder as u32; + + Ok(std::time::Duration::new(adjusted_seconds, nanos_remainder)) +}