Review feedback

crates/query-engine/translation/src/translation/query/error.rs

@@ -15,6 +15,8 @@ pub enum Error {
     },
     RelationshipArgumentWasOverriden(String),
     EmptyPathForStarCountAggregate,
+    EmptyPathForSingleColumnAggregate,
+    MissingAggregateForArraryRelationOrdering,
     NoFields,
     TypeMismatch(serde_json::Value, database::ScalarType),
     UnexpectedVariable,
@@ -66,6 +68,15 @@ impl std::fmt::Display for Error {
             Error::EmptyPathForStarCountAggregate => {
                 write!(f, "No path elements supplied for Star Count Aggregate")
             }
+            Error::EmptyPathForSingleColumnAggregate => {
+                write!(f, "No path elements supplied for Single Column Aggregate")
+            }
+            Error::MissingAggregateForArraryRelationOrdering => {
+                write!(
+                    f,
+                    "No aggregation function was suppilied for ordering on an array relationship"
+                )
+            }
             Error::NoFields => {
                 write!(f, "No fields in request.")
             }

crates/query-engine/translation/src/translation/query/sorting.rs

@@ -154,9 +154,7 @@ fn translate_order_by_star_count_aggregate(
             // return the column to order by (from our fancy join)
             Ok((column_alias, select))
         }
-        None => Err(Error::NotSupported(
-            "order by star count aggregates".to_string(),
-        )),
+        None => Err(Error::EmptyPathForStarCountAggregate),
     }
 }
 
@@ -279,9 +277,7 @@ fn translate_order_by_target_for_column(
         // if we got an aggregation function, the query does not make sense.
         match function {
             None => Ok(())?,
-            Some(func) => Err(Error::NotSupported(format!(
-                "Cannot order by aggregation function '{func}' on the same table"
-            )))?,
+            Some(_) => Err(Error::EmptyPathForSingleColumnAggregate)?,
         };
 
         // if there were no relationship columns, we don't need to build a query, just return the column.
@@ -374,9 +370,9 @@ fn process_path_element_for_order_by_target_for_column(
     let relationship = env.lookup_relationship(&path_element.relationship)?;
 
     match relationship.relationship_type {
-        models::RelationshipType::Array if aggregate_function_for_arrays.is_none() => Err(
-            Error::NotSupported("order by an array relationship".to_string()),
-        ),
+        models::RelationshipType::Array if aggregate_function_for_arrays.is_none() => {
+            Err(Error::MissingAggregateForArraryRelationOrdering)
+        }
         models::RelationshipType::Array => Ok(()),
         models::RelationshipType::Object => Ok(()),
     }?;

crates/query-engine/translation/tests/goldenfiles/sorting_by_no_relationship_aggregate/request.json

@@ -15,7 +15,7 @@
             "type": "single_column_aggregate",
             "column": "TopRadioChartPlacement",
             "function": "impossible",
-            "path": [ ]
+            "path": []
           },
           "order_direction": "desc"
         }

crates/query-engine/translation/tests/snapshots/tests__negative_tests__sorting_by_no_relationship_aggregate.snap

@@ -0,0 +1,5 @@
+---
+source: crates/query-engine/translation/tests/tests.rs
+expression: "format!(\"{result}\")"
+---
+No path elements supplied for Single Column Aggregate

crates/query-engine/translation/tests/tests.rs

@@ -141,18 +141,23 @@ fn sorting_by_nested_relationship_count() {
     insta::assert_snapshot!(result);
 }
 
-#[test]
-fn sorting_by_no_relationship_aggregate() {
-    let result = common::test_translation("sorting_by_no_relationship_aggregate");
-    insta::assert_snapshot!(format!("{result:?}"));
-}
-
 #[test]
 fn sorting_by_relationship_count_with_predicate() {
     let result = common::test_translation("sorting_by_relationship_count_with_predicate").unwrap();
     insta::assert_snapshot!(result);
 }
 
+mod negative_tests {
+    use crate::common;
+
+    #[test]
+    fn sorting_by_no_relationship_aggregate() {
+        let result = common::test_translation("sorting_by_no_relationship_aggregate")
+            .expect_err("Expected error");
+        insta::assert_snapshot!(result.to_string());
+    }
+}
+
 mod native_queries {
     use crate::common;