chore: merge settings possible_values to range

src/query/service/tests/it/storages/testdata/columns_table.txt

@@ -279,6 +279,7 @@ DB.Table: 'system'.'columns', Table: columns-table_id:1, ver:0, Engine: SystemCo
 | 'query_kind'                      | 'system'             | 'query_log'           | 'String'              | 'VARCHAR'           | ''       | ''       | 'NO'     | ''       |
 | 'query_start_time'                | 'system'             | 'query_log'           | 'Timestamp'           | 'TIMESTAMP'         | ''       | ''       | 'NO'     | ''       |
 | 'query_text'                      | 'system'             | 'query_log'           | 'String'              | 'VARCHAR'           | ''       | ''       | 'NO'     | ''       |
+| 'range'                           | 'system'             | 'settings'            | 'String'              | 'VARCHAR'           | ''       | ''       | 'NO'     | ''       |
 | 'referenced_column_name'          | 'information_schema' | 'key_column_usage'    | 'NULL'                | 'NULL'              | ''       | ''       | 'NO'     | ''       |
 | 'referenced_table_name'           | 'information_schema' | 'key_column_usage'    | 'NULL'                | 'NULL'              | ''       | ''       | 'NO'     | ''       |
 | 'referenced_table_schema'         | 'information_schema' | 'key_column_usage'    | 'NULL'                | 'NULL'              | ''       | ''       | 'NO'     | ''       |

src/query/settings/Cargo.toml

@@ -24,3 +24,6 @@ log = { workspace = true }
 num_cpus = "1.13.1"
 once_cell = { workspace = true }
 sys-info = "0.9"
+
+[dev-dependencies]
+tokio = { workspace = true }

src/query/settings/src/lib.rs

@@ -22,4 +22,5 @@ pub use settings::ScopeLevel;
 pub use settings::Settings;
 pub use settings_default::ReplaceIntoShuffleStrategy;
 pub use settings_default::SettingMode;
+pub use settings_default::SettingRange;
 pub use settings_getter_setter::FlightCompression;

src/query/settings/src/settings.rs

@@ -25,6 +25,7 @@ use itertools::Itertools;
 
 use crate::settings_default::DefaultSettingValue;
 use crate::settings_default::DefaultSettings;
+use crate::settings_default::SettingRange;
 use crate::SettingMode;
 
 #[derive(serde::Serialize, serde::Deserialize, Clone)]
@@ -115,7 +116,7 @@ pub struct SettingsItem {
     pub desc: &'static str,
     pub user_value: UserSettingValue,
     pub default_value: UserSettingValue,
-    pub possible_values: Option<Vec<&'static str>>,
+    pub range: Option<SettingRange>,
 }
 
 pub struct SettingsIter<'a> {
@@ -156,15 +157,15 @@ impl<'a> Iterator for SettingsIter<'a> {
                         desc: default_value.desc,
                         user_value: default_value.value.clone(),
                         default_value: default_value.value,
-                        possible_values: default_value.possible_values,
+                        range: default_value.range,
                     },
                     Some(change_value) => SettingsItem {
                         name: key,
                         level: change_value.level.clone(),
                         desc: default_value.desc,
                         user_value: change_value.value.clone(),
                         default_value: default_value.value,
-                        possible_values: default_value.possible_values,
+                        range: default_value.range,
                     },
                 }),
             };

src/query/settings/src/settings_getter_setter.rs

@@ -64,36 +64,40 @@ impl Settings {
         unsafe { self.unchecked_try_set_u64(key, val) }
     }
 
