From 2246952e6d6a1410035f7cdf0dc7c5a8e56e0252 Mon Sep 17 00:00:00 2001 From: Brandon Martin Date: Thu, 12 Dec 2024 07:52:19 -0700 Subject: [PATCH] Clippy, formatting, and tests --- crates/configuration/src/introspection.rs | 6 ++- ...e_stored_procedure_with_relationships.snap | 14 +++--- crates/query-engine/sql/src/sql/helpers.rs | 18 ++++---- .../src/translation/query/aggregates.rs | 46 +++++++++++-------- .../translation/src/translation/query/mod.rs | 10 +++- .../translation/src/translation/query/root.rs | 10 ++-- 6 files changed, 62 insertions(+), 42 deletions(-) diff --git a/crates/configuration/src/introspection.rs b/crates/configuration/src/introspection.rs index 5551fcb1..a2701c0d 100644 --- a/crates/configuration/src/introspection.rs +++ b/crates/configuration/src/introspection.rs @@ -1,6 +1,6 @@ //! Configuration and state for our connector. -use serde::Deserialize; use ndc_models::TypeRepresentation; +use serde::Deserialize; #[derive(Deserialize, Debug)] pub struct IntrospectStoredProcedureArgument { @@ -80,7 +80,9 @@ pub fn map_type_representation(sql_type: &str) -> Option { "datetime" | "datetime2" | "smalldatetime" => Some(TypeRepresentation::Timestamp), "datetimeoffset" => Some(TypeRepresentation::TimestampTZ), "time" | "timestamp" => Some(TypeRepresentation::String), - "char" | "varchar" | "text" | "nchar" | "nvarchar" | "ntext" => Some(TypeRepresentation::String), + "char" | "varchar" | "text" | "nchar" | "nvarchar" | "ntext" => { + Some(TypeRepresentation::String) + } "binary" | "varbinary" | "image" => Some(TypeRepresentation::String), "uniqueidentifier" => Some(TypeRepresentation::UUID), "xml" => Some(TypeRepresentation::String), diff --git a/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_with_relationships.snap b/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_with_relationships.snap index d76bbcb8..aa531911 100644 --- a/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_with_relationships.snap +++ b/crates/ndc-sqlserver/tests/snapshots/mutation_tests__stored_procedures__execute_stored_procedure_with_relationships.snap @@ -13,31 +13,31 @@ expression: result "rows": [ { "InvoiceId": 98, - "Total": 3.98 + "Total": "3.98" }, { "InvoiceId": 121, - "Total": 3.96 + "Total": "3.96" }, { "InvoiceId": 143, - "Total": 5.94 + "Total": "5.94" }, { "InvoiceId": 195, - "Total": 0.99 + "Total": "0.99" }, { "InvoiceId": 316, - "Total": 1.98 + "Total": "1.98" }, { "InvoiceId": 327, - "Total": 13.86 + "Total": "13.86" }, { "InvoiceId": 382, - "Total": 8.91 + "Total": "8.91" } ] } diff --git a/crates/query-engine/sql/src/sql/helpers.rs b/crates/query-engine/sql/src/sql/helpers.rs index 8f24b2d6..e15181ce 100644 --- a/crates/query-engine/sql/src/sql/helpers.rs +++ b/crates/query-engine/sql/src/sql/helpers.rs @@ -50,22 +50,24 @@ pub fn make_column( table: TableReference, name: ColumnName, alias: ColumnAlias, - column_type: ScalarType, + column_type: &ScalarType, ) -> (ColumnAlias, Expression) { ( alias, - cast_column(column_type, Expression::ColumnReference(ColumnReference::TableColumn { table, name })), + cast_column( + column_type, + Expression::ColumnReference(ColumnReference::TableColumn { table, name }), + ), ) } /// Should we cast the column to a specific type? -pub fn cast_column(column_type: ScalarType, expression: Expression) -> Expression { +pub fn cast_column(column_type: &ScalarType, expression: Expression) -> Expression { match column_type.0.as_str() { - "bigint" | "decimal" | "numeric" | "money" | "smallmoney" => - Expression::Cast { - expression: Box::new(expression), - r#type: ScalarType("varchar".to_string()), - }, + "bigint" | "decimal" | "numeric" | "money" | "smallmoney" => Expression::Cast { + expression: Box::new(expression), + r#type: ScalarType("varchar".to_string()), + }, _ => expression, } } diff --git a/crates/query-engine/translation/src/translation/query/aggregates.rs b/crates/query-engine/translation/src/translation/query/aggregates.rs index d1785f13..18e4fce3 100644 --- a/crates/query-engine/translation/src/translation/query/aggregates.rs +++ b/crates/query-engine/translation/src/translation/query/aggregates.rs @@ -2,8 +2,11 @@ use indexmap::IndexMap; -use crate::translation::{error::Error, helpers::{Env, CollectionOrProcedureInfo}}; -use query_engine_sql::sql::{self, helpers::cast_column, ast::ScalarType}; +use crate::translation::{ + error::Error, + helpers::{CollectionOrProcedureInfo, Env}, +}; +use query_engine_sql::sql::{self, ast::ScalarType, helpers::cast_column}; /// Translate any aggregates we should include in the query into our SQL AST. pub fn translate( @@ -48,22 +51,25 @@ pub fn translate( }, ); let column_type = match collection_info { - CollectionOrProcedureInfo::Collection(ci) => { - match ci { - crate::translation::helpers::CollectionInfo::Table { info, .. } => { - info.columns.get(column.as_str()).map(|c| c.r#type.clone()) - }, - crate::translation::helpers::CollectionInfo::NativeQuery { info, .. } => { - info.columns.get(column.as_str()).map(|c| c.r#type.clone()) - }, - _ => None, + CollectionOrProcedureInfo::Collection(ci) => match ci { + crate::translation::helpers::CollectionInfo::Table { info, .. } => { + info.columns.get(column.as_str()).map(|c| c.r#type.clone()) } - } - CollectionOrProcedureInfo::Procedure(_) => None + crate::translation::helpers::CollectionInfo::NativeQuery { + info, + .. + } => info.columns.get(column.as_str()).map(|c| c.r#type.clone()), + _ => None, + }, + CollectionOrProcedureInfo::Procedure(_) => None, }; - let function_type = column_type.and_then(|column_type| env.metadata.aggregate_functions.0 - .get(&column_type) - .and_then(|functions| functions.get(function.as_str()))); + let function_type = column_type.and_then(|column_type| { + env.metadata + .aggregate_functions + .0 + .get(&column_type) + .and_then(|functions| functions.get(function.as_str())) + }); match function.as_str() { "SUM" | "AVG" => sql::ast::Expression::Cast { expression: Box::new(sql::ast::Expression::FunctionCall { @@ -81,12 +87,12 @@ pub fn translate( args: vec![column_ref_expression], }; match function_type.map(|f| f.return_type.clone()) { - Some(scalar_type) => cast_column(ScalarType(scalar_type.0), expression), - None => expression + Some(scalar_type) => { + cast_column(&ScalarType(scalar_type.0), expression) + } + None => expression, } } - - } } ndc_models::Aggregate::StarCount {} => { diff --git a/crates/query-engine/translation/src/translation/query/mod.rs b/crates/query-engine/translation/src/translation/query/mod.rs index 64a35032..c8a6091d 100644 --- a/crates/query-engine/translation/src/translation/query/mod.rs +++ b/crates/query-engine/translation/src/translation/query/mod.rs @@ -89,8 +89,14 @@ pub fn translate_query( )?; // translate aggregate select. if there are no fields, make this a None - let aggregate_select = - root::translate_aggregate_query(env, state, collection_info, current_table, from_clause, query)?; + let aggregate_select = root::translate_aggregate_query( + env, + state, + collection_info, + current_table, + from_clause, + query, + )?; match (row_select, aggregate_select) { (Some(rows), None) => Ok(sql::helpers::SelectSet::Rows(rows)), diff --git a/crates/query-engine/translation/src/translation/query/root.rs b/crates/query-engine/translation/src/translation/query/root.rs index 0076f2d3..c0aa4629 100644 --- a/crates/query-engine/translation/src/translation/query/root.rs +++ b/crates/query-engine/translation/src/translation/query/root.rs @@ -30,8 +30,12 @@ pub fn translate_aggregate_query( None => Ok(None), Some(aggregate_fields) => { // create all aggregate columns - let aggregate_columns = - aggregates::translate(¤t_table.reference, aggregate_fields, collection_info, env)?; + let aggregate_columns = aggregates::translate( + ¤t_table.reference, + aggregate_fields, + collection_info, + env, + )?; // create the select clause and the joins, order by, where clauses. // We don't add the limit afterwards. @@ -191,7 +195,7 @@ pub fn translate_rows_query( current_table.reference.clone(), column_info.name, sql::helpers::make_column_alias(alias.to_string()), - ScalarType(column_info.r#type.0), + &ScalarType(column_info.r#type.0), )) } ndc_models::Field::Relationship {