Merge pull request #7699 from sundy-li/user-settings

mergify[bot] · web-flow · commit 35c44d1f7601 · 2022-09-19T02:13:03.000Z
chore(query): make user setting depend on a lightweight UserSettingValue
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/src/meta/types/Cargo.toml b/src/meta/types/Cargo.toml
@@ -11,14 +11,14 @@ doctest = false
 test = false
 
 [dependencies]
-common-datavalues = { path = "../../query/datavalues" }
 common-exception = { path = "../../common/exception" }
 common-storage = { path = "../../common/storage" }
 
 openraft = { git = "https://github.com/datafuselabs/openraft", tag = "v0.7.2" }
 sled = { git = "https://github.com/datafuse-extras/sled", tag = "v0.34.7-datafuse.1", default-features = false }
 
 anyerror = "=0.1.7"
+chrono = "0.4.20"
 derive_more = "0.99.17"
 enumflags2 = { version = "0.7.5", features = ["serde"] }
 hex = "0.4.3"
diff --git a/src/meta/types/src/lib.rs b/src/meta/types/src/lib.rs
@@ -196,5 +196,6 @@ pub use user_privilege::UserPrivilegeSet;
 pub use user_privilege::UserPrivilegeType;
 pub use user_quota::UserQuota;
 pub use user_setting::UserSetting;
+pub use user_setting::UserSettingValue;
 pub use user_stage::*;
 pub use with::With;
diff --git a/src/meta/types/src/user_setting.rs b/src/meta/types/src/user_setting.rs
@@ -12,33 +12,89 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use common_datavalues::DataValue;
+use core::fmt;
+
 use common_exception::ErrorCode;
 use common_exception::Result;
 use serde::Deserialize;
+use serde::Deserializer;
 use serde::Serialize;
+use serde::Serializer;
 
 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
-#[serde(default)]
 pub struct UserSetting {
     // The name of the setting.
     pub name: String,
     // The value of the setting.
-    pub value: DataValue,
+    pub value: UserSettingValue,
 }
-impl UserSetting {
-    pub fn create(name: &str, value: DataValue) -> UserSetting {
-        UserSetting {
-            name: name.to_string(),
-            value,
+
+#[derive(Serialize, Deserialize, Clone, PartialEq, Eq)]
+pub enum UserSettingValue {
+    UInt64(u64),
+
+    // TO BE COMPATIBLE WITH old version: `String<Vec<u8>>`
+    #[serde(deserialize_with = "deser_str_from_vu8")]
+    #[serde(serialize_with = "str_vu8")]
+    String(String),
+}
+
+fn deser_str_from_vu8<'de, D>(deserializer: D) -> std::result::Result<String, D::Error>
+where D: Deserializer<'de> {
+    let s: Vec<u8> = Deserialize::deserialize(deserializer)?;
+    Ok(String::from_utf8(s).unwrap())
+}
+
+fn str_vu8<S>(data: &String, s: S) -> std::result::Result<S::Ok, S::Error>
+where S: Serializer {
+    s.serialize_bytes(data.as_bytes())
+}
+
+impl UserSettingValue {
+    pub fn as_u64(&self) -> Result<u64> {
+        match self {
+            UserSettingValue::UInt64(v) => Ok(*v),
+            other => Result::Err(ErrorCode::BadDataValueType(format!(
+                "Unexpected type:{:?} to get u64 number",
+                other
+            ))),
+        }
+    }
+
+    pub fn as_string(&self) -> Result<String> {
+        match self {
+            UserSettingValue::String(v) => Ok(v.to_owned()),
+            other => Result::Err(ErrorCode::BadDataValueType(format!(
+                "Unexpected type:{:?} to get u64 number",
+                other
+            ))),
+        }
+    }
+}
+
+impl fmt::Display for UserSettingValue {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            UserSettingValue::UInt64(v) => write!(f, "{}", v),
+            UserSettingValue::String(v) => write!(f, "{}", v),
         }
     }
 }
-impl Default for UserSetting {
-    fn default() -> Self {
+
+impl fmt::Debug for UserSettingValue {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            UserSettingValue::UInt64(v) => write!(f, "{}", v),
+            UserSettingValue::String(_) => write!(f, "{}", self),
+        }
+    }
+}
+
+impl UserSetting {
+    pub fn create(name: &str, value: UserSettingValue) -> UserSetting {
         UserSetting {
-            name: "".to_string(),
-            value: DataValue::Null,
+            name: name.to_string(),
+            value,
         }
     }
 }
