From e89cf1a4ad98c6ef9d8f033bcbc9ee4cef8c9379 Mon Sep 17 00:00:00 2001 From: YoEight Date: Fri, 6 Feb 2026 22:52:37 -0500 Subject: [PATCH] feat: implement static analysis on `HAVING` clause --- src/analysis.rs | 51 +++++- src/tests/analysis.rs | 12 ++ .../reject_invalid_having_clause.eql | 6 + src/tests/resources/valid_having_clause.eql | 6 + ..._analysis__accept_valid_having_clause.snap | 168 ++++++++++++++++++ ...nalysis__reject_invalid_having_clause.snap | 9 + 6 files changed, 249 insertions(+), 3 deletions(-) create mode 100644 src/tests/resources/reject_invalid_having_clause.eql create mode 100644 src/tests/resources/valid_having_clause.eql create mode 100644 src/tests/snapshots/eventql_parser__tests__analysis__accept_valid_having_clause.snap create mode 100644 src/tests/snapshots/eventql_parser__tests__analysis__reject_invalid_having_clause.snap diff --git a/src/analysis.rs b/src/analysis.rs index 91948b8..88f275e 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -651,7 +651,16 @@ impl<'a> Analysis<'a> { self.analyze_expr(&mut ctx, &group_by.expr, Type::Unspecified)?; if let Some(expr) = &group_by.predicate { + ctx.allow_agg_func = true; + ctx.use_agg_funcs = true; + self.analyze_expr(&mut ctx, expr, Type::Bool)?; + if !self.expect_agg_expr(expr)? { + return Err(AnalysisError::ExpectAggExpr( + expr.attrs.pos.line, + expr.attrs.pos.col, + )); + } } ctx.allow_agg_func = true; @@ -669,7 +678,7 @@ impl<'a> Analysis<'a> { order_by.expr.attrs.pos.col, )); } else if query.group_by.is_some() { - self.expect_agg_expr(&order_by.expr)?; + self.expect_agg_func(&order_by.expr)?; } } @@ -903,7 +912,7 @@ impl<'a> Analysis<'a> { } if *aggregate { - return self.expect_agg_expr(expr); + return self.expect_agg_func(expr); } for arg in &app.args { @@ -924,7 +933,7 @@ impl<'a> Analysis<'a> { } } - fn expect_agg_expr(&self, expr: &Expr) -> AnalysisResult<()> { + fn expect_agg_func(&self, expr: &Expr) -> AnalysisResult<()> { if let Value::App(app) = &expr.value && let Some(Type::App { aggregate: true, .. @@ -944,6 +953,42 @@ impl<'a> Analysis<'a> { )) } + fn expect_agg_expr(&self, expr: &Expr) -> AnalysisResult { + match &expr.value { + Value::Id(id) => { + if self.scope.entries.contains_key(id.as_str()) { + return Err(AnalysisError::UnallowedAggFuncUsageWithSrcField( + expr.attrs.pos.line, + expr.attrs.pos.col, + )); + } + + Ok(false) + } + Value::Group(expr) => self.expect_agg_expr(expr), + Value::Binary(binary) => { + let lhs = self.expect_agg_expr(&binary.lhs)?; + let rhs = self.expect_agg_expr(&binary.rhs)?; + + if !lhs && !rhs { + return Err(AnalysisError::ExpectAggExpr( + expr.attrs.pos.line, + expr.attrs.pos.col, + )); + } + + Ok(true) + } + Value::Unary(unary) => self.expect_agg_expr(unary.expr.as_ref()), + Value::App(_) => { + self.expect_agg_func(expr)?; + Ok(true) + } + + _ => Ok(false), + } + } + fn ensure_agg_param_is_source_bound(&self, expr: &Expr) -> AnalysisResult<()> { match &expr.value { Value::Id(id) if !self.options.default_scope.entries.contains_key(id.as_str()) => { diff --git a/src/tests/analysis.rs b/src/tests/analysis.rs index bf52ffe..98d76f0 100644 --- a/src/tests/analysis.rs +++ b/src/tests/analysis.rs @@ -256,3 +256,15 @@ fn test_analyze_accept_group_by_with_agg_rec() { let query = parse_query(include_str!("./resources/accept_group_by_with_agg_rec.eql")).unwrap(); insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default())); } + +#[test] +fn test_reject_invalid_having_clause() { + let query = parse_query(include_str!("./resources/reject_invalid_having_clause.eql")).unwrap(); + insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default())); +} + +#[test] +fn test_accept_valid_having_clause() { + let query = parse_query(include_str!("./resources/valid_having_clause.eql")).unwrap(); + insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default())); +} diff --git a/src/tests/resources/reject_invalid_having_clause.eql b/src/tests/resources/reject_invalid_having_clause.eql new file mode 100644 index 0000000..d347038 --- /dev/null +++ b/src/tests/resources/reject_invalid_having_clause.eql @@ -0,0 +1,6 @@ +FROM e IN events +GROUP BY e.data.department HAVING e.data.salary > 10000 +PROJECT INTO { + department: UNIQUE(e.data.department), + average: AVG(e.data.salary) +} \ No newline at end of file diff --git a/src/tests/resources/valid_having_clause.eql b/src/tests/resources/valid_having_clause.eql new file mode 100644 index 0000000..f112371 --- /dev/null +++ b/src/tests/resources/valid_having_clause.eql @@ -0,0 +1,6 @@ +FROM e IN events +GROUP BY e.data.department HAVING AVG(e.data.salary) > 10000 +PROJECT INTO { + department: UNIQUE(e.data.department), + average: AVG(e.data.salary) +} \ No newline at end of file diff --git a/src/tests/snapshots/eventql_parser__tests__analysis__accept_valid_having_clause.snap b/src/tests/snapshots/eventql_parser__tests__analysis__accept_valid_having_clause.snap new file mode 100644 index 0000000..1ae8186 --- /dev/null +++ b/src/tests/snapshots/eventql_parser__tests__analysis__accept_valid_having_clause.snap @@ -0,0 +1,168 @@ +--- +source: src/tests/analysis.rs +expression: "query.run_static_analysis(&Default::default())" +--- +Ok: + attrs: + pos: + line: 1 + col: 1 + sources: + - binding: + name: e + pos: + line: 1 + col: 6 + kind: + Name: events + predicate: ~ + group_by: + expr: + attrs: + pos: + line: 2 + col: 10 + value: + Access: + target: + attrs: + pos: + line: 2 + col: 10 + value: + Access: + target: + attrs: + pos: + line: 2 + col: 10 + value: + Id: e + field: data + field: department + predicate: + attrs: + pos: + line: 2 + col: 35 + value: + Binary: + lhs: + attrs: + pos: + line: 2 + col: 35 + value: + App: + func: AVG + args: + - attrs: + pos: + line: 2 + col: 39 + value: + Access: + target: + attrs: + pos: + line: 2 + col: 39 + value: + Access: + target: + attrs: + pos: + line: 2 + col: 39 + value: + Id: e + field: data + field: salary + operator: Gt + rhs: + attrs: + pos: + line: 2 + col: 56 + value: + Number: 10000 + order_by: ~ + limit: ~ + projection: + attrs: + pos: + line: 3 + col: 14 + value: + Record: + - name: department + value: + attrs: + pos: + line: 4 + col: 17 + value: + App: + func: UNIQUE + args: + - attrs: + pos: + line: 4 + col: 24 + value: + Access: + target: + attrs: + pos: + line: 4 + col: 24 + value: + Access: + target: + attrs: + pos: + line: 4 + col: 24 + value: + Id: e + field: data + field: department + - name: average + value: + attrs: + pos: + line: 5 + col: 14 + value: + App: + func: AVG + args: + - attrs: + pos: + line: 5 + col: 18 + value: + Access: + target: + attrs: + pos: + line: 5 + col: 18 + value: + Access: + target: + attrs: + pos: + line: 5 + col: 18 + value: + Id: e + field: data + field: salary + distinct: false + meta: + project: + Record: + average: Number + department: Unspecified + aggregate: true diff --git a/src/tests/snapshots/eventql_parser__tests__analysis__reject_invalid_having_clause.snap b/src/tests/snapshots/eventql_parser__tests__analysis__reject_invalid_having_clause.snap new file mode 100644 index 0000000..3393032 --- /dev/null +++ b/src/tests/snapshots/eventql_parser__tests__analysis__reject_invalid_having_clause.snap @@ -0,0 +1,9 @@ +--- +source: src/tests/analysis.rs +expression: "query.run_static_analysis(&Default::default())" +--- +Err: + Analysis: + ExpectAggExpr: + - 2 + - 35