Skip to content

Commit 8077565

Browse files
parmesantparmesant
committed
coderabbit suggestions and bugfix
1 parent 8b1bc18 commit 8077565

File tree

3 files changed

+130
-65
lines changed

3 files changed

+130
-65
lines changed

‎src/alerts/alerts_utils.rs

Lines changed: 83 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
*
1717
*/
1818

19+
use std::fmt::Display;
20+
1921
use arrow_array::{Float64Array, Int64Array, RecordBatch};
2022
use datafusion::{
2123
functions_aggregate::{
@@ -345,67 +347,91 @@ pub fn get_filter_string(where_clause: &Conditions) -> Result<String, String> {
345347
LogicalOperator::And => {
346348
let mut exprs = vec![];
347349
for condition in &where_clause.condition_config {
348-
let value = NumberOrString::from_string(condition.value.to_string());
349-
match value {
350-
NumberOrString::Number(val) => exprs.push(format!(
351-
"{} {} {}",
352-
condition.column, condition.operator, val
353-
)),
354-
NumberOrString::String(val) => exprs.push(format!(
355-
"{} {} '{}'",
356-
condition.column, condition.operator, val
357-
)),
350+
if let Some(value) = &condition.value {
351+
// ad-hoc error check in case value is some and operator is either `is null` or `is not null`
352+
if condition.operator.eq(&WhereConfigOperator::IsNull)
353+
|| condition.operator.eq(&WhereConfigOperator::IsNotNull)
354+
{
355+
return Err("value must be null when operator is either `is null` or `is not null`"
356+
.into());
357+
}
358+
let value = NumberOrString::from_string(value.to_owned());
359+
match value {
360+
NumberOrString::Number(val) => exprs.push(format!(
361+
"\"{}\" {} {}",
362+
condition.column, condition.operator, val
363+
)),
364+
NumberOrString::String(val) => exprs.push(format!(
365+
"\"{}\" {} '{}'",
366+
condition.column,
367+
condition.operator,
368+
val.replace('\'', "''")
369+
)),
370+
}
371+
} else {
372+
exprs.push(format!("\"{}\" {}", condition.column, condition.operator))
358373
}
359374
}
360375

361376
Ok(exprs.join(" AND "))
362377
}
363-
_ => Err(String::from("Invalid option 'or'")),
378+
_ => Err(String::from("Invalid option 'or', only 'and' is supported")),
364379
},
365-
_ => Err(String::from("Invalid option 'null'")),
380+
_ => Err(String::from(
381+
"Invalid option 'null', only 'and' is supported",
382+
)),
366383
}
367384
}
368385

369386
fn match_alert_operator(expr: &ConditionConfig) -> Expr {
370387
// the form accepts value as a string
371388
// if it can be parsed as a number, then parse it
372389
// else keep it as a string
373-
let value = NumberOrString::from_string(expr.value.clone());
374-
375-
// for maintaining column case
376-
let column = format!(r#""{}""#, expr.column);
377-
match expr.operator {
378-
WhereConfigOperator::Equal => col(column).eq(lit(value)),
379-
WhereConfigOperator::NotEqual => col(column).not_eq(lit(value)),
380-
WhereConfigOperator::LessThan => col(column).lt(lit(value)),
381-
WhereConfigOperator::GreaterThan => col(column).gt(lit(value)),
382-
WhereConfigOperator::LessThanOrEqual => col(column).lt_eq(lit(value)),
383-
WhereConfigOperator::GreaterThanOrEqual => col(column).gt_eq(lit(value)),
384-
WhereConfigOperator::IsNull => col(column).is_null(),
385-
WhereConfigOperator::IsNotNull => col(column).is_not_null(),
386-
WhereConfigOperator::ILike => col(column).ilike(lit(&expr.value)),
387-
WhereConfigOperator::Contains => col(column).like(lit(&expr.value)),
388-
WhereConfigOperator::BeginsWith => Expr::BinaryExpr(BinaryExpr::new(
389-
Box::new(col(column)),
390-
Operator::RegexIMatch,
391-
Box::new(lit(format!("^{}", expr.value))),
392-
)),
393-
WhereConfigOperator::EndsWith => Expr::BinaryExpr(BinaryExpr::new(
394-
Box::new(col(column)),
395-
Operator::RegexIMatch,
396-
Box::new(lit(format!("{}$", expr.value))),
397-
)),
398-
WhereConfigOperator::DoesNotContain => col(column).not_ilike(lit(&expr.value)),
399-
WhereConfigOperator::DoesNotBeginWith => Expr::BinaryExpr(BinaryExpr::new(
400-
Box::new(col(column)),
401-
Operator::RegexNotIMatch,
402-
Box::new(lit(format!("^{}", expr.value))),
403-
)),
404-
WhereConfigOperator::DoesNotEndWith => Expr::BinaryExpr(BinaryExpr::new(
405-
Box::new(col(column)),
406-
Operator::RegexNotIMatch,
407-
Box::new(lit(format!("{}$", expr.value))),
408-
)),
390+
if let Some(value) = &expr.value {
391+
let value = NumberOrString::from_string(value.clone());
392+
393+
// for maintaining column case
394+
let column = format!(r#""{}""#, expr.column);
395+
match expr.operator {
396+
WhereConfigOperator::Equal => col(column).eq(lit(value)),
397+
WhereConfigOperator::NotEqual => col(column).not_eq(lit(value)),
398+
WhereConfigOperator::LessThan => col(column).lt(lit(value)),
399+
WhereConfigOperator::GreaterThan => col(column).gt(lit(value)),
400+
WhereConfigOperator::LessThanOrEqual => col(column).lt_eq(lit(value)),
401+
WhereConfigOperator::GreaterThanOrEqual => col(column).gt_eq(lit(value)),
402+
WhereConfigOperator::ILike => col(column).ilike(lit(value)),
403+
WhereConfigOperator::Contains => col(column).like(lit(value)),
404+
WhereConfigOperator::BeginsWith => Expr::BinaryExpr(BinaryExpr::new(
405+
Box::new(col(column)),
406+
Operator::RegexIMatch,
407+
Box::new(lit(format!("^{}", value))),
408+
)),
409+
WhereConfigOperator::EndsWith => Expr::BinaryExpr(BinaryExpr::new(
410+
Box::new(col(column)),
411+
Operator::RegexIMatch,
412+
Box::new(lit(format!("{}$", value))),
413+
)),
414+
WhereConfigOperator::DoesNotContain => col(column).not_ilike(lit(value)),
415+
WhereConfigOperator::DoesNotBeginWith => Expr::BinaryExpr(BinaryExpr::new(
416+
Box::new(col(column)),
417+
Operator::RegexNotIMatch,
418+
Box::new(lit(format!("^{}", value))),
419+
)),
420+
WhereConfigOperator::DoesNotEndWith => Expr::BinaryExpr(BinaryExpr::new(
421+
Box::new(col(column)),
422+
Operator::RegexNotIMatch,
423+
Box::new(lit(format!("{}$", value))),
424+
)),
425+
_ => unreachable!("value must not be null for operators other than `is null` and `is not null`. Should've been caught in validation")
426+
}
427+
} else {
428+
// for maintaining column case
429+
let column = format!(r#""{}""#, expr.column);
430+
match expr.operator {
431+
WhereConfigOperator::IsNull => col(column).is_null(),
432+
WhereConfigOperator::IsNotNull => col(column).is_not_null(),
433+
_ => unreachable!("value must be null for `is null` and `is not null`. Should've been caught in validation")
434+
}
409435
}
410436
}
411437

@@ -444,3 +470,12 @@ impl NumberOrString {
444470
}
445471
}
446472
}
473+
474+
impl Display for NumberOrString {
475+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
476+
match self {
477+
NumberOrString::Number(v) => write!(f, "{v}"),
478+
NumberOrString::String(v) => write!(f, "{v}"),
479+
}
480+
}
481+
}

‎src/alerts/mod.rs

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ impl Display for AlertOperator {
220220
}
221221
}
222222

223-
#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, FromStr)]
223+
#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, FromStr, PartialEq, Eq)]
224224
#[serde(rename_all = "camelCase")]
225225
pub enum WhereConfigOperator {
226226
#[serde(rename = "=")]
@@ -326,7 +326,7 @@ pub struct FilterConfig {
326326
pub struct ConditionConfig {
327327
pub column: String,
328328
pub operator: WhereConfigOperator,
329-
pub value: String,
329+
pub value: Option<String>,
330330
}
331331

332332
#[derive(Debug, serde::Serialize, serde::Deserialize, Clone)]
@@ -343,20 +343,28 @@ impl Conditions {
343343
LogicalOperator::And | LogicalOperator::Or => {
344344
let expr1 = &self.condition_config[0];
345345
let expr2 = &self.condition_config[1];
346-
format!(
347-
"[{} {} {} {op} {} {} {}]",
348-
expr1.column,
349-
expr1.operator,
350-
expr1.value,
351-
expr2.column,
352-
expr2.operator,
353-
expr2.value
354-
)
346+
let expr1_msg = if let Some(val) = &expr1.value {
347+
format!("{} {} {}", expr1.column, expr1.operator, val)
348+
} else {
349+
format!("{} {}", expr1.column, expr1.operator)
350+
};
351+
352+
let expr2_msg = if let Some(val) = &expr2.value {
353+
format!("{} {} {}", expr2.column, expr2.operator, val)
354+
} else {
355+
format!("{} {}", expr2.column, expr2.operator)
356+
};
357+
358+
format!("[{} {op} {}]", expr1_msg, expr2_msg)
355359
}
356360
},
357361
None => {
358362
let expr = &self.condition_config[0];
359-
format!("[{} {} {}]", expr.column, expr.operator, expr.value)
363+
if let Some(val) = &expr.value {
364+
format!("{} {} {}", expr.column, expr.operator, val)
365+
} else {
366+
format!("{} {}", expr.column, expr.operator)
367+
}
360368
}
361369
}
362370
}
@@ -645,6 +653,27 @@ impl AlertConfig {
645653
}
646654
}
647655
}
656+
657+
// validate that the value should be None in case of `is null` and `is not null`
658+
for condition in config.condition_config.iter() {
659+
let needs_no_value = matches!(
660+
condition.operator,
661+
WhereConfigOperator::IsNull | WhereConfigOperator::IsNotNull
662+
);
663+
664+
if needs_no_value && condition.value.is_some() {
665+
return Err(AlertError::CustomError(
666+
"value must be null when operator is either `is null` or `is not null`"
667+
.into(),
668+
));
669+
}
670+
if !needs_no_value && condition.value.as_ref().is_none_or(|v| v.trim().is_empty()) {
671+
return Err(AlertError::CustomError(
672+
"value must not be null when operator is neither `is null` nor `is not null`"
673+
.into(),
674+
));
675+
}
676+
}
648677
Ok(())
649678
}
650679