diff --git a/src/meta/types/src/user_stage.rs b/src/meta/types/src/user_stage.rs
@@ -15,8 +15,8 @@
 use std::fmt;
 use std::str::FromStr;
 
-use common_datavalues::chrono::DateTime;
-use common_datavalues::chrono::Utc;
+use chrono::DateTime;
+use chrono::Utc;
 use common_storage::StorageParams;
 
 use crate::UserIdentity;
diff --git a/src/query/management/tests/it/setting.rs b/src/query/management/tests/it/setting.rs
@@ -15,20 +15,20 @@
 use std::sync::Arc;
 
 use common_base::base::tokio;
-use common_datavalues::DataValue;
 use common_exception::Result;
 use common_management::*;
 use common_meta_api::KVApi;
 use common_meta_embedded::MetaEmbedded;
 use common_meta_types::SeqV;
 use common_meta_types::UserSetting;
+use common_meta_types::UserSettingValue;
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
 async fn test_set_setting() -> Result<()> {
     let (kv_api, mgr) = new_setting_api().await?;
 
     {
-        let setting = UserSetting::create("max_threads", DataValue::UInt64(3));
+        let setting = UserSetting::create("max_threads", UserSettingValue::UInt64(3));
         mgr.set_setting(setting.clone()).await?;
         let value = kv_api
             .get_kv("__fd_settings/databend_query/max_threads")
@@ -48,7 +48,7 @@ async fn test_set_setting() -> Result<()> {
 
     // Set again.
     {
-        let setting = UserSetting::create("max_threads", DataValue::UInt64(1));
+        let setting = UserSetting::create("max_threads", UserSettingValue::UInt64(1));
         mgr.set_setting(setting.clone()).await?;
         let value = kv_api
             .get_kv("__fd_settings/databend_query/max_threads")
@@ -68,14 +68,17 @@ async fn test_set_setting() -> Result<()> {
 
     // Get settings.
     {
-        let expect = vec![UserSetting::create("max_threads", DataValue::UInt64(1))];
+        let expect = vec![UserSetting::create(
+            "max_threads",
+            UserSettingValue::UInt64(1),
+        )];
         let actual = mgr.get_settings().await?;
         assert_eq!(actual, expect);
     }
 
     // Get setting.
     {
-        let expect = UserSetting::create("max_threads", DataValue::UInt64(1));
+        let expect = UserSetting::create("max_threads", UserSettingValue::UInt64(1));
         let actual = mgr.get_setting("max_threads", None).await?;
         assert_eq!(actual.data, expect);
     }
diff --git a/src/query/service/src/context_function.rs b/src/query/service/src/context_function.rs
@@ -53,7 +53,7 @@ impl ContextFunction {
                 ctx.get_connection_id().into_bytes(),
             ))],
             "timezone" => vec![Expression::create_literal(DataValue::String(
-                ctx.get_settings().get_timezone()?,
+                ctx.get_settings().get_timezone()?.into_bytes(),
             ))],
             _ => vec![],
         })
diff --git a/src/query/service/src/interpreters/interpreter_query_log.rs b/src/query/service/src/interpreters/interpreter_query_log.rs
@@ -305,7 +305,8 @@ impl InterpreterQueryLog {
             .get_settings()
             .get_setting_values_short()
         {
-            write!(session_settings, "{}={}, ", key, value).expect("write to string must succeed");
+            write!(session_settings, "{}={:?}, ", key, value)
+                .expect("write to string must succeed");
         }
         session_settings.push_str("scope: SESSION");
 
@@ -418,7 +419,8 @@ impl InterpreterQueryLog {
             .get_settings()
             .get_setting_values_short()
         {
-            write!(session_settings, "{}={}, ", key, value).expect("write to string must succeed");
+            write!(session_settings, "{}={:?}, ", key, value)
+                .expect("write to string must succeed");
         }
         session_settings.push_str("scope: SESSION");
 
diff --git a/src/query/service/src/sessions/query_ctx.rs b/src/query/service/src/sessions/query_ctx.rs
@@ -351,9 +351,7 @@ impl TableContext for QueryContext {
         swaped_precommit_blocks
     }
     fn try_get_function_context(&self) -> Result<FunctionContext> {
-        let tz = String::from_utf8(self.get_settings().get_timezone()?).map_err(|_| {
-            ErrorCode::LogicalError("Timezone has been checked and should be valid.")
-        })?;
+        let tz = self.get_settings().get_timezone()?;
         let tz = tz.parse::<Tz>().map_err(|_| {
             ErrorCode::InvalidTimezone("Timezone has been checked and should be valid")
         })?;
diff --git a/src/query/service/src/sessions/session.rs b/src/query/service/src/sessions/session.rs
@@ -137,22 +137,19 @@ impl Session {
     pub fn get_format_settings(&self) -> Result<FormatSettings> {
         let settings = &self.session_ctx.get_settings();
         let mut format = FormatSettings {
-            record_delimiter: settings.get_record_delimiter()?,
-            field_delimiter: settings.get_field_delimiter()?,
+            record_delimiter: settings.get_record_delimiter()?.into_bytes(),
+            field_delimiter: settings.get_field_delimiter()?.into_bytes(),
             empty_as_default: settings.get_empty_as_default()? > 0,
             skip_header: settings.get_skip_header()?,
             ..Default::default()
         };
 
-        let tz = String::from_utf8(settings.get_timezone()?).map_err(|_| {
-            ErrorCode::LogicalError("Timezone has been checked and should be valid.")
-        })?;
+        let tz = settings.get_timezone()?;
         format.timezone = tz.parse::<Tz>().map_err(|_| {
             ErrorCode::InvalidTimezone("Timezone has been checked and should be valid")
         })?;
 
-        let compress = String::from_utf8(settings.get_compression()?)
-            .map_err(|_| ErrorCode::UnknownCompressionType("Compress type must be valid utf-8"))?;
+        let compress = settings.get_compression()?;
         format.compression = compress.parse()?;
         format.ident_case_sensitive = settings.get_unquoted_ident_case_sensitive()?;
         Ok(format)
diff --git a/src/query/service/src/sql/planner/semantic/type_check.rs b/src/query/service/src/sql/planner/semantic/type_check.rs
@@ -1425,8 +1425,6 @@ impl<'a> TypeChecker<'a> {
             ),
             ("timezone", &[]) => {
                 let tz = self.ctx.get_settings().get_timezone().unwrap();
-                // No need to handle err, the tz in settings is valid.
-                let tz = String::from_utf8(tz).unwrap();
                 Some(
                     self.resolve(
                         &Expr::Literal {
diff --git a/src/query/settings/Cargo.toml b/src/query/settings/Cargo.toml
@@ -11,7 +11,6 @@ test = false
 [dependencies]
 common-ast = { path = "../../query/ast" }
 common-config = { path = "../config" }
-common-datavalues = { path = "../datavalues" }
 common-exception = { path = "../../common/exception" }
 common-meta-types = { path = "../../meta/types" }
 common-users = { path = "../users" }
diff --git a/src/query/settings/src/lib.rs b/src/query/settings/src/lib.rs
diff --git a/src/query/storages/preludes/src/system/settings_table.rs b/src/query/storages/preludes/src/system/settings_table.rs
diff --git a/tests/fuse-compat/compat-logictest/fuse_compat_write b/tests/fuse-compat/compat-logictest/fuse_compat_write

Original file line number	Diff line number	Diff line change
`@@ -305,7 +305,8 @@ impl InterpreterQueryLog {`
`305`	`305`	`.get_settings()`
`306`	`306`	`.get_setting_values_short()`
`307`	`307`	`{`
`308`		`- write!(session_settings, "{}={}, ", key, value).expect("write to string must succeed");`
	`308`	`+ write!(session_settings, "{}={:?}, ", key, value)`
	`309`	`+ .expect("write to string must succeed");`
`309`	`310`	`}`
`310`	`311`	`session_settings.push_str("scope: SESSION");`
`311`	`312`
`@@ -418,7 +419,8 @@ impl InterpreterQueryLog {`
`418`	`419`	`.get_settings()`
`419`	`420`	`.get_setting_values_short()`
`420`	`421`	`{`
`421`		`- write!(session_settings, "{}={}, ", key, value).expect("write to string must succeed");`
	`422`	`+ write!(session_settings, "{}={:?}, ", key, value)`
	`423`	`+ .expect("write to string must succeed");`
`422`	`424`	`}`
`423`	`425`	`session_settings.push_str("scope: SESSION");`
`424`	`426`