From ee68fa4fa890b898e7b7baf646c381e209b116f8 Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Wed, 1 Nov 2023 17:44:40 +0000 Subject: [PATCH] Return empty array when no rows results --- crates/ndc-sqlserver/tests/query_tests.rs | 6 +--- .../src/phases/translation/sql/ast.rs | 1 + .../src/phases/translation/sql/convert.rs | 3 +- .../src/phases/translation/sql/helpers.rs | 32 +++++++++++++------ .../tests__dup_array_relationship.snap | 6 ++-- .../tests__it_aggregate_count_albums.snap | 2 +- .../tests__it_converts_select_with_limit.snap | 2 +- .../tests__it_select_where_not_null.snap | 2 +- ...tests__it_select_where_related_exists.snap | 4 +-- .../tests__it_select_where_string.snap | 2 +- ...sts__it_select_where_unrelated_exists.snap | 2 +- .../tests__it_simple_array_relationship.snap | 4 +-- .../tests__it_simple_object_relationship.snap | 4 +-- .../snapshots/tests__nested_aggregates.snap | 4 +-- .../tests__nested_array_relationships.snap | 6 ++-- ...sorting_by_nested_relationship_column.snap | 2 +- ...tests__sorting_by_relationship_column.snap | 2 +- .../tests__sorting_by_relationship_count.snap | 2 +- justfile | 7 ++-- readme.md | 20 ++++++------ 20 files changed, 61 insertions(+), 52 deletions(-) diff --git a/crates/ndc-sqlserver/tests/query_tests.rs b/crates/ndc-sqlserver/tests/query_tests.rs index 776ce266..e059064f 100644 --- a/crates/ndc-sqlserver/tests/query_tests.rs +++ b/crates/ndc-sqlserver/tests/query_tests.rs @@ -36,14 +36,12 @@ mod predicates { insta::assert_json_snapshot!(result); } - /* - // this fails because empty responses don't return `{ rows: [] }` and instead return `{}` #[tokio::test] async fn select_where_album_id_less_than() { let result = run_query("select_where_album_id_less_than").await; insta::assert_json_snapshot!(result); } - */ + #[tokio::test] async fn select_where_album_id_less_than_or_equal_to() { let result = run_query("select_where_album_id_less_than_or_equal_to").await; @@ -86,14 +84,12 @@ mod predicates { insta::assert_json_snapshot!(result); } - // need to run query for each set of variables #[tokio::test] async fn select_where_variable() { let result = run_query("select_where_variable").await; insta::assert_json_snapshot!(result); } - // need to run query for each set of variables #[tokio::test] async fn select_where_variable_int() { let result = run_query("select_where_variable_int").await; diff --git a/crates/query-engine/src/phases/translation/sql/ast.rs b/crates/query-engine/src/phases/translation/sql/ast.rs index 77d9aff2..30a20b9f 100644 --- a/crates/query-engine/src/phases/translation/sql/ast.rs +++ b/crates/query-engine/src/phases/translation/sql/ast.rs @@ -201,6 +201,7 @@ pub enum BinaryArrayOperator { #[derive(Debug, Clone, PartialEq, Eq)] pub enum Function { Coalesce, + IsNull, JsonAgg, Unknown(String), } diff --git a/crates/query-engine/src/phases/translation/sql/convert.rs b/crates/query-engine/src/phases/translation/sql/convert.rs index c8e80578..3b2e7f20 100644 --- a/crates/query-engine/src/phases/translation/sql/convert.rs +++ b/crates/query-engine/src/phases/translation/sql/convert.rs @@ -365,6 +365,7 @@ impl Function { match self { Function::Coalesce => sql.append_syntax("coalesce"), Function::JsonAgg => sql.append_syntax("json_agg"), + Function::IsNull => sql.append_syntax("isnull"), Function::Unknown(name) => sql.append_syntax(name), } } @@ -386,7 +387,7 @@ impl CountType { impl Value { pub fn to_sql(&self, sql: &mut SQL) { match &self { - Value::EmptyJsonArray => sql.append_syntax("JSON_VALUE('[]','$')"), + Value::EmptyJsonArray => sql.append_syntax("'[]'"), Value::Int4(i) => sql.append_syntax(format!("{}", i).as_str()), Value::String(s) => sql.append_param(Param::String(s.clone())), Value::Variable(v) => sql.append_param(Param::Variable(v.clone())), diff --git a/crates/query-engine/src/phases/translation/sql/helpers.rs b/crates/query-engine/src/phases/translation/sql/helpers.rs index cbffb0d7..def31cc5 100644 --- a/crates/query-engine/src/phases/translation/sql/helpers.rs +++ b/crates/query-engine/src/phases/translation/sql/helpers.rs @@ -141,8 +141,8 @@ pub fn star_select(from: From) -> Select { /// given a set of rows and aggregate queries, combine them into /// one Select -/// SELECT JSON_VALUE([aggregates].[json], "$.aggregates") as [aggregates], -/// JSON_QUERY([rows].[json], "$.json") AS [rows] +/// SELECT JSON_VALUE([aggregates].[aggregates_json], "$.aggregates_json") as [aggregates], +/// JSON_QUERY(isnull([rows].[row_json],'[]'), "$.row_json") AS [rows] /// FROM ( /// SELECT * /// FROM ( @@ -177,10 +177,16 @@ pub fn select_rowset( SelectSet::Rows(row_select) => { let rows_row = vec![( make_column_alias("rows".to_string()), - Expression::ColumnName(ColumnName::AliasedColumn { - name: row_column_alias.clone(), - table: TableName::AliasedTable(row_table_alias.clone()), - }), + Expression::FunctionCall { + function: Function::IsNull, + args: vec![ + Expression::ColumnName(ColumnName::AliasedColumn { + name: row_column_alias.clone(), + table: TableName::AliasedTable(row_table_alias.clone()), + }), + Expression::Value(Value::EmptyJsonArray), + ], + }, )]; let mut final_row_select = simple_select(rows_row); @@ -227,10 +233,16 @@ pub fn select_rowset( ( make_column_alias("rows".to_string()), Expression::JsonQuery( - Box::new(Expression::ColumnName(ColumnName::AliasedColumn { - name: row_column_alias.clone(), - table: TableName::AliasedTable(row_table_alias.clone()), - })), + Box::new(Expression::FunctionCall { + function: Function::IsNull, + args: vec![ + Expression::ColumnName(ColumnName::AliasedColumn { + name: row_column_alias.clone(), + table: TableName::AliasedTable(row_table_alias.clone()), + }), + Expression::Value(Value::EmptyJsonArray), + ], + }), JsonPath { elements: vec![row_column_alias.clone()], }, diff --git a/crates/query-engine/tests/snapshots/tests__dup_array_relationship.snap b/crates/query-engine/tests/snapshots/tests__dup_array_relationship.snap index 44dc6a0c..a637d763 100644 --- a/crates/query-engine/tests/snapshots/tests__dup_array_relationship.snap +++ b/crates/query-engine/tests/snapshots/tests__dup_array_relationship.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -13,7 +13,7 @@ FROM [public].[Artist] AS [Artist] OUTER APPLY ( SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -32,7 +32,7 @@ FROM ) AS [albums]([json]) OUTER APPLY ( SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__it_aggregate_count_albums.snap b/crates/query-engine/tests/snapshots/tests__it_aggregate_count_albums.snap index 14f4d288..37612108 100644 --- a/crates/query-engine/tests/snapshots/tests__it_aggregate_count_albums.snap +++ b/crates/query-engine/tests/snapshots/tests__it_aggregate_count_albums.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - JSON_QUERY([rows].[row_json], '$.row_json') AS [rows], + JSON_QUERY(isnull([rows].[row_json], '[]'), '$.row_json') AS [rows], JSON_QUERY( JSON_VALUE([aggregates].[agg_json], '$.agg_json'), '$' diff --git a/crates/query-engine/tests/snapshots/tests__it_converts_select_with_limit.snap b/crates/query-engine/tests/snapshots/tests__it_converts_select_with_limit.snap index 4498a734..859c2046 100644 --- a/crates/query-engine/tests/snapshots/tests__it_converts_select_with_limit.snap +++ b/crates/query-engine/tests/snapshots/tests__it_converts_select_with_limit.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__it_select_where_not_null.snap b/crates/query-engine/tests/snapshots/tests__it_select_where_not_null.snap index c67183ed..d9f028c0 100644 --- a/crates/query-engine/tests/snapshots/tests__it_select_where_not_null.snap +++ b/crates/query-engine/tests/snapshots/tests__it_select_where_not_null.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__it_select_where_related_exists.snap b/crates/query-engine/tests/snapshots/tests__it_select_where_related_exists.snap index fd3a0dfc..89579938 100644 --- a/crates/query-engine/tests/snapshots/tests__it_select_where_related_exists.snap +++ b/crates/query-engine/tests/snapshots/tests__it_select_where_related_exists.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -13,7 +13,7 @@ FROM [public].[Artist] AS [artist] OUTER APPLY ( SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__it_select_where_string.snap b/crates/query-engine/tests/snapshots/tests__it_select_where_string.snap index ef07eadc..26f450f7 100644 --- a/crates/query-engine/tests/snapshots/tests__it_select_where_string.snap +++ b/crates/query-engine/tests/snapshots/tests__it_select_where_string.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__it_select_where_unrelated_exists.snap b/crates/query-engine/tests/snapshots/tests__it_select_where_unrelated_exists.snap index 7bc33932..bec69397 100644 --- a/crates/query-engine/tests/snapshots/tests__it_select_where_unrelated_exists.snap +++ b/crates/query-engine/tests/snapshots/tests__it_select_where_unrelated_exists.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__it_simple_array_relationship.snap b/crates/query-engine/tests/snapshots/tests__it_simple_array_relationship.snap index 2c561025..0e0a5e15 100644 --- a/crates/query-engine/tests/snapshots/tests__it_simple_array_relationship.snap +++ b/crates/query-engine/tests/snapshots/tests__it_simple_array_relationship.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -12,7 +12,7 @@ FROM [public].[Artist] AS [Artist] OUTER APPLY ( SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__it_simple_object_relationship.snap b/crates/query-engine/tests/snapshots/tests__it_simple_object_relationship.snap index de3336a7..f1fda2af 100644 --- a/crates/query-engine/tests/snapshots/tests__it_simple_object_relationship.snap +++ b/crates/query-engine/tests/snapshots/tests__it_simple_object_relationship.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -12,7 +12,7 @@ FROM [public].[Album] AS [Album] OUTER APPLY ( SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__nested_aggregates.snap b/crates/query-engine/tests/snapshots/tests__nested_aggregates.snap index c65b512a..45b4c585 100644 --- a/crates/query-engine/tests/snapshots/tests__nested_aggregates.snap +++ b/crates/query-engine/tests/snapshots/tests__nested_aggregates.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -13,7 +13,7 @@ FROM [public].[Artist] AS [Artist] OUTER APPLY ( SELECT - JSON_QUERY([rows].[row_json], '$.row_json') AS [rows], + JSON_QUERY(isnull([rows].[row_json], '[]'), '$.row_json') AS [rows], JSON_QUERY( JSON_VALUE([aggregates].[agg_json], '$.agg_json'), '$' diff --git a/crates/query-engine/tests/snapshots/tests__nested_array_relationships.snap b/crates/query-engine/tests/snapshots/tests__nested_array_relationships.snap index be7196e7..15663331 100644 --- a/crates/query-engine/tests/snapshots/tests__nested_array_relationships.snap +++ b/crates/query-engine/tests/snapshots/tests__nested_array_relationships.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -12,7 +12,7 @@ FROM [public].[Artist] AS [Artist] OUTER APPLY ( SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT @@ -21,7 +21,7 @@ FROM [public].[Album] AS [Album] OUTER APPLY ( SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__sorting_by_nested_relationship_column.snap b/crates/query-engine/tests/snapshots/tests__sorting_by_nested_relationship_column.snap index 9794e8ba..4fec262a 100644 --- a/crates/query-engine/tests/snapshots/tests__sorting_by_nested_relationship_column.snap +++ b/crates/query-engine/tests/snapshots/tests__sorting_by_nested_relationship_column.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_column.snap b/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_column.snap index c2837cfb..4ba74ce2 100644 --- a/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_column.snap +++ b/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_column.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_count.snap b/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_count.snap index 318e63f5..d5ce286a 100644 --- a/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_count.snap +++ b/crates/query-engine/tests/snapshots/tests__sorting_by_relationship_count.snap @@ -3,7 +3,7 @@ source: crates/query-engine/tests/tests.rs expression: result --- SELECT - [rows].[row_json] AS [rows] + isnull([rows].[row_json], '[]') AS [rows] FROM ( SELECT diff --git a/justfile b/justfile index 7a12a878..878ba2b0 100644 --- a/justfile +++ b/justfile @@ -146,15 +146,14 @@ run-engine: start-dependencies # pasting multiline SQL into `sqlcmd` is a bad time, so here is a script to # smash a file in for rapid fire application development business value -run-temp-sql: start-dependencies +run-temp-sql: + docker compose up --wait sqlserver sqlcmd -S localhost,64003 -U SA -P "Password!" -d "Chinook" -i "./temp.sql" ## repl-sqlserver: start a sqlserver docker image and connect to it using sqlcmd repl-sqlserver: #!/usr/bin/env bash - docker compose up -d sqlserver - # need a proper health check here instead ... but yolo - sleep 2 + docker compose up -wait sqlserver sqlcmd -S localhost,64003 -U SA -P "Password!" -d "Chinook" # run `clippy` linter diff --git a/readme.md b/readme.md index 4123c537..88be3f66 100644 --- a/readme.md +++ b/readme.md @@ -21,7 +21,6 @@ Things we definitely don't have: - database introspection - explain queries - reinstate benchmarks -- CI job for testing config server The best view of progress is probably `/crates/ndc-sqlserver/tests/`, and look at which tests are still commented out. If you'd to contribute, a very good @@ -33,16 +32,16 @@ start would be to uncomment one and try to fix any query errors. 1. Install [rustup](https://www.rust-lang.org/tools/install). 2. Install additional tools: - - `cargo install cargo-watch cargo-insta` - - `rustup component add rust-analyzer` - - `rustup component add clippy` - - `rustup component add rustfmt` + - `cargo install cargo-watch cargo-insta` + - `rustup component add rust-analyzer` + - `rustup component add clippy` + - `rustup component add rustfmt` 3. Install [just](https://github.com/casey/just) 4. Install [Docker](https://www.docker.com/) 5. Install protoc. Here are a few options: - - `brew install protobuf` - - `apt-get install protobuf-compiler` - - `dnf install protobuf-compiler` + - `brew install protobuf` + - `apt-get install protobuf-compiler` + - `dnf install protobuf-compiler` 6. Clone [v3-engine](https://github.com/hasura/v3-engine) in a directory near this one: ``` (cd .. && git clone git@github.com:hasura/v3-engine.git) @@ -69,8 +68,8 @@ just run ``` curl -H "Content-Type: application/json" \ --data "@crates/ndc-sqlserver/tests/goldenfiles/select_where_variable.json" \ - http://localhost:8100/query \ - | jq + http://localhost:8100/query \ + | jq ``` Among the docker containers is a Jaeger instance for tracing/debugging, accessible at http://127.0.0.1:4002. @@ -106,6 +105,7 @@ See [architecture.md](./architecture.md). } } ``` + (or `just test-integrated`) ## Write a database execution test