From 1d2157e6be9f30696109e6fb5cdbd38b2fb580c1 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Fri, 10 Mar 2023 18:30:50 +0800 Subject: [PATCH 1/2] add type pairs macro Signed-off-by: Bugen Zhao --- src/common/src/types/mod.rs | 66 ++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/src/common/src/types/mod.rs b/src/common/src/types/mod.rs index b84d54e5bc9b..6e0690c2fd24 100644 --- a/src/common/src/types/mod.rs +++ b/src/common/src/types/mod.rs @@ -1151,30 +1151,52 @@ impl ScalarImpl { } } +/// `for_all_type_pairs` is a macro that records all logical type (`DataType`) variants and their +/// corresponding physical type (`ScalarImpl`, `ArrayImpl`, or `ArrayBuilderImpl`) variants. +/// +/// This is useful for checking whether a physical type is compatible with a logical type. +#[macro_export] +macro_rules! for_all_type_pairs { + ($macro:ident, $($x:tt),*) => { + $macro! { [$($x),*], + { Boolean, Bool }, + { Int16, Int16 }, + { Int32, Int32 }, + { Int64, Int64 }, + { Float32, Float32 }, + { Float64, Float64 }, + { Varchar, Utf8 }, + { Bytea, Bytea }, + { Date, NaiveDate }, + { Time, NaiveTime }, + { Timestamp, NaiveDateTime }, + { Timestamptz, Int64 }, + { Interval, Interval }, + { Decimal, Decimal }, + { Jsonb, Jsonb }, + { List, List }, + { Struct, Struct } + } + }; +} + +/// Check whether the `target` with `type` matches the `data_type`. +#[macro_export] +macro_rules! check_matches { + ([$data_type:expr, $target:expr, $type:ident], $( { $DataType:ident, $PhysicalType:ident }),*) => { + match ($data_type, $target) { + $( + ($crate::types::DataType::$DataType { .. }, $type::$PhysicalType(_)) => true, + ($crate::types::DataType::$DataType { .. }, _) => false, // so that we won't forget to match a new logical type + )* + } + }; +} + +/// Returns whether the `literal` matches the `data_type`. pub fn literal_type_match(data_type: &DataType, literal: Option<&ScalarImpl>) -> bool { match literal { - Some(datum) => { - matches!( - (data_type, datum), - (DataType::Boolean, ScalarImpl::Bool(_)) - | (DataType::Int16, ScalarImpl::Int16(_)) - | (DataType::Int32, ScalarImpl::Int32(_)) - | (DataType::Int64, ScalarImpl::Int64(_)) - | (DataType::Float32, ScalarImpl::Float32(_)) - | (DataType::Float64, ScalarImpl::Float64(_)) - | (DataType::Varchar, ScalarImpl::Utf8(_)) - | (DataType::Bytea, ScalarImpl::Bytea(_)) - | (DataType::Date, ScalarImpl::NaiveDate(_)) - | (DataType::Time, ScalarImpl::NaiveTime(_)) - | (DataType::Timestamp, ScalarImpl::NaiveDateTime(_)) - | (DataType::Timestamptz, ScalarImpl::Int64(_)) - | (DataType::Decimal, ScalarImpl::Decimal(_)) - | (DataType::Interval, ScalarImpl::Interval(_)) - | (DataType::Jsonb, ScalarImpl::Jsonb(_)) - | (DataType::Struct { .. }, ScalarImpl::Struct(_)) - | (DataType::List { .. }, ScalarImpl::List(_)) - ) - } + Some(datum) => for_all_type_pairs! { check_matches, data_type, datum, ScalarImpl }, None => true, } } From d98bf66c6a6727587120370b793de4c64f2caa5f Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Fri, 10 Mar 2023 19:05:40 +0800 Subject: [PATCH 2/2] refactor schema check Signed-off-by: Bugen Zhao --- src/common/src/types/mod.rs | 31 +++++++++--------- src/common/src/util/schema_check.rs | 49 +++++++++++++---------------- 2 files changed, 36 insertions(+), 44 deletions(-) diff --git a/src/common/src/types/mod.rs b/src/common/src/types/mod.rs index 6e0690c2fd24..ff44b5b9c419 100644 --- a/src/common/src/types/mod.rs +++ b/src/common/src/types/mod.rs @@ -1157,8 +1157,8 @@ impl ScalarImpl { /// This is useful for checking whether a physical type is compatible with a logical type. #[macro_export] macro_rules! for_all_type_pairs { - ($macro:ident, $($x:tt),*) => { - $macro! { [$($x),*], + ($macro:ident) => { + $macro! { { Boolean, Bool }, { Int16, Int16 }, { Int32, Int32 }, @@ -1180,23 +1180,22 @@ macro_rules! for_all_type_pairs { }; } -/// Check whether the `target` with `type` matches the `data_type`. -#[macro_export] -macro_rules! check_matches { - ([$data_type:expr, $target:expr, $type:ident], $( { $DataType:ident, $PhysicalType:ident }),*) => { - match ($data_type, $target) { - $( - ($crate::types::DataType::$DataType { .. }, $type::$PhysicalType(_)) => true, - ($crate::types::DataType::$DataType { .. }, _) => false, // so that we won't forget to match a new logical type - )* - } - }; -} - /// Returns whether the `literal` matches the `data_type`. pub fn literal_type_match(data_type: &DataType, literal: Option<&ScalarImpl>) -> bool { match literal { - Some(datum) => for_all_type_pairs! { check_matches, data_type, datum, ScalarImpl }, + Some(scalar) => { + macro_rules! matches { + ($( { $DataType:ident, $PhysicalType:ident }),*) => { + match (data_type, scalar) { + $( + (DataType::$DataType { .. }, ScalarImpl::$PhysicalType(_)) => true, + (DataType::$DataType { .. }, _) => false, // so that we won't forget to match a new logical type + )* + } + } + } + for_all_type_pairs! { matches } + } None => true, } } diff --git a/src/common/src/util/schema_check.rs b/src/common/src/util/schema_check.rs index 981d7370790d..21791f9a96b6 100644 --- a/src/common/src/util/schema_check.rs +++ b/src/common/src/util/schema_check.rs @@ -15,43 +15,36 @@ use itertools::Itertools; use crate::array::column::Column; +use crate::array::ArrayImpl; +use crate::for_all_type_pairs; use crate::types::DataType; /// Check if the schema of `columns` matches the expected `data_types`. Used for debugging. pub fn schema_check<'a, T, C>(data_types: T, columns: C) -> Result<(), String> where - T: IntoIterator + Clone, - C: IntoIterator + Clone, + T: IntoIterator, + C: IntoIterator, { - tracing::event!( - tracing::Level::TRACE, - "input schema = \n{:#?}\nexpected schema = \n{:#?}", - columns - .clone() - .into_iter() - .map(|col| col.array_ref().get_ident()) - .collect_vec(), - data_types.clone().into_iter().collect_vec(), - ); - - for (i, pair) in columns.into_iter().zip_longest(data_types).enumerate() { - let array = pair.as_ref().left().map(|c| c.array_ref()); - let builder = pair.as_ref().right().map(|d| d.create_array_builder(0)); // TODO: check `data_type` directly - - macro_rules! check_schema { - ($( { $variant_name:ident, $suffix_name:ident, $array:ty, $builder:ty } ),*) => { - use crate::array::ArrayBuilderImpl; - use crate::array::ArrayImpl; - - match (array, &builder) { - $( (Some(ArrayImpl::$variant_name(_)), Some(ArrayBuilderImpl::$variant_name(_))) => {} ),* - _ => return Err(format!("column {} should be {:?}, while stream chunk gives {:?}", - i, builder.map(|b| b.get_ident()), array.map(|a| a.get_ident()))), + for (i, pair) in data_types + .into_iter() + .zip_longest(columns.into_iter().map(|c| c.array_ref())) + .enumerate() + { + macro_rules! matches { + ($( { $DataType:ident, $PhysicalType:ident }),*) => { + match (pair.as_ref().left(), pair.as_ref().right()) { + $( (Some(DataType::$DataType { .. }), Some(ArrayImpl::$PhysicalType(_))) => continue, )* + (data_type, array) => { + let array_ident = array.map(|a| a.get_ident()); + return Err(format!( + "column type mismatched at position {i}: expected {data_type:?}, found {array_ident:?}" + )); + } } - }; + } } - for_all_variants! { check_schema }; + for_all_type_pairs! { matches } } Ok(())