From 28bd7404ad47654aaf2afa0749fae63de354377f Mon Sep 17 00:00:00 2001
From: dennis zhuang <killme2008@gmail.com>
Date: Fri, 16 Dec 2022 11:17:01 +0800
Subject: [PATCH] feat: change column's default property to nullable (#751)

* feat: change column's default property to nullable

* chore: use all instead of any

* fix: compile error

* fix: dependencies order in cargo
---
 src/frontend/src/table.rs |  7 ++---
 src/sql/src/statements.rs | 56 ++++++++++++++++++++++++++++++++++++---
 src/storage/Cargo.toml    |  2 +-
 src/table/Cargo.toml      |  2 +-
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/src/frontend/src/table.rs b/src/frontend/src/table.rs
index 2d157f30a8..12bbbdd49d 100644
--- a/src/frontend/src/table.rs
+++ b/src/frontend/src/table.rs
@@ -354,7 +354,7 @@ impl DistTable {
         Ok(partition_rule)
     }
 
-    /// Define a `alter_by_expr` instead of impl [`Table::alter`] to avoid redundant conversion between  
+    /// Define a `alter_by_expr` instead of impl [`Table::alter`] to avoid redundant conversion between
     /// [`table::requests::AlterTableRequest`] and [`AlterExpr`].
     pub(crate) async fn alter_by_expr(&self, expr: AlterExpr) -> Result<()> {
         let table_routes = self.table_routes.get_route(&self.table_name).await?;
@@ -735,6 +735,7 @@ mod test {
 
     #[tokio::test(flavor = "multi_thread")]
     async fn test_dist_table_scan() {
+        common_telemetry::init_default_ut_logging();
         let table = Arc::new(new_dist_table().await);
         // should scan all regions
         // select a, row_id from numbers
@@ -896,8 +897,8 @@ mod test {
     async fn new_dist_table() -> DistTable {
         let column_schemas = vec![
             ColumnSchema::new("ts", ConcreteDataType::int64_datatype(), false),
-            ColumnSchema::new("a", ConcreteDataType::int32_datatype(), false),
-            ColumnSchema::new("row_id", ConcreteDataType::int32_datatype(), false),
+            ColumnSchema::new("a", ConcreteDataType::int32_datatype(), true),
+            ColumnSchema::new("row_id", ConcreteDataType::int32_datatype(), true),
         ];
         let schema = Arc::new(Schema::new(column_schemas.clone()));
 
diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs
index ba8397ca01..6dcc5fd220 100644
--- a/src/sql/src/statements.rs
+++ b/src/sql/src/statements.rs
@@ -245,7 +245,8 @@ pub fn column_def_to_schema(column_def: &ColumnDef, is_time_index: bool) -> Resu
     let is_nullable = column_def
         .options
         .iter()
-        .any(|o| matches!(o.option, ColumnOption::Null));
+        .all(|o| !matches!(o.option, ColumnOption::NotNull))
+        && !is_time_index;
 
     let name = column_def.name.value.clone();
     let data_type = sql_data_type_to_concrete_data_type(&column_def.data_type)?;
@@ -265,10 +266,10 @@ pub fn sql_column_def_to_grpc_column_def(col: ColumnDef) -> Result<api::v1::Colu
     let name = col.name.value.clone();
     let data_type = sql_data_type_to_concrete_data_type(&col.data_type)?;
 
-    let nullable = !col
+    let is_nullable = col
         .options
         .iter()
-        .any(|o| matches!(o.option, ColumnOption::NotNull));
+        .all(|o| !matches!(o.option, ColumnOption::NotNull));
 
     let default_constraint = parse_column_default_constraint(&name, &data_type, &col.options)?
         .map(ColumnDefaultConstraint::try_into) // serialize default constraint to bytes
@@ -281,7 +282,7 @@ pub fn sql_column_def_to_grpc_column_def(col: ColumnDef) -> Result<api::v1::Colu
     Ok(api::v1::ColumnDef {
         name,
         datatype: data_type,
-        is_nullable: nullable,
+        is_nullable,
         default_constraint,
     })
 }
@@ -603,4 +604,51 @@ mod tests {
         let grpc_column_def = sql_column_def_to_grpc_column_def(column_def).unwrap();
         assert!(!grpc_column_def.is_nullable);
     }
+
+    #[test]
+    pub fn test_column_def_to_schema() {
+        let column_def = ColumnDef {
+            name: "col".into(),
+            data_type: SqlDataType::Double,
+            collation: None,
+            options: vec![],
+        };
+
+        let column_schema = column_def_to_schema(&column_def, false).unwrap();
+
+        assert_eq!("col", column_schema.name);
+        assert_eq!(
+            ConcreteDataType::float64_datatype(),
+            column_schema.data_type
+        );
+        assert!(column_schema.is_nullable());
+        assert!(!column_schema.is_time_index());
+
+        let column_schema = column_def_to_schema(&column_def, true).unwrap();
+
+        assert_eq!("col", column_schema.name);
+        assert_eq!(
+            ConcreteDataType::float64_datatype(),
+            column_schema.data_type
+        );
+        assert!(!column_schema.is_nullable());
+        assert!(column_schema.is_time_index());
+
+        let column_def = ColumnDef {
+            name: "col2".into(),
+            data_type: SqlDataType::String,
+            collation: None,
+            options: vec![ColumnOptionDef {
+                name: None,
+                option: ColumnOption::NotNull,
+            }],
+        };
+
+        let column_schema = column_def_to_schema(&column_def, false).unwrap();
+
+        assert_eq!("col2", column_schema.name);
+        assert_eq!(ConcreteDataType::string_datatype(), column_schema.data_type);
+        assert!(!column_schema.is_nullable());
+        assert!(!column_schema.is_time_index());
+    }
 }
diff --git a/src/storage/Cargo.toml b/src/storage/Cargo.toml
index 9c107d3e64..32c220cbab 100644
--- a/src/storage/Cargo.toml
+++ b/src/storage/Cargo.toml
@@ -21,8 +21,8 @@ futures = "0.3"
 futures-util = "0.3"
 lazy_static = "1.4"
 object-store = { path = "../object-store" }
-paste = "1.0"
 parquet = { version = "26", features = ["async"] }
+paste = "1.0"
 planus = "0.2"
 prost = "0.11"
 regex = "1.5"
diff --git a/src/table/Cargo.toml b/src/table/Cargo.toml
index 59d9a901bd..9adc0e625f 100644
--- a/src/table/Cargo.toml
+++ b/src/table/Cargo.toml
@@ -27,6 +27,6 @@ tokio = { version = "1.18", features = ["full"] }
 
 [dev-dependencies]
 datafusion-expr = "14.0.0"
+parquet = { version = "26", features = ["async"] }
 tempdir = "0.3"
 tokio-util = { version = "0.7", features = ["compat"] }
-parquet = { version = "26", features = ["async"] }