Skip to content

Commit 93e2c62

Browse files
codedmartcodedmart
committed
Feedback
1 parent fae7a6f commit 93e2c62

File tree

6 files changed

+274
-336
lines changed

6 files changed

+274
-336
lines changed

‎crates/configuration/src/to_runtime_configuration.rs

+12-13
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ fn convert_scalar_types(
8888
aggregate_functions: scalar_type.aggregate_functions,
8989
comparison_operators: scalar_type.comparison_operators,
9090
type_representation: scalar_type.type_representation.clone(),
91-
full_type_representation: scalar_type.type_representation,
9291
},
9392
)
9493
})
@@ -139,7 +138,7 @@ fn convert_read_only_column_info(
139138
) -> query_engine_metadata::metadata::ReadOnlyColumnInfo {
140139
query_engine_metadata::metadata::ReadOnlyColumnInfo {
141140
name: read_only_column_info.name,
142-
r#type: convert_type(read_only_column_info.r#type),
141+
r#type: read_only_column_info.r#type,
143142
nullable: convert_nullable(&read_only_column_info.nullable),
144143
description: read_only_column_info.description,
145144
}
@@ -152,16 +151,16 @@ fn convert_nullable(nullable: &metadata::Nullable) -> query_engine_metadata::met
152151
}
153152
}
154153

155-
fn convert_type(r#type: metadata::Type) -> query_engine_metadata::metadata::Type {
156-
match r#type {
157-
metadata::Type::ArrayType(t) => {
158-
query_engine_metadata::metadata::Type::ArrayType(Box::new(convert_type(*t)))
159-
}
160-
metadata::Type::RangeType(t) => query_engine_metadata::metadata::Type::RangeType(t),
161-
metadata::Type::StructType(t) => query_engine_metadata::metadata::Type::StructType(t),
162-
metadata::Type::ScalarType(t) => query_engine_metadata::metadata::Type::ScalarType(t),
163-
}
164-
}
154+
// fn convert_type(r#type: metadata::Type) -> query_engine_metadata::metadata::Type {
155+
// match r#type {
156+
// metadata::Type::ArrayType(t) => {
157+
// query_engine_metadata::metadata::Type::ArrayType(Box::new(convert_type(*t)))
158+
// }
159+
// metadata::Type::RangeType(t) => query_engine_metadata::metadata::Type::RangeType(t),
160+
// metadata::Type::StructType(t) => query_engine_metadata::metadata::Type::StructType(t),
161+
// metadata::Type::ScalarType(t) => query_engine_metadata::metadata::Type::ScalarType(t),
162+
// }
163+
// }
165164