+    /// Sets a u64 value for a given key in the settings.
+    /// Ensures that the key exists, the setting type is UInt64, and the value is within any defined numeric range.
     unsafe fn unchecked_try_set_u64(&self, key: &str, val: u64) -> Result<()> {
-        match DefaultSettings::instance()?.settings.get(key) {
-            None => Err(ErrorCode::UnknownVariable(format!(
-                "Unknown variable: {:?}",
-                key
-            ))),
-            Some(default_val) => {
-                if !matches!(&default_val.value, UserSettingValue::UInt64(_)) {
-                    return Err(ErrorCode::BadArguments(format!(
-                        "Set a integer({}) into {:?}.",
-                        val, key
-                    )));
-                }
-
-                if let Some(range) = &default_val.range {
-                    if !range.contains(&val) {
-                        return Err(ErrorCode::BadArguments(format!(
-                            "{} value should in range: {:?}",
-                            key, range
-                        )));
-                    }
+        // Retrieve the instance of default settings
+        let default_settings = DefaultSettings::instance()?;
+
+        let setting_value = default_settings
+            .settings
+            .get(key)
+            .ok_or_else(|| ErrorCode::UnknownVariable(format!("Unknown variable: {:?}", key)))?;
+
+        match &setting_value.value {
+            UserSettingValue::UInt64(_) => {
+                // If a numeric range is defined, validate the value against this range
+                if let Some(range) = &setting_value.range {
+                    // Check if the value falls within the numeric range
+                    range.is_within_numeric_range(val).map_err(|err| {
+                        ErrorCode::WrongValueForVariable(format!("{}: {}", key, err.message()))
+                    })?;
                 }
 
+                // Insert the value into changes with a session scope
                 self.changes.insert(key.to_string(), ChangeValue {
                     level: ScopeLevel::Session,
                     value: UserSettingValue::UInt64(val),
                 });
 
                 Ok(())
             }
+            // If the setting type is not UInt64, return an error
+            _ => Err(ErrorCode::BadArguments(format!(
+                "Set an integer ({}) into {:?}",
+                val, key
+            ))),
         }
     }
 
@@ -103,20 +107,13 @@ impl Settings {
         unsafe { self.unchecked_set_setting(k, v) }
     }
 
-    unsafe fn unchecked_set_setting(&self, k: String, v: String) -> Result<(), ErrorCode> {
-        if let (key, Some(value)) = DefaultSettings::convert_value(k.clone(), v)? {
-            self.changes.insert(key, ChangeValue {
-                value,
-                level: ScopeLevel::Session,
-            });
-
-            return Ok(());
-        }
-
-        Err(ErrorCode::UnknownVariable(format!(
-            "Unknown variable: {:?}",
-            k
-        )))
+    unsafe fn unchecked_set_setting(&self, k: String, v: String) -> Result<()> {
+        let (key, value) = DefaultSettings::convert_value(k.clone(), v)?;
+        self.changes.insert(key, ChangeValue {
+            value,
+            level: ScopeLevel::Session,
+        });
+        Ok(())
     }
 
     pub fn get_enable_clickhouse_handler(&self) -> Result<bool> {

src/query/settings/src/settings_global.rs

@@ -58,23 +58,16 @@ impl Settings {
 
     #[async_backtrace::framed]
     pub async fn set_global_setting(&self, k: String, v: String) -> Result<()> {
-        if let (key, Some(value)) = DefaultSettings::convert_value(k.clone(), v)? {
-            self.changes.insert(key.clone(), ChangeValue {
-                value: value.clone(),
-                level: ScopeLevel::Global,
-            });
+        let (key, value) = DefaultSettings::convert_value(k.clone(), v)?;
+        self.changes.insert(key.clone(), ChangeValue {
+            value: value.clone(),
+            level: ScopeLevel::Global,
+        });
 
-            UserApiProvider::instance()
-                .set_setting(&self.tenant, UserSetting { name: key, value })
-                .await?;
-
-            return Ok(());
-        }
-
-        Err(ErrorCode::UnknownVariable(format!(
-            "Unknown variable: {:?}",
-            k
-        )))
+        UserApiProvider::instance()
+            .set_setting(&self.tenant, UserSetting { name: key, value })
+            .await?;
+        Ok(())
     }
 
     #[async_backtrace::framed]
@@ -95,29 +88,16 @@ impl Settings {
                         warn!("Ignore deprecated global setting {} = {}", name, val);
                         continue;
                     }
-                    Some(default_setting_value) => {
-                        if !default_setting_value
-                            .possible_values
-                            .as_ref()
-                            .map(|values| values.iter().any(|v| v.eq_ignore_ascii_case(&val)))
-                            .unwrap_or(true)
-                        {
-                            // the settings may be deprecated
-                            warn!("Ignore invalid global setting {} = {}", name, val);
-                            continue;
-                        }
-
-                        match &default_setting_value.value {
-                            UserSettingValue::UInt64(_) => ChangeValue {
-                                level: ScopeLevel::Global,
-                                value: UserSettingValue::UInt64(val.parse::<u64>()?),
-                            },
-                            UserSettingValue::String(_) => ChangeValue {
-                                level: ScopeLevel::Global,
-                                value: UserSettingValue::String(val.clone()),
-                            },
-                        }
-                    }
+                    Some(default_setting_value) => match &default_setting_value.value {
+                        UserSettingValue::UInt64(_) => ChangeValue {
+                            level: ScopeLevel::Global,
+                            value: UserSettingValue::UInt64(val.parse::<u64>()?),
+                        },
+                        UserSettingValue::String(_) => ChangeValue {
+                            level: ScopeLevel::Global,
+                            value: UserSettingValue::String(val.clone()),
+                        },
+                    },
                 });
         }
 

src/query/settings/tests/it/main.rs

@@ -0,0 +1,14 @@
+// Copyright 2021 Datafuse Labs.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+mod setting;

src/query/settings/tests/it/setting.rs

@@ -0,0 +1,100 @@
+// Copyright 2022 Datafuse Labs.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use common_settings::Settings;
+
+#[test]
+fn test_set_settings() {
+    let settings = Settings::create("test".to_string());
+    // Number range.
+    {
+        settings.set_max_threads(2).unwrap();
+
+        let result = settings.set_max_threads(1025);
+        let expect = "WrongValueForVariable. Code: 2803, Text = max_threads: Value 1025 is not within the range [1, 1024].";
+        assert_eq!(expect, format!("{}", result.unwrap_err()));
+    }
+
+    // Number range.
+    {
+        // Ok
+        settings
+            .set_setting("enable_table_lock".to_string(), "1".to_string())
+            .unwrap();
+        // Ok
+        settings
+            .set_setting("enable_table_lock".to_string(), "0".to_string())
+            .unwrap();
+
+        // Ok with float.
+        settings
+            .set_setting("max_memory_usage".to_string(), "1610612736.0".to_string())
+            .unwrap();
+
+        // Ok with float.
+        settings
+            .set_setting("enable_table_lock".to_string(), "1.0".to_string())
+            .unwrap();
+
+        // Error
+        let result = settings.set_setting("enable_table_lock".to_string(), "3".to_string());
+        let expect =
+            "WrongValueForVariable. Code: 2803, Text = Value 3 is not within the range [0, 1].";
+        assert_eq!(expect, format!("{}", result.unwrap_err()));
+
+        // Error
+        let result = settings.set_setting("enable_table_lock".to_string(), "xx".to_string());
+        let expect = "WrongValueForVariable. Code: 2803, Text = xx is not a valid integer value.";
+        assert_eq!(expect, format!("{}", result.unwrap_err()));
+    }
+
+    // String out of range.
+    {
+        // Ok
+        settings
+            .set_setting("query_flight_compression".to_string(), "LZ4".to_string())
+            .unwrap();
+
+        // Ok
+        settings
+            .set_setting("query_flight_compression".to_string(), "lz4".to_string())
+            .unwrap();
+
+        // Error
+        let result = settings.set_setting("query_flight_compression".to_string(), "xx".to_string());
+        let expect = "WrongValueForVariable. Code: 2803, Text = Value xx is not within the allowed values [\"None\", \"LZ4\", \"ZSTD\"].";
+        assert_eq!(expect, format!("{}", result.unwrap_err()));
+    }
+
+    // String without range.
+    {
+        // Ok
+        settings
+            .set_setting("sandbox_tenant".to_string(), "xx".to_string())
+            .unwrap();
+    }
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+async fn test_set_global_settings() {
+    let settings = Settings::create("test".to_string());
+    let result = settings
+        .set_global_setting(
+            "query_flight_compression_notfound".to_string(),
+            "xx".to_string(),
+        )
+        .await;
+    let expect = "UnknownVariable. Code: 2801, Text = Unknown variable: \"query_flight_compression_notfound\".";
+    assert_eq!(expect, format!("{}", result.unwrap_err()));
+}

src/query/storages/system/src/settings_table.rs

@@ -49,18 +49,29 @@ impl SyncSystemTable for SettingsTable {
         let mut names: Vec<String> = vec![];
         let mut values: Vec<String> = vec![];
         let mut defaults: Vec<String> = vec![];
+        let mut ranges: Vec<String> = vec![];
         let mut levels: Vec<String> = vec![];
         let mut descs: Vec<String> = vec![];
         let mut types: Vec<String> = vec![];
         for item in settings.into_iter() {
             // Name.
             names.push(item.name);
+
             // Value.
             values.push(escape(format!("{:?}", item.user_value).as_str()).to_string());
+
             // Default Value.
             defaults.push(escape(format!("{:?}", item.default_value).as_str()).to_string());
+
+            // Range Value.
+            match item.range {
+                Some(range) => ranges.push(format!("{}", range)),
+                None => ranges.push("None".to_string()),
+            }
+
             // Scope level.
             levels.push(format!("{:?}", item.level));
+
             // Desc.
             descs.push(item.desc.to_string());
 
@@ -75,6 +86,7 @@ impl SyncSystemTable for SettingsTable {
         let names: Vec<Vec<u8>> = names.iter().map(|x| x.as_bytes().to_vec()).collect();
         let values: Vec<Vec<u8>> = values.iter().map(|x| x.as_bytes().to_vec()).collect();
         let defaults: Vec<Vec<u8>> = defaults.iter().map(|x| x.as_bytes().to_vec()).collect();
+        let ranges: Vec<Vec<u8>> = ranges.iter().map(|x| x.as_bytes().to_vec()).collect();
         let levels: Vec<Vec<u8>> = levels.iter().map(|x| x.as_bytes().to_vec()).collect();
         let descs: Vec<Vec<u8>> = descs.iter().map(|x| x.as_bytes().to_vec()).collect();
         let types: Vec<Vec<u8>> = types.iter().map(|x| x.as_bytes().to_vec()).collect();
@@ -83,6 +95,7 @@ impl SyncSystemTable for SettingsTable {
             StringType::from_data(names),
             StringType::from_data(values),
             StringType::from_data(defaults),
+            StringType::from_data(ranges),
             StringType::from_data(levels),
             StringType::from_data(descs),
             StringType::from_data(types),
@@ -96,6 +109,7 @@ impl SettingsTable {
             TableField::new("name", TableDataType::String),
             TableField::new("value", TableDataType::String),
             TableField::new("default", TableDataType::String),
+            TableField::new("range", TableDataType::String),
             TableField::new("level", TableDataType::String),
             TableField::new("description", TableDataType::String),
             TableField::new("type", TableDataType::String),