From 7fa2e50f670c5326aa850e2002b2b2da76ac823d Mon Sep 17 00:00:00 2001 From: YoEight Date: Sun, 1 Feb 2026 19:35:27 -0500 Subject: [PATCH] feat: enforce using aggregate functions when using order by in a group by query --- src/analysis.rs | 46 +++++-- src/error.rs | 23 ++++ src/tests/analysis.rs | 18 +++ ...accept_group_by_with_order_by_with_agg.eql | 4 + .../reject_group_by_with_order_by_no_agg.eql | 4 + ...ccept_group_by_with_order_by_with_agg.snap | 112 ++++++++++++++++++ ..._reject_group_by_with_order_by_no_agg.snap | 9 ++ 7 files changed, 205 insertions(+), 11 deletions(-) create mode 100644 src/tests/resources/accept_group_by_with_order_by_with_agg.eql create mode 100644 src/tests/resources/reject_group_by_with_order_by_no_agg.eql create mode 100644 src/tests/snapshots/eventql_parser__tests__analysis__analyze_accept_group_by_with_order_by_with_agg.snap create mode 100644 src/tests/snapshots/eventql_parser__tests__analysis__analyze_reject_group_by_with_order_by_no_agg.snap diff --git a/src/analysis.rs b/src/analysis.rs index d37b171..fa95a8c 100644 --- a/src/analysis.rs +++ b/src/analysis.rs @@ -655,17 +655,21 @@ impl<'a> Analysis<'a> { } } + let project = self.analyze_projection(&mut ctx, &query.projection)?; + if let Some(order_by) = &query.order_by { - if !matches!(&order_by.expr.value, Value::Access(_)) { + self.analyze_expr(&mut ctx, &order_by.expr, Type::Unspecified)?; + + if query.group_by.is_none() && !matches!(&order_by.expr.value, Value::Access(_)) { return Err(AnalysisError::ExpectFieldLiteral( order_by.expr.attrs.pos.line, order_by.expr.attrs.pos.col, )); + } else if query.group_by.is_some() { + self.expect_agg_expr(&order_by.expr)?; } - self.analyze_expr(&mut ctx, &order_by.expr, Type::Unspecified)?; } - let project = self.analyze_projection(&mut ctx, &query.projection)?; let scope = self.exit_scope(); Ok(Query { @@ -875,11 +879,11 @@ impl<'a> Analysis<'a> { )); } - for arg in &app.args { - if *aggregate { - self.ensure_agg_param_is_source_bound(arg)?; - } + if *aggregate { + return self.expect_agg_expr(expr); + } + for arg in &app.args { self.invalidate_agg_func_usage(arg)?; } } @@ -897,7 +901,27 @@ impl<'a> Analysis<'a> { } } - fn ensure_agg_param_is_source_bound(&mut self, expr: &Expr) -> AnalysisResult<()> { + fn expect_agg_expr(&self, expr: &Expr) -> AnalysisResult<()> { + if let Value::App(app) = &expr.value + && let Some(Type::App { + aggregate: true, .. + }) = self.options.default_scope.entries.get(app.func.as_str()) + { + for arg in &app.args { + self.ensure_agg_param_is_source_bound(arg)?; + self.invalidate_agg_func_usage(arg)?; + } + + return Ok(()); + } + + Err(AnalysisError::ExpectAggExpr( + expr.attrs.pos.line, + expr.attrs.pos.col, + )) + } + + 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()) => { Ok(()) @@ -914,7 +938,7 @@ impl<'a> Analysis<'a> { } fn ensure_agg_binary_op_is_source_bound( - &mut self, + &self, attrs: &Attrs, binary: &Binary, ) -> AnalysisResult<()> { @@ -930,7 +954,7 @@ impl<'a> Analysis<'a> { Ok(()) } - fn ensure_agg_binary_op_branch_is_source_bound(&mut self, expr: &Expr) -> bool { + fn ensure_agg_binary_op_branch_is_source_bound(&self, expr: &Expr) -> bool { match &expr.value { Value::Id(id) => !self.options.default_scope.entries.contains_key(id.as_str()), Value::Array(exprs) => { @@ -965,7 +989,7 @@ impl<'a> Analysis<'a> { } } - fn invalidate_agg_func_usage(&mut self, expr: &Expr) -> AnalysisResult<()> { + fn invalidate_agg_func_usage(&self, expr: &Expr) -> AnalysisResult<()> { match &expr.value { Value::Number(_) | Value::String(_) diff --git a/src/error.rs b/src/error.rs index bb99193..220aa8a 100644 --- a/src/error.rs +++ b/src/error.rs @@ -339,6 +339,29 @@ pub enum AnalysisError { /// ``` #[error("{0}:{1}: constant expressions are forbidden in PROJECT INTO clause")] ConstantExprInProjectIntoClause(u32, u32), + + /// Expect an aggregate expression at this position in the query. + /// + /// Fields: `(line, column)` + /// + /// Invalid usage: + /// ```eql + /// FROM e IN events + /// GROUP BY e.data.department + /// // ERROR: the order by clause should use an aggregage expresion because GROUP Bys + /// // require an aggregate expression in this context. + /// ORDER BY e.data.salary + /// PROJECT INTO AVG(e.data.salary) + /// ``` + /// Valid usage: + /// ```eql + /// FROM e IN events + /// GROUP BY e.data.department + /// ORDER BY AVG(e.data.salary) + /// PROJECT INTO AVG(e.data.salary) + /// ``` + #[error("{0}:{1}: expect aggregate expression")] + ExpectAggExpr(u32, u32), } impl From for Error { diff --git a/src/tests/analysis.rs b/src/tests/analysis.rs index 6214428..6cac2e0 100644 --- a/src/tests/analysis.rs +++ b/src/tests/analysis.rs @@ -217,3 +217,21 @@ fn test_analyze_allow_constant_agg_func() { let query = parse_query(include_str!("./resources/allow_constant_agg_func.eql")).unwrap(); insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default())); } + +#[test] +fn test_analyze_reject_group_by_with_order_by_no_agg() { + let query = parse_query(include_str!( + "./resources/reject_group_by_with_order_by_no_agg.eql" + )) + .unwrap(); + insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default())); +} + +#[test] +fn test_analyze_accept_group_by_with_order_by_with_agg() { + let query = parse_query(include_str!( + "./resources/accept_group_by_with_order_by_with_agg.eql" + )) + .unwrap(); + insta::assert_yaml_snapshot!(query.run_static_analysis(&Default::default())); +} diff --git a/src/tests/resources/accept_group_by_with_order_by_with_agg.eql b/src/tests/resources/accept_group_by_with_order_by_with_agg.eql new file mode 100644 index 0000000..a3edfc3 --- /dev/null +++ b/src/tests/resources/accept_group_by_with_order_by_with_agg.eql @@ -0,0 +1,4 @@ +FROM e IN events +GROUP BY e.data.department +ORDER BY AVG(e.data.salary) +PROJECT INTO AVG(e.data.salary) diff --git a/src/tests/resources/reject_group_by_with_order_by_no_agg.eql b/src/tests/resources/reject_group_by_with_order_by_no_agg.eql new file mode 100644 index 0000000..cec92d4 --- /dev/null +++ b/src/tests/resources/reject_group_by_with_order_by_no_agg.eql @@ -0,0 +1,4 @@ +FROM e IN events +GROUP BY e.data.department +ORDER BY e.data.salary +PROJECT INTO AVG(e.data.salary) diff --git a/src/tests/snapshots/eventql_parser__tests__analysis__analyze_accept_group_by_with_order_by_with_agg.snap b/src/tests/snapshots/eventql_parser__tests__analysis__analyze_accept_group_by_with_order_by_with_agg.snap new file mode 100644 index 0000000..03356f1 --- /dev/null +++ b/src/tests/snapshots/eventql_parser__tests__analysis__analyze_accept_group_by_with_order_by_with_agg.snap @@ -0,0 +1,112 @@ +--- +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: ~ + order_by: + expr: + attrs: + pos: + line: 3 + col: 10 + value: + App: + func: AVG + args: + - attrs: + pos: + line: 3 + col: 14 + value: + Access: + target: + attrs: + pos: + line: 3 + col: 14 + value: + Access: + target: + attrs: + pos: + line: 3 + col: 14 + value: + Id: e + field: data + field: salary + order: Asc + limit: ~ + projection: + attrs: + pos: + line: 4 + col: 14 + value: + App: + func: AVG + args: + - attrs: + pos: + line: 4 + col: 18 + value: + Access: + target: + attrs: + pos: + line: 4 + col: 18 + value: + Access: + target: + attrs: + pos: + line: 4 + col: 18 + value: + Id: e + field: data + field: salary + distinct: false + meta: + project: Number + aggregate: true diff --git a/src/tests/snapshots/eventql_parser__tests__analysis__analyze_reject_group_by_with_order_by_no_agg.snap b/src/tests/snapshots/eventql_parser__tests__analysis__analyze_reject_group_by_with_order_by_no_agg.snap new file mode 100644 index 0000000..44e4317 --- /dev/null +++ b/src/tests/snapshots/eventql_parser__tests__analysis__analyze_reject_group_by_with_order_by_no_agg.snap @@ -0,0 +1,9 @@ +--- +source: src/tests/analysis.rs +expression: "query.run_static_analysis(&Default::default())" +--- +Err: + Analysis: + ExpectAggExpr: + - 3 + - 10