Merge pull request #24 from hasura:fix-variables-again

Fix-variables-again
hasura · Aug 17, 2024 · a5df7ec · a5df7ec
2 parents 3da8729 + f10c9d3
commit a5df7ec
Show file tree

Hide file tree

Showing 12 changed files with 121 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [1.0.1]
+
+- Bug fix: remove erroneous group by and order by clauses in `foreach` queries. Remote relationships should now function as expected. The previous fix was incorrect.
+
 ## [1.0.0]
 
 - Stable Release

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -6,5 +6,5 @@ members = [
 ]
 resolver = "2"
 
-package.version = "1.0.0"
+package.version = "1.0.1"
 package.edition = "2021"
diff --git a/Dockerfile b/Dockerfile
@@ -1,5 +1,5 @@
 # https://github.com/LukeMathWalker/cargo-chef
-FROM rust:1.75.0 as chef
+FROM rust:1.80.1 as chef
 RUN cargo install cargo-chef
 WORKDIR /app
 

diff --git a/crates/ndc-clickhouse/src/sql/query_builder.rs b/crates/ndc-clickhouse/src/sql/query_builder.rs
@@ -165,34 +165,10 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
             vec![rowset_subquery.into_table_with_joins(vec![])]
         };
 
-        let order_by = if self.request.variables.is_some() {
-            vec![OrderByExpr {
-                expr: Expr::CompoundIdentifier(vec![
-                    Ident::new_quoted("_vars"),
-                    Ident::new_quoted("_varset_id"),
-                ]),
-                asc: Some(true),
-                nulls_first: None,
-            }]
-        } else {
-            vec![]
-        };
-
-        let group_by = if self.request.variables.is_some() {
-            vec![Expr::CompoundIdentifier(vec![
-                Ident::new_quoted("_vars"),
-                Ident::new_quoted("_varset_id"),
-            ])]
-        } else {
-            vec![]
-        };
-
         Ok(Query::new()
             .with(with)
             .select(select)
             .from(from)
-            .group_by(group_by)
-            .order_by(order_by)
             .into_statement()
             .format("TabSeparatedRaw"))
     }

diff --git a/crates/ndc-clickhouse/tests/query_builder.rs b/crates/ndc-clickhouse/tests/query_builder.rs
@@ -252,6 +252,10 @@ mod variables {
     async fn empty_variable_sets() -> Result<(), Box<dyn Error>> {
         test_generated_sql("02_empty_variable_sets").await
     }
+    #[tokio::test]
+    async fn singe_set() -> Result<(), Box<dyn Error>> {
+        test_generated_sql("03_single_set").await
+    }
 }
 
 #[cfg(test)]

diff --git a/.../ndc-clickhouse/tests/query_builder/chinook/03_variables/01_simple_predicate.request.json b/.../ndc-clickhouse/tests/query_builder/chinook/03_variables/01_simple_predicate.request.json
@@ -4,6 +4,9 @@
     "variables": [
         {
             "ArtistId": 1
+        },
+        {
+            "ArtistId": 2
         }
     ],
     "query": {

diff --git a/...ndc-clickhouse/tests/query_builder/chinook/03_variables/01_simple_predicate.statement.sql b/...ndc-clickhouse/tests/query_builder/chinook/03_variables/01_simple_predicate.statement.sql
@@ -4,7 +4,7 @@ WITH "_vars" AS (
   FROM
     format(
       JSONColumns,
-      '{"_varset_id":[1],"_var_ArtistId":[1]}'
+      '{"_varset_id":[1,2],"_var_ArtistId":[1,2]}'
     )
 )
 SELECT
@@ -45,8 +45,4 @@ FROM
       ) AS "_row"
     GROUP BY
       "_row"."_varset_id"
-  ) AS "_rowset" ON "_vars"."_varset_id" = "_rowset"."_varset_id"
-GROUP BY
-  "_vars"."_varset_id"
-ORDER BY
-  "_vars"."_varset_id" ASC FORMAT TabSeparatedRaw;
+  ) AS "_rowset" ON "_vars"."_varset_id" = "_rowset"."_varset_id" FORMAT TabSeparatedRaw;
diff --git a/...-clickhouse/tests/query_builder/chinook/03_variables/02_empty_variable_sets.statement.sql b/...-clickhouse/tests/query_builder/chinook/03_variables/02_empty_variable_sets.statement.sql
@@ -42,8 +42,4 @@ FROM
       ) AS "_row"
     GROUP BY
       "_row"."_varset_id"
