Merge pull request #14 from hasura/allow-explain-invalid-queries

remove error handling for invalid foreach requests

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.2.4]
+
+- Allow explain of invalid foreach queries. Will generate invalid SQL, for proper execution these should be parameterized
+
 ## [0.2.3]
 
 - Fix ordering of result sets for foreach queries (remote joins)

Cargo.toml

@@ -6,5 +6,5 @@ members = [
 ]
 resolver = "2"
 
-package.version = "0.2.3"
+package.version = "0.2.4"
 package.edition = "2021"

crates/ndc-clickhouse/src/connector/handler/explain.rs

@@ -49,7 +49,18 @@ pub async fn explain(
     let details = BTreeMap::from_iter(vec![
         (
             "SQL Query".to_string(),
-            pretty_print_sql(&unsafe_statement.to_unsafe_sql_string()),
+            add_variables_note(
+                &request,
+                &pretty_print_sql(&unsafe_statement.to_unsafe_sql_string()),
+            ),
+        ),
+        (
+            "Parameterized SQL Query".to_string(),
+            add_variables_note(&request, &pretty_print_sql(&statement_string)),
+        ),
+        (
+            "Parameters".to_string(),
+            serde_json::to_string(&parameters).map_err(|err| ExplainError::Other(Box::new(err)))?,
         ),
         ("Execution Plan".to_string(), explain),
     ]);
@@ -68,3 +79,19 @@ fn pretty_print_sql(query: &str) -> String {
 
     format(query, &params, options)
 }
+
+const EXPLAIN_NOTE: &str = r#"-- note: the source object for _vars should be a JSON string of the form
+-- `{"_varset_id": [1,2,3], "_var_ID": [1,2,3], "_var_NAME": ["Name1","Name2","Name3"]}`
+-- The example assumes the variables ID and NAME, change as appropriate. "_varset_id" is an index starting from 1
+-- Each array member corresponds to a row, all arrays should have the same number of members. See clickhouse docs for more:
+-- https://clickhouse.com/docs/en/interfaces/formats#jsoncolumns
+-- https://clickhouse.com/docs/en/sql-reference/table-functions/format
+"#;
+
+fn add_variables_note(request: &models::QueryRequest, statement: &str) -> String {
+    if request.variables.is_some() {
+        format!("{EXPLAIN_NOTE}\n{statement}")
+    } else {
+        statement.to_owned()
+    }
+}

crates/ndc-clickhouse/src/sql/query_builder.rs

@@ -111,9 +111,6 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
         };
 
         let with = if let Some(variables) = &self.request.variables {
-            if variables.is_empty() {
-                return Err(QueryBuilderError::EmptyQueryVariablesList);
-            }
             let mut variable_values: IndexMap<String, Vec<serde_json::Value>> = IndexMap::new();
 
             variable_values.insert(
@@ -1658,7 +1655,6 @@ impl<'r, 'c> QueryBuilder<'r, 'c> {
             }
         }
     }
-
     fn collection_relationship(
         &self,
         relationship: &str,

crates/ndc-clickhouse/src/sql/query_builder/error.rs

@@ -28,8 +28,6 @@ pub enum QueryBuilderError {
     Unexpected(String),
     /// There was an issue creating typecasting strings
     Typecasting(String),
-    /// An empty list of variables was passed. If variables are passed, we expect at least one set.
-    EmptyQueryVariablesList,
 }
 
 impl fmt::Display for QueryBuilderError {
@@ -66,10 +64,6 @@ impl fmt::Display for QueryBuilderError {
             QueryBuilderError::NotSupported(e) => write!(f, "Not supported: {e}"),
             QueryBuilderError::Unexpected(e) => write!(f, "Unexpected: {e}"),
             QueryBuilderError::Typecasting(e) => write!(f, "Typecasting: {e}"),
-            QueryBuilderError::EmptyQueryVariablesList => write!(
-                f,
-                "Empty query variables list: we expect at least one set, or no list."
-            ),
         }
     }
 }