166165
fn convert_native_query_sql_either(
167166
sql: metadata::NativeQuerySqlEither,
@@ -323,7 +322,7 @@ fn convert_column_info(
323322
) -> query_engine_metadata::metadata::ColumnInfo {
324323
query_engine_metadata::metadata::ColumnInfo {
325324
name: column_info.name,
326-
r#type: convert_type(column_info.r#type),
325+
r#type: column_info.r#type,
327326
nullable: convert_nullable(&column_info.nullable),
328327
has_default: convert_has_default(&column_info.has_default),
329328
is_identity: convert_is_identity(&column_info.is_identity),

‎crates/configuration/src/version1.rs

+48-55
Original file line numberDiff line numberDiff line change
@@ -175,15 +175,9 @@ pub async fn configure(
175175
if let Some(column) = columns.into_iter().next() {
176176
if let Some(value) = column.value {
177177
if let serde_json::Value::String(str) = value {
178-
match serde_json::from_str::<TablesInfo>(&str) {
179-
Ok(table_info_map) => {
180-
// tables_info.merge(table_info_map);
181-
Ok(table_info_map)
182-
}
183-
Err(err) => {
184-
Err(format!("Failed to deserialize TablesInfo from JSON: {err}"))
185-
}
186-
}
178+
serde_json::from_str::<TablesInfo>(&str).map_err(|err| {
179+
format!("Failed to deserialize TablesInfo from JSON: {err}")
180+
})
187181
} else {
188182
Err(format!("Expected a string value, found: {value:?}"))
189183
}
@@ -288,7 +282,7 @@ struct TypeItem {
288282
}
289283

290284
fn get_aggregate_functions_for_type(
291-
type_representation: &Option<database::TypeRepresentation>,
285+
type_representation: &Option<database::BigQueryType>,
292286
type_name: &ScalarTypeName,
293287
) -> BTreeMap<AggregateFunctionName, database::AggregateFunction> {
294288
let mut aggregate_functions = BTreeMap::new();
@@ -297,15 +291,15 @@ fn get_aggregate_functions_for_type(
297291
Some(type_rep) => {
298292
if matches!(
299293
type_rep,
300-
database::TypeRepresentation::Int64
301-
| database::TypeRepresentation::Float64
302-
| database::TypeRepresentation::Numeric
303-
| database::TypeRepresentation::BigNumeric
304-
| database::TypeRepresentation::String
305-
| database::TypeRepresentation::Date
306-
| database::TypeRepresentation::Datetime
307-
| database::TypeRepresentation::Timestamp
308-
| database::TypeRepresentation::Time
294+
database::BigQueryType::Int64
295+
| database::BigQueryType::Float64
296+
| database::BigQueryType::Numeric
297+
| database::BigQueryType::BigNumeric
298+
| database::BigQueryType::String
299+
| database::BigQueryType::Date
300+
| database::BigQueryType::Datetime
301+
| database::BigQueryType::Timestamp
302+
| database::BigQueryType::Time
309303
) {
310304
aggregate_functions.insert(
311305
AggregateFunctionName::new("MIN".into()),
@@ -323,10 +317,10 @@ fn get_aggregate_functions_for_type(
323317

324318
if matches!(
325319
type_rep,
326-
database::TypeRepresentation::Int64
327-
| database::TypeRepresentation::Float64
328-
| database::TypeRepresentation::Numeric
329-
| database::TypeRepresentation::BigNumeric
320+
database::BigQueryType::Int64
321+
| database::BigQueryType::Float64
322+
| database::BigQueryType::Numeric
323+
| database::BigQueryType::BigNumeric
330324
) {
331325
aggregate_functions.insert(
332326
AggregateFunctionName::new("AVG".into()),
@@ -378,7 +372,6 @@ fn get_scalar_types(type_names: &Vec<TypeItem>, schema_name: String) -> database
378372
aggregate_functions: get_aggregate_functions_for_type(&type_rep, &scalar_type_name),
379373
description: None,
380374
type_representation: type_rep.clone(),
381-
full_type_representation: type_rep.clone(),
382375
},
383376
);
384377
}
@@ -390,7 +383,7 @@ fn get_scalar_types(type_names: &Vec<TypeItem>, schema_name: String) -> database
390383
// we look up available types in `sys.types` but hard code their behaviour by looking them up below
391384
// categories taken from https://learn.microsoft.com/en-us/sql/t-sql/data-types/data-types-transact-sql
392385
fn get_comparison_operators_for_type(
393-
type_representation: &Option<database::TypeRepresentation>,
386+
type_representation: &Option<database::BigQueryType>,
394387
type_name: &ScalarTypeName,
395388
) -> BTreeMap<ComparisonOperatorName, database::ComparisonOperator> {
396389
let mut comparison_operators = BTreeMap::new();
@@ -399,11 +392,11 @@ fn get_comparison_operators_for_type(
399392
Some(type_rep) => {
400393
if !matches!(
401394
type_rep,
402-
database::TypeRepresentation::Array(_)
403-
| database::TypeRepresentation::Bytes
404-
| database::TypeRepresentation::Json
405-
| database::TypeRepresentation::Geography
406-
| database::TypeRepresentation::Struct(_)
395+
database::BigQueryType::Array(_)
396+
| database::BigQueryType::Bytes
397+
| database::BigQueryType::Json
398+
| database::BigQueryType::Geography
399+
| database::BigQueryType::Struct(_)
407400
) {
408401
comparison_operators.insert(
409402
ComparisonOperatorName::new("_eq".into()),
@@ -428,7 +421,7 @@ fn get_comparison_operators_for_type(
428421

429422
if matches!(
430423
type_rep,
431-
database::TypeRepresentation::String | database::TypeRepresentation::Bytes
424+
database::BigQueryType::String | database::BigQueryType::Bytes
432425
) {
433426
comparison_operators.insert(
434427
ComparisonOperatorName::new("_like".into()),
@@ -452,11 +445,11 @@ fn get_comparison_operators_for_type(
452445

453446
if !matches!(
454447
type_rep,
455-
database::TypeRepresentation::Array(_)
456-
| database::TypeRepresentation::Json
457-
| database::TypeRepresentation::Bytes
458-
| database::TypeRepresentation::Geography
459-
| database::TypeRepresentation::Struct(_)
448+
database::BigQueryType::Array(_)
449+
| database::BigQueryType::Json
450+
| database::BigQueryType::Bytes
451+
| database::BigQueryType::Geography
452+
| database::BigQueryType::Struct(_)
460453
) {
461454
comparison_operators.insert(
462455
ComparisonOperatorName::new("_neq".into()),
@@ -512,21 +505,21 @@ fn get_comparison_operators_for_type(
512505
comparison_operators
513506
}
514507

515-
fn get_type_representation(type_item: &TypeItem) -> Option<database::TypeRepresentation> {
508+
fn get_type_representation(type_item: &TypeItem) -> Option<database::BigQueryType> {
516509
match type_item.name.as_str().to_lowercase().as_str() {
517-
"string" => Some(database::TypeRepresentation::String),
518-
"bytes" => Some(database::TypeRepresentation::Bytes),
519-
"int64" => Some(database::TypeRepresentation::Int64),
520-
"float64" => Some(database::TypeRepresentation::Float64),
521-
"bool" => Some(database::TypeRepresentation::Boolean),
522-
"numeric" => Some(database::TypeRepresentation::Numeric),
523-
"bignumeric" => Some(database::TypeRepresentation::BigNumeric),
524-
"geography" => Some(database::TypeRepresentation::Geography),
525-
"date" => Some(database::TypeRepresentation::Date),
526-
"datetime" => Some(database::TypeRepresentation::Datetime),
527-
"time" => Some(database::TypeRepresentation::Time),
528-
"timestamp" => Some(database::TypeRepresentation::Timestamp),
529-
"json" => Some(database::TypeRepresentation::Json),
510+
"string" => Some(database::BigQueryType::String),
511+
"bytes" => Some(database::BigQueryType::Bytes),
512+
"int64" => Some(database::BigQueryType::Int64),
513+
"float64" => Some(database::BigQueryType::Float64),
514+
"bool" => Some(database::BigQueryType::Boolean),
515+
"numeric" => Some(database::BigQueryType::Numeric),
516+
"bignumeric" => Some(database::BigQueryType::BigNumeric),
517+
"geography" => Some(database::BigQueryType::Geography),
518+
"date" => Some(database::BigQueryType::Date),
519+
"datetime" => Some(database::BigQueryType::Datetime),
520+
"time" => Some(database::BigQueryType::Time),
521+
"timestamp" => Some(database::BigQueryType::Timestamp),
522+
"json" => Some(database::BigQueryType::Json),
530523
t if t.starts_with("range<") => {
531524
let inner_type = t.trim_start_matches("range<").trim_end_matches('>');
532525
let range_type = match inner_type {
@@ -535,15 +528,15 @@ fn get_type_representation(type_item: &TypeItem) -> Option<database::TypeReprese
535528
"timestamp" => database::TypeRange::Timestamp,
536529
_ => database::TypeRange::Date, // Default to Date if unrecognized
537530
};
538-
Some(database::TypeRepresentation::Range(Box::new(range_type)))
531+
Some(database::BigQueryType::Range(Box::new(range_type)))
539532
}
540533
t if t.starts_with("array<") => {
541534
let inner_type = t.trim_start_matches("array<").trim_end_matches('>');
542535
let inner_repr = get_type_representation(&TypeItem {
543536
name: ScalarTypeName::new(inner_type.into()),
544537
})
545-
.unwrap_or(database::TypeRepresentation::String);
546-
Some(database::TypeRepresentation::Array(Box::new(inner_repr)))
538+
.unwrap_or(database::BigQueryType::String);
539+
Some(database::BigQueryType::Array(Box::new(inner_repr)))
547540
}
548541
t if t.starts_with("struct<") => {
549542
let fields = t.trim_start_matches("struct<").trim_end_matches('>');
@@ -555,11 +548,11 @@ fn get_type_representation(type_item: &TypeItem) -> Option<database::TypeReprese
555548
let field_type = get_type_representation(&TypeItem {
556549
name: ScalarTypeName::new(parts[1].into()),
557550
})
558-
.unwrap_or(database::TypeRepresentation::String);
551+
.unwrap_or(database::BigQueryType::String);
559552
struct_fields.insert(field_name, Box::new(field_type));
560553
}
561554
}
562-
Some(database::TypeRepresentation::Struct(struct_fields))
555+
Some(database::BigQueryType::Struct(struct_fields))
563556
}
564557
_ => None,
565558
}

‎crates/connectors/ndc-bigquery/src/schema.rs

+17-17
Original file line numberDiff line numberDiff line change
@@ -183,25 +183,25 @@ pub async fn get_schema(
183183

184184
/// Map our local type representation to ndc-spec type representation.
185185
fn map_type_representation(
186-
type_representation: &metadata::TypeRepresentation,
186+
type_representation: &metadata::BigQueryType,
187187
) -> models::TypeRepresentation {
188188
match type_representation {
189-
metadata::TypeRepresentation::Array(_) => models::TypeRepresentation::JSON,
190-
metadata::TypeRepresentation::BigNumeric => models::TypeRepresentation::BigDecimal,
191-
metadata::TypeRepresentation::Boolean => models::TypeRepresentation::Boolean,
192-
metadata::TypeRepresentation::Bytes => models::TypeRepresentation::Bytes,
193-
metadata::TypeRepresentation::Date => models::TypeRepresentation::Date,
194-
metadata::TypeRepresentation::Datetime => models::TypeRepresentation::TimestampTZ,
195-
metadata::TypeRepresentation::Float64 => models::TypeRepresentation::Float64,
196-
metadata::TypeRepresentation::Geography => models::TypeRepresentation::Geography,
197-
metadata::TypeRepresentation::Int64 => models::TypeRepresentation::Int64,
198-
metadata::TypeRepresentation::Json => models::TypeRepresentation::JSON,
199-
metadata::TypeRepresentation::Numeric => models::TypeRepresentation::BigDecimal,
200-
metadata::TypeRepresentation::Range(_) => models::TypeRepresentation::JSON,
201-
metadata::TypeRepresentation::String => models::TypeRepresentation::String,
202-
metadata::TypeRepresentation::Struct(_) => models::TypeRepresentation::JSON,
203-
metadata::TypeRepresentation::Time => models::TypeRepresentation::String,
204-
metadata::TypeRepresentation::Timestamp => models::TypeRepresentation::TimestampTZ,
189+
metadata::BigQueryType::Array(_) => models::TypeRepresentation::JSON,
190+
metadata::BigQueryType::BigNumeric => models::TypeRepresentation::BigDecimal,
191+
metadata::BigQueryType::Boolean => models::TypeRepresentation::Boolean,
192+
metadata::BigQueryType::Bytes => models::TypeRepresentation::Bytes,
193+
metadata::BigQueryType::Date => models::TypeRepresentation::Date,
194+
metadata::BigQueryType::Datetime => models::TypeRepresentation::TimestampTZ,
195+
metadata::BigQueryType::Float64 => models::TypeRepresentation::Float64,
196+
metadata::BigQueryType::Geography => models::TypeRepresentation::Geography,
197+
metadata::BigQueryType::Int64 => models::TypeRepresentation::Int64,
198+
metadata::BigQueryType::Json => models::TypeRepresentation::JSON,
199+
metadata::BigQueryType::Numeric => models::TypeRepresentation::BigDecimal,
200+
metadata::BigQueryType::Range(_) => models::TypeRepresentation::JSON,
201+
metadata::BigQueryType::String => models::TypeRepresentation::String,
202+
metadata::BigQueryType::Struct(_) => models::TypeRepresentation::JSON,
203+
metadata::BigQueryType::Time => models::TypeRepresentation::String,
204+
metadata::BigQueryType::Timestamp => models::TypeRepresentation::TimestampTZ,
205205
}
206206
}
207207

0 commit comments

Comments
 (0)