-  ) AS "_rowset" ON "_vars"."_varset_id" = "_rowset"."_varset_id"
-GROUP BY
-  "_vars"."_varset_id"
-ORDER BY
-  "_vars"."_varset_id" ASC FORMAT TabSeparatedRaw;
+  ) AS "_rowset" ON "_vars"."_varset_id" = "_rowset"."_varset_id" FORMAT TabSeparatedRaw;
diff --git a/crates/ndc-clickhouse/tests/query_builder/chinook/03_variables/03_single_set.request.json b/crates/ndc-clickhouse/tests/query_builder/chinook/03_variables/03_single_set.request.json
@@ -0,0 +1,53 @@
+{
+    "$schema": "../../request.schema.json",
+    "collection": "Chinook_Album",
+    "variables": [
+        {
+            "ArtistId": 1
+        }
+    ],
+    "query": {
+        "fields": {
+            "albumId": {
+                "type": "column",
+                "column": "AlbumId",
+                "fields": null
+            },
+            "artistId": {
+                "type": "column",
+                "column": "ArtistId",
+                "fields": null
+            },
+            "title": {
+                "type": "column",
+                "column": "Title",
+                "fields": null
+            }
+        },
+        "predicate": {
+            "type": "and",
+            "expressions": [
+                {
+                    "type": "and",
+                    "expressions": [
+                        {
+                            "type": "binary_comparison_operator",
+                            "column": {
+                                "type": "column",
+                                "name": "ArtistId",
+                                "path": []
+                            },
+                            "operator": "_eq",
+                            "value": {
+                                "type": "variable",
+                                "name": "ArtistId"
+                            }
+                        }
+                    ]
+                }
+            ]
+        }
+    },
+    "arguments": {},
+    "collection_relationships": {}
+}
diff --git a/crates/ndc-clickhouse/tests/query_builder/chinook/03_variables/03_single_set.statement.sql b/crates/ndc-clickhouse/tests/query_builder/chinook/03_variables/03_single_set.statement.sql
@@ -0,0 +1,48 @@
+WITH "_vars" AS (
+  SELECT
+    *
+  FROM
+    format(
+      JSONColumns,
+      '{"_varset_id":[1],"_var_ArtistId":[1]}'
+    )
+)
+SELECT
+  toJSONString(
+    groupArray(
+      cast(
+        "_rowset"."_rowset",
+        'Tuple(rows Array(Tuple("albumId" Int32, "artistId" Int32, "title" String)))'
+      )
+    )
+  ) AS "rowsets"
+FROM
+  "_vars" AS "_vars"
+  LEFT JOIN (
+    SELECT
+      tuple(
+        groupArray(
+          tuple(
+            "_row"."_field_albumId",
+            "_row"."_field_artistId",
+            "_row"."_field_title"
+          )
+        )
+      ) AS "_rowset",
+      "_row"."_varset_id" AS "_varset_id"
+    FROM
+      (
+        SELECT
+          "_origin"."AlbumId" AS "_field_albumId",
+          "_origin"."ArtistId" AS "_field_artistId",
+          "_origin"."Title" AS "_field_title",
+          "_vars"."_varset_id" AS "_varset_id"
+        FROM
+          "_vars" AS "_vars"
+          CROSS JOIN "Chinook"."Album" AS "_origin"
+        WHERE
+          "_origin"."ArtistId" = "_vars"."_var_ArtistId"
+      ) AS "_row"
+    GROUP BY
+      "_row"."_varset_id"
+  ) AS "_rowset" ON "_vars"."_varset_id" = "_rowset"."_varset_id" FORMAT TabSeparatedRaw;
diff --git a/docker-compose.yaml b/docker-compose.yaml
@@ -1,5 +1,6 @@
 services:
   ndc-clickhouse:
+    image: ndc-clickhouse-debug
     build:
       context: .
       args: