From 5808c91ed52808c89030168a5b3eab372fe7cfd9 Mon Sep 17 00:00:00 2001 From: Philip Lykke Carlsen Date: Wed, 15 Nov 2023 15:48:51 +0100 Subject: [PATCH] Review feedback --- .../translation/src/translation/query/error.rs | 11 +++++++++++ .../src/translation/query/sorting.rs | 14 +++++--------- .../request.json | 2 +- ...s__sorting_by_no_relationship_aggregate.snap | 5 +++++ ...s__sorting_by_no_relationship_aggregate.snap | 5 ----- crates/query-engine/translation/tests/tests.rs | 17 +++++++++++------ 6 files changed, 33 insertions(+), 21 deletions(-) create mode 100644 crates/query-engine/translation/tests/snapshots/tests__negative_tests__sorting_by_no_relationship_aggregate.snap delete mode 100644 crates/query-engine/translation/tests/snapshots/tests__sorting_by_no_relationship_aggregate.snap diff --git a/crates/query-engine/translation/src/translation/query/error.rs b/crates/query-engine/translation/src/translation/query/error.rs index 7948d3af6..2343d5ed3 100644 --- a/crates/query-engine/translation/src/translation/query/error.rs +++ b/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.") } diff --git a/crates/query-engine/translation/src/translation/query/sorting.rs b/crates/query-engine/translation/src/translation/query/sorting.rs index e3654f27d..a75f4f7cd 100644 --- a/crates/query-engine/translation/src/translation/query/sorting.rs +++ b/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(()), }?; diff --git a/crates/query-engine/translation/tests/goldenfiles/sorting_by_no_relationship_aggregate/request.json b/crates/query-engine/translation/tests/goldenfiles/sorting_by_no_relationship_aggregate/request.json index 6e61a131d..6a6b1221b 100644 --- a/crates/query-engine/translation/tests/goldenfiles/sorting_by_no_relationship_aggregate/request.json +++ b/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" } diff --git a/crates/query-engine/translation/tests/snapshots/tests__negative_tests__sorting_by_no_relationship_aggregate.snap b/crates/query-engine/translation/tests/snapshots/tests__negative_tests__sorting_by_no_relationship_aggregate.snap new file mode 100644 index 000000000..7aa0b1b38 --- /dev/null +++ b/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 diff --git a/crates/query-engine/translation/tests/snapshots/tests__sorting_by_no_relationship_aggregate.snap b/crates/query-engine/translation/tests/snapshots/tests__sorting_by_no_relationship_aggregate.snap deleted file mode 100644 index 2ea0167d3..000000000 --- a/crates/query-engine/translation/tests/snapshots/tests__sorting_by_no_relationship_aggregate.snap +++ /dev/null @@ -1,5 +0,0 @@ ---- -source: crates/query-engine/translation/tests/tests.rs -expression: "format!(\"{result:?}\")" ---- -Err(NotSupported("Cannot order by aggregation function 'impossible' on the same table")) diff --git a/crates/query-engine/translation/tests/tests.rs b/crates/query-engine/translation/tests/tests.rs index 8a3d1dab4..e6b01472b 100644 --- a/crates/query-engine/translation/tests/tests.rs +++ b/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;