‎src/query/mod.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,7 @@ impl CountsRequest {
445445

446446
/// This function will get executed only if self.conditions is some
447447
pub async fn get_df_sql(&self) -> Result<String, QueryError> {
448+
// unwrap because we have asserted that it is some
448449
let count_conditions = self.conditions.as_ref().unwrap();
449450

450451
let time_range = TimeRange::parse_human_time(&self.start_time, &self.end_time)?;
@@ -453,20 +454,20 @@ impl CountsRequest {
453454

454455
let date_bin = if dur.num_minutes() <= 60 * 10 {
455456
// date_bin 1 minute
456-
format!("CAST(DATE_BIN('1 minute', {}.p_timestamp, TIMESTAMP '2025-01-01T00:00:00.000Z') AS TEXT) as start_time, DATE_BIN('1 minute', p_timestamp, TIMESTAMP '2025-01-01T00:00:00.000Z') + INTERVAL '1 minute' as end_time", self.stream)
457+
format!("CAST(DATE_BIN('1 minute', \"{}\".p_timestamp, TIMESTAMP '1970-01-01 00:00:00+00') AS TEXT) as start_time, DATE_BIN('1 minute', p_timestamp, TIMESTAMP '1970-01-01 00:00:00+00') + INTERVAL '1 minute' as end_time", self.stream)
457458
} else if dur.num_minutes() > 60 * 10 && dur.num_minutes() < 60 * 240 {
458459
// date_bin 1 hour
459-
String::from("CAST(DATE_BIN('1 hour', p_timestamp, TIMESTAMP '2025-01-01T00:00:00.000Z') AS TEXT) as start_time, DATE_BIN('1 hour', p_timestamp, TIMESTAMP '2025-01-01T00:00:00.000Z') + INTERVAL '1 hour' as end_time")
460+
format!("CAST(DATE_BIN('1 hour', \"{}\".p_timestamp, TIMESTAMP '1970-01-01 00:00:00+00') AS TEXT) as start_time, DATE_BIN('1 hour', p_timestamp, TIMESTAMP '1970-01-01 00:00:00+00') + INTERVAL '1 hour' as end_time", self.stream)
460461
} else {
461462
// date_bin 1 day
462-
String::from("CAST(DATE_BIN('1 day', p_timestamp, TIMESTAMP '2025-01-01T00:00:00.000Z') AS TEXT) as start_time, DATE_BIN('1 day', p_timestamp, TIMESTAMP '2025-01-01T00:00:00.000Z') + INTERVAL '1 day' as end_time")
463+
format!("CAST(DATE_BIN('1 day', \"{}\".p_timestamp, TIMESTAMP '1970-01-01 00:00:00+00') AS TEXT) as start_time, DATE_BIN('1 day', p_timestamp, TIMESTAMP '1970-01-01 00:00:00+00') + INTERVAL '1 day' as end_time", self.stream)
463464
};
464465

465466
let query = if let Some(conditions) = &count_conditions.conditions {
466467
let f = get_filter_string(conditions).map_err(QueryError::CustomError)?;
467-
format!("SELECT {date_bin}, COUNT(*) as count FROM {} WHERE {} GROUP BY end_time,start_time ORDER BY end_time",self.stream,f)
468+
format!("SELECT {date_bin}, COUNT(*) as count FROM \"{}\" WHERE {} GROUP BY end_time,start_time ORDER BY end_time",self.stream,f)
468469
} else {
469-
format!("SELECT {date_bin}, COUNT(*) as count FROM {} GROUP BY end_time,start_time ORDER BY end_time",self.stream)
470+
format!("SELECT {date_bin}, COUNT(*) as count FROM \"{}\" GROUP BY end_time,start_time ORDER BY end_time",self.stream)
470471
};
471472
Ok(query)
472473
}

0 commit comments

Comments
 (0)