feat: enforce using aggregate function in order by when group by is used

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(_)

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<LexerError> for Error {

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()));
+}

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)

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)

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

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