From 0de3b72814be73c246c4f0e76cde7802b77eebd2 Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 10:45:26 +0800 Subject: [PATCH 01/11] chore: merge settings possible_values to range --- Cargo.lock | 1 + src/query/settings/Cargo.toml | 1 + src/query/settings/src/settings.rs | 3 - src/query/settings/src/settings_default.rs | 226 ++++++++---------- .../settings/src/settings_getter_setter.rs | 54 +++-- src/query/settings/src/settings_global.rs | 37 ++- 6 files changed, 160 insertions(+), 162 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dbb9d411f8a5a..2a52c49de3b02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2663,6 +2663,7 @@ dependencies = [ "dashmap", "itertools 0.10.5", "log", + "num", "num_cpus", "once_cell", "serde", diff --git a/src/query/settings/Cargo.toml b/src/query/settings/Cargo.toml index db1cd9ced37cb..ca484350ba674 100644 --- a/src/query/settings/Cargo.toml +++ b/src/query/settings/Cargo.toml @@ -24,3 +24,4 @@ log = { workspace = true } num_cpus = "1.13.1" once_cell = { workspace = true } sys-info = "0.9" +num = "0.4.1" diff --git a/src/query/settings/src/settings.rs b/src/query/settings/src/settings.rs index 551357851dbc1..afa8b9a4fc154 100644 --- a/src/query/settings/src/settings.rs +++ b/src/query/settings/src/settings.rs @@ -115,7 +115,6 @@ pub struct SettingsItem { pub desc: &'static str, pub user_value: UserSettingValue, pub default_value: UserSettingValue, - pub possible_values: Option>, } pub struct SettingsIter<'a> { @@ -156,7 +155,6 @@ 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, }, Some(change_value) => SettingsItem { name: key, @@ -164,7 +162,6 @@ impl<'a> Iterator for SettingsIter<'a> { desc: default_value.desc, user_value: change_value.value.clone(), default_value: default_value.value, - possible_values: default_value.possible_values, }, }), }; diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index a85bf395f43c0..4c1d838796052 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -37,13 +37,56 @@ pub enum SettingMode { Write, } +#[derive(Clone, Debug)] +pub enum SettingRange { + Numeric(Range), + String(Vec<&'static str>), +} + +impl SettingRange { + /// Checks if an integer value is within the numeric range. + pub fn is_within_numeric_range(&self, value: u64) -> Result<()> { + match self { + SettingRange::Numeric(range) => { + if range.contains(&value) { + Ok(()) + } else { + Err(ErrorCode::BadArguments(format!( + "Value {} is not within the range {:?}", + value, range + ))) + } + } + _ => Err(ErrorCode::BadArguments( + "Expected numeric range".to_string(), + )), + } + } + + /// Checks if a string value is within the string range. + pub fn is_within_string_range(&self, value: &str) -> Result<()> { + match self { + SettingRange::String(values) => { + if values.iter().any(|s| s.eq_ignore_ascii_case(value)) { + Ok(()) + } else { + Err(ErrorCode::BadArguments(format!( + "Value {} is not within the allowed values {:?}", + value, values + ))) + } + } + _ => Err(ErrorCode::BadArguments("Expected string range".to_string())), + } + } +} + #[derive(Clone, Debug)] pub struct DefaultSettingValue { pub(crate) value: UserSettingValue, pub(crate) desc: &'static str, - pub(crate) possible_values: Option>, pub(crate) mode: SettingMode, - pub(crate) range: Option>, + pub(crate) range: Option, } #[derive(Clone)] @@ -64,28 +107,24 @@ impl DefaultSettings { ("max_block_size", DefaultSettingValue { value: UserSettingValue::UInt64(65536), desc: "Sets the maximum byte size of a single data block that can be read.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("parquet_max_block_size", DefaultSettingValue { value: UserSettingValue::UInt64(8192), desc: "Max block size for parquet reader", - possible_values: None, mode: SettingMode::Both, range: None, }), ("max_threads", DefaultSettingValue { value: UserSettingValue::UInt64(num_cpus), desc: "Sets the maximum number of threads to execute a request.", - possible_values: None, mode: SettingMode::Both, - range: Some(1..1024), + range: Some(SettingRange::Numeric(1..1024)), }), ("max_memory_usage", DefaultSettingValue { value: UserSettingValue::UInt64(max_memory_usage), desc: "Sets the maximum memory usage in bytes for processing a single query.", - possible_values: None, mode: SettingMode::Both, range: None, }), @@ -93,36 +132,31 @@ impl DefaultSettings { // unit of retention_period is hour value: UserSettingValue::UInt64(12), desc: "Sets the retention period in hours.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("max_storage_io_requests", DefaultSettingValue { value: UserSettingValue::UInt64(default_max_storage_io_requests), desc: "Sets the maximum number of concurrent I/O requests.", - possible_values: None, mode: SettingMode::Both, - range: Some(1..1024), + range: Some(SettingRange::Numeric(1..1024)), }), ("storage_io_min_bytes_for_seek", DefaultSettingValue { value: UserSettingValue::UInt64(48), desc: "Sets the minimum byte size of data that must be read from storage in a single I/O operation \ when seeking a new location in the data file.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("storage_io_max_page_bytes_for_read", DefaultSettingValue { value: UserSettingValue::UInt64(512 * 1024), desc: "Sets the maximum byte size of data pages that can be read from storage in a single I/O operation.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("flight_client_timeout", DefaultSettingValue { value: UserSettingValue::UInt64(60), desc: "Sets the maximum time in seconds that a flight client request can be processed.", - possible_values: None, mode: SettingMode::Both, range: None, }), @@ -133,182 +167,160 @@ impl DefaultSettings { UserSettingValue::UInt64(result_timeout_secs) }, desc: "Set the timeout in seconds that a http query session expires without any polls.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("storage_read_buffer_size", DefaultSettingValue { value: UserSettingValue::UInt64(1024 * 1024), desc: "Sets the byte size of the buffer used for reading data into memory.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("input_read_buffer_size", DefaultSettingValue { value: UserSettingValue::UInt64(4 * 1024 * 1024), desc: "Sets the memory size in bytes allocated to the buffer used by the buffered reader to read data from storage.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("timezone", DefaultSettingValue { value: UserSettingValue::String("UTC".to_owned()), desc: "Sets the timezone.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("group_by_two_level_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(20000), desc: "Sets the number of keys in a GROUP BY operation that will trigger a two-level aggregation.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("max_inlist_to_or", DefaultSettingValue { value: UserSettingValue::UInt64(3), desc: "Sets the maximum number of values that can be included in an IN expression to be converted to an OR operator.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("unquoted_ident_case_sensitive", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Determines whether Databend treats unquoted identifiers as case-sensitive.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("quoted_ident_case_sensitive", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Determines whether Databend treats quoted identifiers as case-sensitive.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("sql_dialect", DefaultSettingValue { value: UserSettingValue::String("PostgreSQL".to_owned()), desc: "Sets the SQL dialect. Available values include \"PostgreSQL\", \"MySQL\", \"Experimental\", and \"Hive\".", - possible_values: Some(vec!["PostgreSQL", "MySQL", "Experimental", "Hive"]), mode: SettingMode::Both, - range: None, + range: Some(SettingRange::String(vec!["PostgreSQL", "MySQL", "Experimental", "Hive"])), }), ("enable_dphyp", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables dphyp join order algorithm.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_cbo", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables cost-based optimization.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("disable_join_reorder", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Disable join reorder optimization.", - possible_values: None, + mode: SettingMode::Both, range: None, }), ("join_spilling_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Maximum amount of memory can use for hash join, 0 is unlimited.", - possible_values: None, + mode: SettingMode::Both, range: None, }), ("enable_runtime_filter", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables runtime filter optimization for JOIN.", - possible_values: None, + mode: SettingMode::Both, range: None, }), ("max_execute_time_in_seconds", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Sets the maximum query execution time in seconds. Setting it to 0 means no limit.", - possible_values: None, + mode: SettingMode::Both, range: None, }), ("collation", DefaultSettingValue { value: UserSettingValue::String("binary".to_owned()), desc: "Sets the character collation. Available values include \"binary\" and \"utf8\".", - possible_values: Some(vec!["binary", "utf8"]), mode: SettingMode::Both, - range: None, + range: Some(SettingRange::String(vec!["binary", "utf8"])), }), ("max_result_rows", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Sets the maximum number of rows that can be returned in a query result when no specific row count is specified. Setting it to 0 means no limit.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("prefer_broadcast_join", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables broadcast join.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("storage_fetch_part_num", DefaultSettingValue { value: UserSettingValue::UInt64(2), desc: "Sets the number of partitions that are fetched in parallel from storage during query execution.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("load_file_metadata_expire_hours", DefaultSettingValue { value: UserSettingValue::UInt64(24 * 7), desc: "Sets the hours that the metadata of files you load data from with COPY INTO will expire in.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("hide_options_in_show_create_table", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Hides table-relevant information, such as SNAPSHOT_LOCATION and STORAGE_FORMAT, at the end of the result of SHOW TABLE CREATE.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("sandbox_tenant", DefaultSettingValue { value: UserSettingValue::String("".to_string()), desc: "Injects a custom 'sandbox_tenant' into this session. This is only for testing purposes and will take effect only when 'internal_enable_sandbox_tenant' is turned on.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("parquet_uncompressed_buffer_size", DefaultSettingValue { value: UserSettingValue::UInt64(2 * 1024 * 1024), desc: "Sets the byte size of the buffer used for reading Parquet files.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_bushy_join", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables generating a bushy join plan with the optimizer.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_query_result_cache", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables caching query results to improve performance for identical queries.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("query_result_cache_max_bytes", DefaultSettingValue { value: UserSettingValue::UInt64(1048576), // 1MB desc: "Sets the maximum byte size of cache for a single query result.", - possible_values: None, mode: SettingMode::Both, range: None, }), @@ -316,84 +328,72 @@ impl DefaultSettings { value: UserSettingValue::UInt64(300), // seconds desc: "Sets the time-to-live (TTL) in seconds for cached query results. \ Once the TTL for a cached result has expired, the result is considered stale and will not be used for new queries.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("query_result_cache_allow_inconsistent", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Determines whether Databend will return cached query results that are inconsistent with the underlying data.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_hive_parquet_predict_pushdown", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enable hive parquet predict pushdown by setting this variable to 1, default value: 1", - possible_values: None, mode: SettingMode::Both, range: None, }), ("hive_parquet_chunk_size", DefaultSettingValue { value: UserSettingValue::UInt64(16384), desc: "the max number of rows each read from parquet to databend processor", - possible_values: None, mode: SettingMode::Both, range: None, }), ("aggregate_spilling_bytes_threshold_per_proc", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Sets the maximum amount of memory in bytes that an aggregator can use before spilling data to storage during query execution.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("aggregate_spilling_memory_ratio", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Sets the maximum memory ratio in bytes that an aggregator can use before spilling data to storage during query execution.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("sort_spilling_bytes_threshold_per_proc", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Sets the maximum amount of memory in bytes that a sorter can use before spilling data to storage during query execution.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("sort_spilling_memory_ratio", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Sets the maximum memory ratio in bytes that a sorter can use before spilling data to storage during query execution.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("group_by_shuffle_mode", DefaultSettingValue { value: UserSettingValue::String(String::from("before_merge")), desc: "Group by shuffle mode, 'before_partial' is more balanced, but more data needs to exchange.", - possible_values: Some(vec!["before_partial", "before_merge"]), mode: SettingMode::Both, - range: None, + range: Some(SettingRange::String(vec!["before_partial", "before_merge"])), }), ("efficiently_memory_group_by", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Memory is used efficiently, but this may cause performance degradation.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("lazy_read_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(1000), desc: "Sets the maximum LIMIT in a query to enable lazy read optimization. Setting it to 0 disables the optimization.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("parquet_fast_read_bytes", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Parquet file with smaller size will be read as a whole file, instead of column by column.", - possible_values: None, mode: SettingMode::Both, range: None, }), @@ -402,7 +402,6 @@ impl DefaultSettings { ("enterprise_license", DefaultSettingValue { value: UserSettingValue::String("".to_owned()), desc: "License key for use enterprise features", - possible_values: None, // license key should not be reported mode: SettingMode::Write, range: None, @@ -410,231 +409,199 @@ impl DefaultSettings { ("enable_table_lock", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables table lock if necessary (enabled by default).", - possible_values: None, mode: SettingMode::Both, range: None, }), ("table_lock_expire_secs", DefaultSettingValue { value: UserSettingValue::UInt64(10), desc: "Sets the seconds that the table lock will expire in.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("acquire_lock_timeout", DefaultSettingValue { value: UserSettingValue::UInt64(15), desc: "Sets the maximum timeout in seconds for acquire a lock.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("deduplicate_label", DefaultSettingValue { value: UserSettingValue::String("".to_owned()), desc: "Sql duplicate label for deduplication.", - possible_values: None, mode: SettingMode::Write, range: None, }), ("enable_distributed_copy_into", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enable distributed execution of copy into.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_experimental_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable experimental merge into.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_distributed_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed merge into.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("merge_into_static_filter_partition_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(1500), desc: "Max number of partitions allowed for static filtering of merge into statement", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_distributed_replace_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of replace into.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_distributed_compact", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table compaction.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_aggregating_index_scan", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enable scanning aggregating index data while querying.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_recluster_after_write", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables re-clustering after write(copy/replace-into).", - possible_values: None, mode: SettingMode::Both, range: None, }), ("use_parquet2", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Use parquet2 instead of parquet_rs when infer_schema().", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_replace_into_partitioning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables partitioning for replace-into statement (if table has cluster keys).", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_replace_into_bloom_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables bloom pruning for replace-into statement.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("replace_into_bloom_pruning_max_column_number", DefaultSettingValue { value: UserSettingValue::UInt64(4), desc: "Max number of columns used by bloom pruning for replace-into statement.", - possible_values: None, + mode: SettingMode::Both, range: None, }), ("replace_into_shuffle_strategy", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "0 for Block level shuffle, 1 for segment level shuffle", - possible_values: None, mode: SettingMode::Both, range: None, }), ("recluster_timeout_secs", DefaultSettingValue { value: UserSettingValue::UInt64(12 * 60 * 60), desc: "Sets the seconds that recluster final will be timeout.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_refresh_aggregating_index_after_write", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Refresh aggregating index after new data written", - possible_values: None, mode: SettingMode::Both, range: None, }), ("ddl_column_type_nullable", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "If columns are default nullable when create or alter table", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_query_profiling", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables recording query profile", - possible_values: None, mode: SettingMode::Both, range: None, }), ("recluster_block_size", DefaultSettingValue { value: UserSettingValue::UInt64(recluster_block_size), desc: "Sets the maximum byte size of blocks for recluster", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_distributed_recluster", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table recluster.", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_parquet_page_index", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables parquet page index", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_parquet_rowgroup_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables parquet rowgroup pruning", - possible_values: None, mode: SettingMode::Both, range: None, }), ("external_server_connect_timeout_secs", DefaultSettingValue { value: UserSettingValue::UInt64(10), desc: "Connection timeout to external server", - possible_values: None, mode: SettingMode::Both, range: None, }), ("external_server_request_timeout_secs", DefaultSettingValue { value: UserSettingValue::UInt64(180), desc: "Request timeout to external server", - possible_values: None, mode: SettingMode::Both, range: None, }), ("enable_parquet_prewhere", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables parquet prewhere", - possible_values: None, mode: SettingMode::Both, range: None, }), ("numeric_cast_option", DefaultSettingValue { value: UserSettingValue::String("rounding".to_string()), desc: "Set numeric cast mode as \"rounding\" or \"truncating\".", - possible_values: Some(vec!["rounding", "truncating"]), mode: SettingMode::Both, - range: None, + range: Some(SettingRange::String(vec!["rounding", "truncating"])), }), ("enable_experimental_rbac_check", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "experiment setting disables stage and udf privilege check(disable by default).", - possible_values: None, mode: SettingMode::Both, range: None, }), ("create_query_flight_client_with_current_rt", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "create query flight client with current runtime", - possible_values: None, mode: SettingMode::Both, range: None, }), ("query_flight_compression", DefaultSettingValue { value: UserSettingValue::String(String::from("LZ4")), desc: "flight compression method", - possible_values: Some(vec!["None", "LZ4", "ZSTD"]), mode: SettingMode::Both, - range: None, + range: Some(SettingRange::String(vec!["None", "LZ4", "ZSTD"])), }), ("enable_refresh_virtual_column_after_write", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Refresh virtual column after new data written", - possible_values: None, mode: SettingMode::Both, range: None, }), @@ -711,56 +678,55 @@ impl DefaultSettings { Ok(Self::instance()?.settings.contains_key(key)) } - pub fn convert_value(k: String, mut v: String) -> Result<(String, Option)> { + /// Converts and validates a setting value based on its key. + /// + /// # Arguments + /// * `k` - The key of the setting. + /// * `v` - The value of the setting to be validated and converted. + /// + /// # Returns + /// * A result containing a tuple of the key and an optional `UserSettingValue` if valid, or an error. + pub fn convert_value(k: String, v: String) -> Result<(String, Option)> { + // Retrieve the default settings instance let default_settings = DefaultSettings::instance()?; + // Match the setting key with the settings definition match default_settings.settings.get(&k) { + // Return None for keys that are not found in the settings None => Ok((k, None)), + + // Process the setting value for found keys Some(setting_value) => { - if let Some(possible_values) = &setting_value.possible_values { - let mut checked_value = false; + match &setting_value.range { + None => Ok((k, None)), + Some(range) => { + match range { + // Numeric range. + SettingRange::Numeric(_) => { + // Attempt to parse the value as an unsigned 64-bit integer + let u64_val = v.parse::().map_err(|_| { + ErrorCode::BadArguments(format!( + "{} is not a valid u64 value", + v + )) + })?; + range.is_within_numeric_range(u64_val)?; + + // Return the key and value if valid + Ok((k, Some(UserSettingValue::UInt64(u64_val)))) + } + // String range. + SettingRange::String(_) => { + // Convert the value to lowercase for case-insensitive comparison + let val_lower = v.to_lowercase(); - for possible_value in possible_values { - if possible_value.eq_ignore_ascii_case(&v) { - checked_value = true; - v = possible_value.to_string(); - } - } + range.is_within_string_range(&val_lower)?; - if !checked_value { - return Err(ErrorCode::WrongValueForVariable(format!( - "Invalid setting value: {:?} for variable {:?}, possible values: {:?}", - v, k, possible_values - ))); - } - } - match setting_value.value { - UserSettingValue::UInt64(_) => { - // decimal 10 * 1.5 to string may result in string like "15.0" - let val = if let Some(p) = v.find('.') { - if v[(p + 1)..].chars().all(|x| x == '0') { - &v[..p] - } else { - return Err(ErrorCode::BadArguments("not a integer")); - } - } else { - &v[..] - }; - - let u64_val = val.parse::()?; - - if let Some(range) = &setting_value.range { - if !range.contains(&u64_val) { - return Err(ErrorCode::BadArguments(format!( - "{} value should in range: {:?}", - k, range - ))); + // Return the key and value if valid + Ok((k, Some(UserSettingValue::String(v)))) } } - - Ok((k, Some(UserSettingValue::UInt64(u64_val)))) } - UserSettingValue::String(_) => Ok((k, Some(UserSettingValue::String(v)))), } } } diff --git a/src/query/settings/src/settings_getter_setter.rs b/src/query/settings/src/settings_getter_setter.rs index 53b45cbeeb670..d27a2c239e5b2 100644 --- a/src/query/settings/src/settings_getter_setter.rs +++ b/src/query/settings/src/settings_getter_setter.rs @@ -64,35 +64,47 @@ 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) { + // Retrieve the instance of default settings + let default_settings = DefaultSettings::instance()?; + + // Check if the key exists in the default settings + match default_settings.settings.get(key) { + // If the key does not exist, return an error indicating an unknown variable 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 - ))); + // If the key exists, proceed to validate and set the value + Some(default_val) => { + // Ensure the setting type is UInt64 + match &default_val.value { + UserSettingValue::UInt64(_) => { + // If a numeric range is defined, validate the value against this range + if let Some(range) = &default_val.range { + // Check if the value falls within the numeric range + range.is_within_numeric_range(val).map_err(|err| { + ErrorCode::BadArguments(format!("{}: {}", key, err)) + })?; + } + + // 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 + ))), } - - self.changes.insert(key.to_string(), ChangeValue { - level: ScopeLevel::Session, - value: UserSettingValue::UInt64(val), - }); - - Ok(()) } } } diff --git a/src/query/settings/src/settings_global.rs b/src/query/settings/src/settings_global.rs index 75c754084ba0a..61aee9a4200b1 100644 --- a/src/query/settings/src/settings_global.rs +++ b/src/query/settings/src/settings_global.rs @@ -25,6 +25,7 @@ use log::warn; use crate::settings::ChangeValue; use crate::settings::Settings; use crate::settings_default::DefaultSettings; +use crate::settings_default::SettingRange; use crate::ScopeLevel; impl Settings { @@ -96,14 +97,34 @@ impl Settings { 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); + // Check if the setting value is valid based on the defined range + let validation_result = match &default_setting_value.range { + Some(SettingRange::Numeric(_)) => match val.parse::() { + Ok(num) => default_setting_value + .range + .as_ref() + .ok_or_else(|| "Range not set correctly".to_string()) + .and_then(|range| { + range + .is_within_numeric_range(num) + .map_err(|err| err.to_string()) + }), + Err(_) => Err("Value is not a valid u64".to_string()), + }, + Some(SettingRange::String(_)) => default_setting_value + .range + .as_ref() + .ok_or_else(|| "Range not set correctly".to_string()) + .and_then(|range| { + range + .is_within_string_range(&val) + .map_err(|err| err.to_string()) + }), + None => Ok(()), + }; + + if let Err(err) = validation_result { + warn!("Ignore invalid global setting {} = {}: {}", name, val, err); continue; } From a32ca7f75a9b2275864288a5cbed96859e3ddaeb Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 11:35:51 +0800 Subject: [PATCH 02/11] change enable_* to range [0, 1] --- Cargo.lock | 1 - src/query/settings/Cargo.toml | 1 - src/query/settings/src/settings_default.rs | 55 +++++++++++----------- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2a52c49de3b02..dbb9d411f8a5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2663,7 +2663,6 @@ dependencies = [ "dashmap", "itertools 0.10.5", "log", - "num", "num_cpus", "once_cell", "serde", diff --git a/src/query/settings/Cargo.toml b/src/query/settings/Cargo.toml index ca484350ba674..db1cd9ced37cb 100644 --- a/src/query/settings/Cargo.toml +++ b/src/query/settings/Cargo.toml @@ -24,4 +24,3 @@ log = { workspace = true } num_cpus = "1.13.1" once_cell = { workspace = true } sys-info = "0.9" -num = "0.4.1" diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index 4c1d838796052..700e15904437d 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -13,7 +13,7 @@ // limitations under the License. use std::collections::HashMap; -use std::ops::Range; +use std::ops::RangeInclusive; use std::sync::Arc; use common_config::GlobalConfig; @@ -39,7 +39,7 @@ pub enum SettingMode { #[derive(Clone, Debug)] pub enum SettingRange { - Numeric(Range), + Numeric(RangeInclusive), String(Vec<&'static str>), } @@ -120,7 +120,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(num_cpus), desc: "Sets the maximum number of threads to execute a request.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(1..1024)), + range: Some(SettingRange::Numeric(1..=1024)), }), ("max_memory_usage", DefaultSettingValue { value: UserSettingValue::UInt64(max_memory_usage), @@ -139,7 +139,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(default_max_storage_io_requests), desc: "Sets the maximum number of concurrent I/O requests.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(1..1024)), + range: Some(SettingRange::Numeric(1..=1024)), }), ("storage_io_min_bytes_for_seek", DefaultSettingValue { value: UserSettingValue::UInt64(48), @@ -222,13 +222,13 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enables dphyp join order algorithm.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_cbo", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables cost-based optimization.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("disable_join_reorder", DefaultSettingValue { value: UserSettingValue::UInt64(0), @@ -247,9 +247,8 @@ impl DefaultSettings { ("enable_runtime_filter", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables runtime filter optimization for JOIN.", - mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("max_execute_time_in_seconds", DefaultSettingValue { value: UserSettingValue::UInt64(0), @@ -310,7 +309,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables generating a bushy join plan with the optimizer.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_query_result_cache", DefaultSettingValue { value: UserSettingValue::UInt64(0), @@ -341,7 +340,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enable hive parquet predict pushdown by setting this variable to 1, default value: 1", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("hive_parquet_chunk_size", DefaultSettingValue { value: UserSettingValue::UInt64(16384), @@ -410,7 +409,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enables table lock if necessary (enabled by default).", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("table_lock_expire_secs", DefaultSettingValue { value: UserSettingValue::UInt64(10), @@ -434,19 +433,19 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enable distributed execution of copy into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_experimental_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable experimental merge into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_distributed_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed merge into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("merge_into_static_filter_partition_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(1500), @@ -458,43 +457,43 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of replace into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_distributed_compact", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table compaction.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_aggregating_index_scan", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enable scanning aggregating index data while querying.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_recluster_after_write", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables re-clustering after write(copy/replace-into).", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("use_parquet2", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Use parquet2 instead of parquet_rs when infer_schema().", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_replace_into_partitioning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables partitioning for replace-into statement (if table has cluster keys).", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_replace_into_bloom_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables bloom pruning for replace-into statement.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("replace_into_bloom_pruning_max_column_number", DefaultSettingValue { value: UserSettingValue::UInt64(4), @@ -519,7 +518,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Refresh aggregating index after new data written", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("ddl_column_type_nullable", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -531,7 +530,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables recording query profile", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("recluster_block_size", DefaultSettingValue { value: UserSettingValue::UInt64(recluster_block_size), @@ -543,13 +542,13 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table recluster.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_parquet_page_index", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables parquet page index", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_parquet_rowgroup_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -573,7 +572,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables parquet prewhere", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("numeric_cast_option", DefaultSettingValue { value: UserSettingValue::String("rounding".to_string()), @@ -585,7 +584,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "experiment setting disables stage and udf privilege check(disable by default).", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("create_query_flight_client_with_current_rt", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -603,7 +602,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Refresh virtual column after new data written", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ]); From a6b02ff105167d56734cbc7a422e62b53c7636b5 Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 13:13:59 +0800 Subject: [PATCH 03/11] add settings unit test --- Cargo.lock | 1 + src/query/settings/Cargo.toml | 3 + src/query/settings/src/settings_default.rs | 52 +++++++++-------- .../settings/src/settings_getter_setter.rs | 2 +- src/query/settings/src/settings_global.rs | 54 ++++-------------- src/query/settings/tests/it/main.rs | 14 +++++ src/query/settings/tests/it/setting.rs | 57 +++++++++++++++++++ 7 files changed, 113 insertions(+), 70 deletions(-) create mode 100644 src/query/settings/tests/it/main.rs create mode 100644 src/query/settings/tests/it/setting.rs diff --git a/Cargo.lock b/Cargo.lock index dbb9d411f8a5a..4909a8e4ebdb2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2667,6 +2667,7 @@ dependencies = [ "once_cell", "serde", "sys-info", + "tokio", ] [[package]] diff --git a/src/query/settings/Cargo.toml b/src/query/settings/Cargo.toml index db1cd9ced37cb..8298c736024a5 100644 --- a/src/query/settings/Cargo.toml +++ b/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 } diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index 700e15904437d..fc92b32d462c9 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -52,8 +52,10 @@ impl SettingRange { Ok(()) } else { Err(ErrorCode::BadArguments(format!( - "Value {} is not within the range {:?}", - value, range + "Value {} is not within the range [{}, {}]", + value, + range.start(), + range.end() ))) } } @@ -222,13 +224,13 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enables dphyp join order algorithm.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_cbo", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables cost-based optimization.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("disable_join_reorder", DefaultSettingValue { value: UserSettingValue::UInt64(0), @@ -248,7 +250,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables runtime filter optimization for JOIN.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("max_execute_time_in_seconds", DefaultSettingValue { value: UserSettingValue::UInt64(0), @@ -309,7 +311,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables generating a bushy join plan with the optimizer.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_query_result_cache", DefaultSettingValue { value: UserSettingValue::UInt64(0), @@ -340,7 +342,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enable hive parquet predict pushdown by setting this variable to 1, default value: 1", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("hive_parquet_chunk_size", DefaultSettingValue { value: UserSettingValue::UInt64(16384), @@ -409,7 +411,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enables table lock if necessary (enabled by default).", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("table_lock_expire_secs", DefaultSettingValue { value: UserSettingValue::UInt64(10), @@ -433,19 +435,19 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enable distributed execution of copy into.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_experimental_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable experimental merge into.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_distributed_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed merge into.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("merge_into_static_filter_partition_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(1500), @@ -457,43 +459,43 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of replace into.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_distributed_compact", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table compaction.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_aggregating_index_scan", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enable scanning aggregating index data while querying.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_recluster_after_write", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables re-clustering after write(copy/replace-into).", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("use_parquet2", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Use parquet2 instead of parquet_rs when infer_schema().", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_replace_into_partitioning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables partitioning for replace-into statement (if table has cluster keys).", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_replace_into_bloom_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables bloom pruning for replace-into statement.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("replace_into_bloom_pruning_max_column_number", DefaultSettingValue { value: UserSettingValue::UInt64(4), @@ -518,7 +520,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Refresh aggregating index after new data written", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("ddl_column_type_nullable", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -530,7 +532,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables recording query profile", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("recluster_block_size", DefaultSettingValue { value: UserSettingValue::UInt64(recluster_block_size), @@ -542,13 +544,13 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table recluster.", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_parquet_page_index", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables parquet page index", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("enable_parquet_rowgroup_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -572,7 +574,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables parquet prewhere", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("numeric_cast_option", DefaultSettingValue { value: UserSettingValue::String("rounding".to_string()), @@ -584,7 +586,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "experiment setting disables stage and udf privilege check(disable by default).", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ("create_query_flight_client_with_current_rt", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -602,7 +604,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Refresh virtual column after new data written", mode: SettingMode::Both, - range: Some(SettingRange::Numeric(0..=1)), + range: None, }), ]); diff --git a/src/query/settings/src/settings_getter_setter.rs b/src/query/settings/src/settings_getter_setter.rs index d27a2c239e5b2..24dabd6accf23 100644 --- a/src/query/settings/src/settings_getter_setter.rs +++ b/src/query/settings/src/settings_getter_setter.rs @@ -87,7 +87,7 @@ impl Settings { if let Some(range) = &default_val.range { // Check if the value falls within the numeric range range.is_within_numeric_range(val).map_err(|err| { - ErrorCode::BadArguments(format!("{}: {}", key, err)) + ErrorCode::BadArguments(format!("{}: {}", key, err.message())) })?; } diff --git a/src/query/settings/src/settings_global.rs b/src/query/settings/src/settings_global.rs index 61aee9a4200b1..1894069f09595 100644 --- a/src/query/settings/src/settings_global.rs +++ b/src/query/settings/src/settings_global.rs @@ -25,7 +25,6 @@ use log::warn; use crate::settings::ChangeValue; use crate::settings::Settings; use crate::settings_default::DefaultSettings; -use crate::settings_default::SettingRange; use crate::ScopeLevel; impl Settings { @@ -96,49 +95,16 @@ impl Settings { warn!("Ignore deprecated global setting {} = {}", name, val); continue; } - Some(default_setting_value) => { - // Check if the setting value is valid based on the defined range - let validation_result = match &default_setting_value.range { - Some(SettingRange::Numeric(_)) => match val.parse::() { - Ok(num) => default_setting_value - .range - .as_ref() - .ok_or_else(|| "Range not set correctly".to_string()) - .and_then(|range| { - range - .is_within_numeric_range(num) - .map_err(|err| err.to_string()) - }), - Err(_) => Err("Value is not a valid u64".to_string()), - }, - Some(SettingRange::String(_)) => default_setting_value - .range - .as_ref() - .ok_or_else(|| "Range not set correctly".to_string()) - .and_then(|range| { - range - .is_within_string_range(&val) - .map_err(|err| err.to_string()) - }), - None => Ok(()), - }; - - if let Err(err) = validation_result { - warn!("Ignore invalid global setting {} = {}: {}", name, val, err); - continue; - } - - match &default_setting_value.value { - UserSettingValue::UInt64(_) => ChangeValue { - level: ScopeLevel::Global, - value: UserSettingValue::UInt64(val.parse::()?), - }, - 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::()?), + }, + UserSettingValue::String(_) => ChangeValue { + level: ScopeLevel::Global, + value: UserSettingValue::String(val.clone()), + }, + }, }); } diff --git a/src/query/settings/tests/it/main.rs b/src/query/settings/tests/it/main.rs new file mode 100644 index 0000000000000..c6ec4929b0993 --- /dev/null +++ b/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; diff --git a/src/query/settings/tests/it/setting.rs b/src/query/settings/tests/it/setting.rs new file mode 100644 index 0000000000000..09f93da735d96 --- /dev/null +++ b/src/query/settings/tests/it/setting.rs @@ -0,0 +1,57 @@ +// 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()); + // Ok. + { + settings.set_max_threads(2).unwrap(); + } + + // Number out of range. + { + let result = settings.set_max_threads(1025); + let expect = "BadArguments. Code: 1006, Text = max_threads: Value 1025 is not within the range [1, 1024]."; + assert_eq!(expect, format!("{}", result.unwrap_err())); + } + + // String out of range. + { + // 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 = "BadArguments. Code: 1006, Text = Value xx is not within the allowed values [\"None\", \"LZ4\", \"ZSTD\"]."; + assert_eq!(expect, format!("{}", result.unwrap_err())); + } +} + +#[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())); +} From 60d469b1c8847e541c2ea89cdc52bdcf70c4b5ea Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 13:30:23 +0800 Subject: [PATCH 04/11] merge with main --- src/query/settings/src/settings_default.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index 9bf9d77b2ff34..c3e17570344c7 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -109,7 +109,6 @@ impl DefaultSettings { ("enable_clickhouse_handler", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables clickhouse handler.", - possible_values: None, mode: SettingMode::Both, range: None, }), From c29d77c705b47579df2e8254a14b6a7e824d5314 Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 15:41:27 +0800 Subject: [PATCH 05/11] make the convert_value more simple --- src/query/settings/src/settings_default.rs | 83 +++++++++---------- .../settings/src/settings_getter_setter.rs | 81 ++++++++---------- src/query/settings/src/settings_global.rs | 25 ++---- 3 files changed, 81 insertions(+), 108 deletions(-) diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index c3e17570344c7..238573bbce9ca 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -686,53 +686,48 @@ impl DefaultSettings { } /// Converts and validates a setting value based on its key. - /// - /// # Arguments - /// * `k` - The key of the setting. - /// * `v` - The value of the setting to be validated and converted. - /// - /// # Returns - /// * A result containing a tuple of the key and an optional `UserSettingValue` if valid, or an error. - pub fn convert_value(k: String, v: String) -> Result<(String, Option)> { + pub fn convert_value(k: String, v: String) -> Result<(String, UserSettingValue)> { // Retrieve the default settings instance let default_settings = DefaultSettings::instance()?; - // Match the setting key with the settings definition - match default_settings.settings.get(&k) { - // Return None for keys that are not found in the settings - None => Ok((k, None)), - - // Process the setting value for found keys - Some(setting_value) => { - match &setting_value.range { - None => Ok((k, None)), - Some(range) => { - match range { - // Numeric range. - SettingRange::Numeric(_) => { - // Attempt to parse the value as an unsigned 64-bit integer - let u64_val = v.parse::().map_err(|_| { - ErrorCode::BadArguments(format!( - "{} is not a valid u64 value", - v - )) - })?; - range.is_within_numeric_range(u64_val)?; - - // Return the key and value if valid - Ok((k, Some(UserSettingValue::UInt64(u64_val)))) - } - // String range. - SettingRange::String(_) => { - // Convert the value to lowercase for case-insensitive comparison - let val_lower = v.to_lowercase(); - - range.is_within_string_range(&val_lower)?; - - // Return the key and value if valid - Ok((k, Some(UserSettingValue::String(v)))) - } - } + let setting_value = default_settings + .settings + .get(&k) + .ok_or_else(|| ErrorCode::UnknownVariable(format!("Unknown variable: {:?}", k)))?; + + match &setting_value.range { + None => { + match setting_value.value { + // Numeric value. + UserSettingValue::UInt64(_) => { + let u64_val = v.parse::().map_err(|_| { + ErrorCode::BadArguments(format!("{} is not a valid u64 value", v)) + })?; + + Ok((k, UserSettingValue::UInt64(u64_val))) + } + // String value. + UserSettingValue::String(_) => Ok((k, UserSettingValue::String(v))), + } + } + Some(range) => { + match range { + // Numeric range. + SettingRange::Numeric(_) => { + let u64_val = v.parse::().map_err(|_| { + ErrorCode::BadArguments(format!("{} is not a valid u64 value", v)) + })?; + range.is_within_numeric_range(u64_val)?; + + Ok((k, UserSettingValue::UInt64(u64_val))) + } + // String range. + SettingRange::String(_) => { + // Convert the value to lowercase for case-insensitive comparison + let val_lower = v.to_lowercase(); + range.is_within_string_range(&val_lower)?; + + Ok((k, UserSettingValue::String(v))) } } } diff --git a/src/query/settings/src/settings_getter_setter.rs b/src/query/settings/src/settings_getter_setter.rs index f4a1110cef13f..24564bb48010b 100644 --- a/src/query/settings/src/settings_getter_setter.rs +++ b/src/query/settings/src/settings_getter_setter.rs @@ -70,42 +70,34 @@ impl Settings { // Retrieve the instance of default settings let default_settings = DefaultSettings::instance()?; - // Check if the key exists in the default settings - match default_settings.settings.get(key) { - // If the key does not exist, return an error indicating an unknown variable - None => Err(ErrorCode::UnknownVariable(format!( - "Unknown variable: {:?}", - key - ))), - - // If the key exists, proceed to validate and set the value - Some(default_val) => { - // Ensure the setting type is UInt64 - match &default_val.value { - UserSettingValue::UInt64(_) => { - // If a numeric range is defined, validate the value against this range - if let Some(range) = &default_val.range { - // Check if the value falls within the numeric range - range.is_within_numeric_range(val).map_err(|err| { - ErrorCode::BadArguments(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 - ))), + 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::BadArguments(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 + ))), } } @@ -115,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 { diff --git a/src/query/settings/src/settings_global.rs b/src/query/settings/src/settings_global.rs index 1894069f09595..2cec85c2a82a5 100644 --- a/src/query/settings/src/settings_global.rs +++ b/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] From fb0003a7f7e69954b1c27d788b3127d76bbfac83 Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 16:02:15 +0800 Subject: [PATCH 06/11] change the string setting range return the value --- src/query/settings/src/settings_default.rs | 18 ++++++++---------- src/query/settings/tests/it/setting.rs | 13 +++++++++++++ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index 238573bbce9ca..24938b01fab83 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -66,16 +66,15 @@ impl SettingRange { } /// Checks if a string value is within the string range. - pub fn is_within_string_range(&self, value: &str) -> Result<()> { + pub fn is_within_string_range(&self, value: &str) -> Result { match self { SettingRange::String(values) => { - if values.iter().any(|s| s.eq_ignore_ascii_case(value)) { - Ok(()) - } else { - Err(ErrorCode::BadArguments(format!( + match values.iter().find(|&s| s.eq_ignore_ascii_case(value)) { + Some(s) => Ok(s.to_string()), + None => Err(ErrorCode::BadArguments(format!( "Value {} is not within the allowed values {:?}", value, values - ))) + ))), } } _ => Err(ErrorCode::BadArguments("Expected string range".to_string())), @@ -723,11 +722,10 @@ impl DefaultSettings { } // String range. SettingRange::String(_) => { - // Convert the value to lowercase for case-insensitive comparison - let val_lower = v.to_lowercase(); - range.is_within_string_range(&val_lower)?; + // value is the standard value of the setting. + let value = range.is_within_string_range(&v)?; - Ok((k, UserSettingValue::String(v))) + Ok((k, UserSettingValue::String(value))) } } } diff --git a/src/query/settings/tests/it/setting.rs b/src/query/settings/tests/it/setting.rs index 09f93da735d96..8b89d72097cc2 100644 --- a/src/query/settings/tests/it/setting.rs +++ b/src/query/settings/tests/it/setting.rs @@ -36,11 +36,24 @@ fn test_set_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 = "BadArguments. Code: 1006, 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)] From 32277b6860dc5844d1ff1ccac9bb75b4275f9058 Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 16:19:27 +0800 Subject: [PATCH 07/11] add enable bool setting range to: [0, 1] --- src/query/settings/src/settings_default.rs | 56 +++++++++++----------- src/query/settings/tests/it/setting.rs | 32 +++++++++++-- 2 files changed, 55 insertions(+), 33 deletions(-) diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index 24938b01fab83..4a4f0390ecb0c 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -109,7 +109,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables clickhouse handler.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("max_block_size", DefaultSettingValue { value: UserSettingValue::UInt64(65536), @@ -229,13 +229,13 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enables dphyp join order algorithm.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_cbo", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables cost-based optimization.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("disable_join_reorder", DefaultSettingValue { value: UserSettingValue::UInt64(0), @@ -247,7 +247,6 @@ impl DefaultSettings { ("join_spilling_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Maximum amount of memory can use for hash join, 0 is unlimited.", - mode: SettingMode::Both, range: None, }), @@ -255,12 +254,11 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables runtime filter optimization for JOIN.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("max_execute_time_in_seconds", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Sets the maximum query execution time in seconds. Setting it to 0 means no limit.", - mode: SettingMode::Both, range: None, }), @@ -316,13 +314,13 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables generating a bushy join plan with the optimizer.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_query_result_cache", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enables caching query results to improve performance for identical queries.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("query_result_cache_max_bytes", DefaultSettingValue { value: UserSettingValue::UInt64(1048576), // 1MB @@ -347,7 +345,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enable hive parquet predict pushdown by setting this variable to 1, default value: 1", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("hive_parquet_chunk_size", DefaultSettingValue { value: UserSettingValue::UInt64(16384), @@ -416,7 +414,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enables table lock if necessary (enabled by default).", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("table_lock_expire_secs", DefaultSettingValue { value: UserSettingValue::UInt64(10), @@ -440,19 +438,19 @@ impl DefaultSettings { value: UserSettingValue::UInt64(1), desc: "Enable distributed execution of copy into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_experimental_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable experimental merge into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_distributed_merge_into", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed merge into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("merge_into_static_filter_partition_threshold", DefaultSettingValue { value: UserSettingValue::UInt64(1500), @@ -464,19 +462,19 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of replace into.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_distributed_compact", DefaultSettingValue { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table compaction.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_aggregating_index_scan", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enable scanning aggregating index data while querying.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_recluster_after_write", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -488,19 +486,19 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Use parquet2 instead of parquet_rs when infer_schema().", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_replace_into_partitioning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables partitioning for replace-into statement (if table has cluster keys).", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_replace_into_bloom_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables bloom pruning for replace-into statement.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("replace_into_bloom_pruning_max_column_number", DefaultSettingValue { value: UserSettingValue::UInt64(4), @@ -525,7 +523,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Refresh aggregating index after new data written", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("ddl_column_type_nullable", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -537,7 +535,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables recording query profile", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("recluster_block_size", DefaultSettingValue { value: UserSettingValue::UInt64(recluster_block_size), @@ -549,19 +547,19 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enable distributed execution of table recluster.", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_parquet_page_index", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables parquet page index", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("enable_parquet_rowgroup_pruning", DefaultSettingValue { value: UserSettingValue::UInt64(1), desc: "Enables parquet rowgroup pruning", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("external_server_connect_timeout_secs", DefaultSettingValue { value: UserSettingValue::UInt64(10), @@ -579,7 +577,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Enables parquet prewhere", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("numeric_cast_option", DefaultSettingValue { value: UserSettingValue::String("rounding".to_string()), @@ -591,7 +589,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "experiment setting disables stage and udf privilege check(disable by default).", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ("create_query_flight_client_with_current_rt", DefaultSettingValue { value: UserSettingValue::UInt64(1), @@ -609,7 +607,7 @@ impl DefaultSettings { value: UserSettingValue::UInt64(0), desc: "Refresh virtual column after new data written", mode: SettingMode::Both, - range: None, + range: Some(SettingRange::Numeric(0..=1)), }), ]); @@ -700,7 +698,7 @@ impl DefaultSettings { // Numeric value. UserSettingValue::UInt64(_) => { let u64_val = v.parse::().map_err(|_| { - ErrorCode::BadArguments(format!("{} is not a valid u64 value", v)) + ErrorCode::BadArguments(format!("{} is not a valid integer value", v)) })?; Ok((k, UserSettingValue::UInt64(u64_val))) @@ -714,7 +712,7 @@ impl DefaultSettings { // Numeric range. SettingRange::Numeric(_) => { let u64_val = v.parse::().map_err(|_| { - ErrorCode::BadArguments(format!("{} is not a valid u64 value", v)) + ErrorCode::BadArguments(format!("{} is not a valid integer value", v)) })?; range.is_within_numeric_range(u64_val)?; diff --git a/src/query/settings/tests/it/setting.rs b/src/query/settings/tests/it/setting.rs index 8b89d72097cc2..ed6e44ee01538 100644 --- a/src/query/settings/tests/it/setting.rs +++ b/src/query/settings/tests/it/setting.rs @@ -17,18 +17,42 @@ use common_settings::Settings; #[test] fn test_set_settings() { let settings = Settings::create("test".to_string()); - // Ok. + // Number range. { settings.set_max_threads(2).unwrap(); - } - // Number out of range. - { let result = settings.set_max_threads(1025); let expect = "BadArguments. Code: 1006, 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(); + + // Error + let result = settings.set_setting("enable_table_lock".to_string(), "1.0".to_string()); + let expect = "BadArguments. Code: 1006, Text = 1.0 is not a valid integer value."; + assert_eq!(expect, format!("{}", result.unwrap_err())); + + // Error + let result = settings.set_setting("enable_table_lock".to_string(), "3".to_string()); + let expect = "BadArguments. Code: 1006, 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 = "BadArguments. Code: 1006, Text = xx is not a valid integer value."; + assert_eq!(expect, format!("{}", result.unwrap_err())); + } + // String out of range. { // Ok From fd993dc4562a009323e24ac7a1a03b28e7e62c2f Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 17:05:13 +0800 Subject: [PATCH 08/11] add f64 to settings and change the settings range check error to WrongValueForVariable --- .../it/storages/testdata/columns_table.txt | 1 + .../it/storages/testdata/settings_table.txt | 160 +++++++++--------- src/query/settings/src/lib.rs | 1 + src/query/settings/src/settings.rs | 4 + src/query/settings/src/settings_default.rs | 53 ++++-- .../settings/src/settings_getter_setter.rs | 2 +- src/query/settings/tests/it/setting.rs | 22 ++- .../storages/system/src/settings_table.rs | 14 ++ 8 files changed, 153 insertions(+), 104 deletions(-) diff --git a/src/query/service/tests/it/storages/testdata/columns_table.txt b/src/query/service/tests/it/storages/testdata/columns_table.txt index d40948af536a8..78b4c13266450 100644 --- a/src/query/service/tests/it/storages/testdata/columns_table.txt +++ b/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' | '' | diff --git a/src/query/service/tests/it/storages/testdata/settings_table.txt b/src/query/service/tests/it/storages/testdata/settings_table.txt index b68f7c7bc1680..6b5ed2231ddd4 100644 --- a/src/query/service/tests/it/storages/testdata/settings_table.txt +++ b/src/query/service/tests/it/storages/testdata/settings_table.txt @@ -1,85 +1,85 @@ ---------- TABLE INFO ------------ DB.Table: 'system'.'settings', Table: settings-table_id:1, ver:0, Engine: SystemSettings -------- TABLE CONTENTS ---------- -+------------------------------------------------+----------------+----------------+-----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------+ -| Column 0 | Column 1 | Column 2 | Column 3 | Column 4 | Column 5 | -+------------------------------------------------+----------------+----------------+-----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------+ -| 'acquire_lock_timeout' | '15' | '15' | 'SESSION' | 'Sets the maximum timeout in seconds for acquire a lock.' | 'UInt64' | -| 'aggregate_spilling_bytes_threshold_per_proc' | '0' | '0' | 'SESSION' | 'Sets the maximum amount of memory in bytes that an aggregator can use before spilling data to storage during query execution.' | 'UInt64' | -| 'aggregate_spilling_memory_ratio' | '0' | '0' | 'SESSION' | 'Sets the maximum memory ratio in bytes that an aggregator can use before spilling data to storage during query execution.' | 'UInt64' | -| 'collation' | 'binary' | 'binary' | 'SESSION' | 'Sets the character collation. Available values include "binary" and "utf8".' | 'String' | -| 'create_query_flight_client_with_current_rt' | '1' | '1' | 'SESSION' | 'create query flight client with current runtime' | 'UInt64' | -| 'ddl_column_type_nullable' | '1' | '1' | 'SESSION' | 'If columns are default nullable when create or alter table' | 'UInt64' | -| 'disable_join_reorder' | '0' | '0' | 'SESSION' | 'Disable join reorder optimization.' | 'UInt64' | -| 'efficiently_memory_group_by' | '0' | '0' | 'SESSION' | 'Memory is used efficiently, but this may cause performance degradation.' | 'UInt64' | -| 'enable_aggregating_index_scan' | '1' | '1' | 'SESSION' | 'Enable scanning aggregating index data while querying.' | 'UInt64' | -| 'enable_bushy_join' | '0' | '0' | 'SESSION' | 'Enables generating a bushy join plan with the optimizer.' | 'UInt64' | -| 'enable_cbo' | '1' | '1' | 'SESSION' | 'Enables cost-based optimization.' | 'UInt64' | -| 'enable_clickhouse_handler' | '0' | '0' | 'SESSION' | 'Enables clickhouse handler.' | 'UInt64' | -| 'enable_distributed_compact' | '0' | '0' | 'SESSION' | 'Enable distributed execution of table compaction.' | 'UInt64' | -| 'enable_distributed_copy_into' | '1' | '1' | 'SESSION' | 'Enable distributed execution of copy into.' | 'UInt64' | -| 'enable_distributed_merge_into' | '0' | '0' | 'SESSION' | 'Enable distributed merge into.' | 'UInt64' | -| 'enable_distributed_recluster' | '0' | '0' | 'SESSION' | 'Enable distributed execution of table recluster.' | 'UInt64' | -| 'enable_distributed_replace_into' | '0' | '0' | 'SESSION' | 'Enable distributed execution of replace into.' | 'UInt64' | -| 'enable_dphyp' | '1' | '1' | 'SESSION' | 'Enables dphyp join order algorithm.' | 'UInt64' | -| 'enable_experimental_merge_into' | '0' | '0' | 'SESSION' | 'Enable experimental merge into.' | 'UInt64' | -| 'enable_experimental_rbac_check' | '0' | '0' | 'SESSION' | 'experiment setting disables stage and udf privilege check(disable by default).' | 'UInt64' | -| 'enable_hive_parquet_predict_pushdown' | '1' | '1' | 'SESSION' | 'Enable hive parquet predict pushdown by setting this variable to 1, default value: 1' | 'UInt64' | -| 'enable_parquet_page_index' | '1' | '1' | 'SESSION' | 'Enables parquet page index' | 'UInt64' | -| 'enable_parquet_prewhere' | '0' | '0' | 'SESSION' | 'Enables parquet prewhere' | 'UInt64' | -| 'enable_parquet_rowgroup_pruning' | '1' | '1' | 'SESSION' | 'Enables parquet rowgroup pruning' | 'UInt64' | -| 'enable_query_profiling' | '0' | '0' | 'SESSION' | 'Enables recording query profile' | 'UInt64' | -| 'enable_query_result_cache' | '0' | '0' | 'SESSION' | 'Enables caching query results to improve performance for identical queries.' | 'UInt64' | -| 'enable_recluster_after_write' | '1' | '1' | 'SESSION' | 'Enables re-clustering after write(copy/replace-into).' | 'UInt64' | -| 'enable_refresh_aggregating_index_after_write' | '0' | '0' | 'SESSION' | 'Refresh aggregating index after new data written' | 'UInt64' | -| 'enable_refresh_virtual_column_after_write' | '0' | '0' | 'SESSION' | 'Refresh virtual column after new data written' | 'UInt64' | -| 'enable_replace_into_bloom_pruning' | '1' | '1' | 'SESSION' | 'Enables bloom pruning for replace-into statement.' | 'UInt64' | -| 'enable_replace_into_partitioning' | '1' | '1' | 'SESSION' | 'Enables partitioning for replace-into statement (if table has cluster keys).' | 'UInt64' | -| 'enable_runtime_filter' | '0' | '0' | 'SESSION' | 'Enables runtime filter optimization for JOIN.' | 'UInt64' | -| 'enable_table_lock' | '1' | '1' | 'SESSION' | 'Enables table lock if necessary (enabled by default).' | 'UInt64' | -| 'external_server_connect_timeout_secs' | '10' | '10' | 'SESSION' | 'Connection timeout to external server' | 'UInt64' | -| 'external_server_request_timeout_secs' | '180' | '180' | 'SESSION' | 'Request timeout to external server' | 'UInt64' | -| 'flight_client_timeout' | '60' | '60' | 'SESSION' | 'Sets the maximum time in seconds that a flight client request can be processed.' | 'UInt64' | -| 'group_by_shuffle_mode' | 'before_merge' | 'before_merge' | 'SESSION' | 'Group by shuffle mode, 'before_partial' is more balanced, but more data needs to exchange.' | 'String' | -| 'group_by_two_level_threshold' | '20000' | '20000' | 'SESSION' | 'Sets the number of keys in a GROUP BY operation that will trigger a two-level aggregation.' | 'UInt64' | -| 'hide_options_in_show_create_table' | '1' | '1' | 'SESSION' | 'Hides table-relevant information, such as SNAPSHOT_LOCATION and STORAGE_FORMAT, at the end of the result of SHOW TABLE CREATE.' | 'UInt64' | -| 'hive_parquet_chunk_size' | '16384' | '16384' | 'SESSION' | 'the max number of rows each read from parquet to databend processor' | 'UInt64' | -| 'http_handler_result_timeout_secs' | '60' | '60' | 'SESSION' | 'Set the timeout in seconds that a http query session expires without any polls.' | 'UInt64' | -| 'input_read_buffer_size' | '4194304' | '4194304' | 'SESSION' | 'Sets the memory size in bytes allocated to the buffer used by the buffered reader to read data from storage.' | 'UInt64' | -| 'join_spilling_threshold' | '0' | '0' | 'SESSION' | 'Maximum amount of memory can use for hash join, 0 is unlimited.' | 'UInt64' | -| 'lazy_read_threshold' | '1000' | '1000' | 'SESSION' | 'Sets the maximum LIMIT in a query to enable lazy read optimization. Setting it to 0 disables the optimization.' | 'UInt64' | -| 'load_file_metadata_expire_hours' | '168' | '168' | 'SESSION' | 'Sets the hours that the metadata of files you load data from with COPY INTO will expire in.' | 'UInt64' | -| 'max_block_size' | '65536' | '65536' | 'SESSION' | 'Sets the maximum byte size of a single data block that can be read.' | 'UInt64' | -| 'max_execute_time_in_seconds' | '0' | '0' | 'SESSION' | 'Sets the maximum query execution time in seconds. Setting it to 0 means no limit.' | 'UInt64' | -| 'max_inlist_to_or' | '3' | '3' | 'SESSION' | 'Sets the maximum number of values that can be included in an IN expression to be converted to an OR operator.' | 'UInt64' | -| 'max_result_rows' | '0' | '0' | 'SESSION' | 'Sets the maximum number of rows that can be returned in a query result when no specific row count is specified. Setting it to 0 means no limit.' | 'UInt64' | -| 'merge_into_static_filter_partition_threshold' | '1500' | '1500' | 'SESSION' | 'Max number of partitions allowed for static filtering of merge into statement' | 'UInt64' | -| 'numeric_cast_option' | 'rounding' | 'rounding' | 'SESSION' | 'Set numeric cast mode as "rounding" or "truncating".' | 'String' | -| 'parquet_fast_read_bytes' | '0' | '0' | 'SESSION' | 'Parquet file with smaller size will be read as a whole file, instead of column by column.' | 'UInt64' | -| 'parquet_max_block_size' | '8192' | '8192' | 'SESSION' | 'Max block size for parquet reader' | 'UInt64' | -| 'parquet_uncompressed_buffer_size' | '2097152' | '2097152' | 'SESSION' | 'Sets the byte size of the buffer used for reading Parquet files.' | 'UInt64' | -| 'prefer_broadcast_join' | '1' | '1' | 'SESSION' | 'Enables broadcast join.' | 'UInt64' | -| 'query_flight_compression' | 'LZ4' | 'LZ4' | 'SESSION' | 'flight compression method' | 'String' | -| 'query_result_cache_allow_inconsistent' | '0' | '0' | 'SESSION' | 'Determines whether Databend will return cached query results that are inconsistent with the underlying data.' | 'UInt64' | -| 'query_result_cache_max_bytes' | '1048576' | '1048576' | 'SESSION' | 'Sets the maximum byte size of cache for a single query result.' | 'UInt64' | -| 'query_result_cache_ttl_secs' | '300' | '300' | 'SESSION' | 'Sets the time-to-live (TTL) in seconds for cached query results. Once the TTL for a cached result has expired, the result is considered stale and will not be used for new queries.' | 'UInt64' | -| 'quoted_ident_case_sensitive' | '1' | '1' | 'SESSION' | 'Determines whether Databend treats quoted identifiers as case-sensitive.' | 'UInt64' | -| 'recluster_timeout_secs' | '43200' | '43200' | 'SESSION' | 'Sets the seconds that recluster final will be timeout.' | 'UInt64' | -| 'replace_into_bloom_pruning_max_column_number' | '4' | '4' | 'SESSION' | 'Max number of columns used by bloom pruning for replace-into statement.' | 'UInt64' | -| 'replace_into_shuffle_strategy' | '0' | '0' | 'SESSION' | '0 for Block level shuffle, 1 for segment level shuffle' | 'UInt64' | -| 'retention_period' | '12' | '12' | 'SESSION' | 'Sets the retention period in hours.' | 'UInt64' | -| 'sandbox_tenant' | '' | '' | 'SESSION' | 'Injects a custom 'sandbox_tenant' into this session. This is only for testing purposes and will take effect only when 'internal_enable_sandbox_tenant' is turned on.' | 'String' | -| 'sort_spilling_bytes_threshold_per_proc' | '0' | '0' | 'SESSION' | 'Sets the maximum amount of memory in bytes that a sorter can use before spilling data to storage during query execution.' | 'UInt64' | -| 'sort_spilling_memory_ratio' | '0' | '0' | 'SESSION' | 'Sets the maximum memory ratio in bytes that a sorter can use before spilling data to storage during query execution.' | 'UInt64' | -| 'sql_dialect' | 'PostgreSQL' | 'PostgreSQL' | 'SESSION' | 'Sets the SQL dialect. Available values include "PostgreSQL", "MySQL", "Experimental", and "Hive".' | 'String' | -| 'storage_fetch_part_num' | '2' | '2' | 'SESSION' | 'Sets the number of partitions that are fetched in parallel from storage during query execution.' | 'UInt64' | -| 'storage_io_max_page_bytes_for_read' | '524288' | '524288' | 'SESSION' | 'Sets the maximum byte size of data pages that can be read from storage in a single I/O operation.' | 'UInt64' | -| 'storage_io_min_bytes_for_seek' | '48' | '48' | 'SESSION' | 'Sets the minimum byte size of data that must be read from storage in a single I/O operation when seeking a new location in the data file.' | 'UInt64' | -| 'storage_read_buffer_size' | '1048576' | '1048576' | 'SESSION' | 'Sets the byte size of the buffer used for reading data into memory.' | 'UInt64' | -| 'table_lock_expire_secs' | '10' | '10' | 'SESSION' | 'Sets the seconds that the table lock will expire in.' | 'UInt64' | -| 'timezone' | 'UTC' | 'UTC' | 'SESSION' | 'Sets the timezone.' | 'String' | -| 'unquoted_ident_case_sensitive' | '0' | '0' | 'SESSION' | 'Determines whether Databend treats unquoted identifiers as case-sensitive.' | 'UInt64' | -| 'use_parquet2' | '0' | '0' | 'SESSION' | 'Use parquet2 instead of parquet_rs when infer_schema().' | 'UInt64' | -+------------------------------------------------+----------------+----------------+-----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------+ ++------------------------------------------------+----------------+----------------+---------------------------------------------------+-----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------+ +| Column 0 | Column 1 | Column 2 | Column 3 | Column 4 | Column 5 | Column 6 | ++------------------------------------------------+----------------+----------------+---------------------------------------------------+-----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------+ +| 'acquire_lock_timeout' | '15' | '15' | 'None' | 'SESSION' | 'Sets the maximum timeout in seconds for acquire a lock.' | 'UInt64' | +| 'aggregate_spilling_bytes_threshold_per_proc' | '0' | '0' | 'None' | 'SESSION' | 'Sets the maximum amount of memory in bytes that an aggregator can use before spilling data to storage during query execution.' | 'UInt64' | +| 'aggregate_spilling_memory_ratio' | '0' | '0' | 'None' | 'SESSION' | 'Sets the maximum memory ratio in bytes that an aggregator can use before spilling data to storage during query execution.' | 'UInt64' | +| 'collation' | 'binary' | 'binary' | '["binary", "utf8"]' | 'SESSION' | 'Sets the character collation. Available values include "binary" and "utf8".' | 'String' | +| 'create_query_flight_client_with_current_rt' | '1' | '1' | 'None' | 'SESSION' | 'create query flight client with current runtime' | 'UInt64' | +| 'ddl_column_type_nullable' | '1' | '1' | 'None' | 'SESSION' | 'If columns are default nullable when create or alter table' | 'UInt64' | +| 'disable_join_reorder' | '0' | '0' | 'None' | 'SESSION' | 'Disable join reorder optimization.' | 'UInt64' | +| 'efficiently_memory_group_by' | '0' | '0' | 'None' | 'SESSION' | 'Memory is used efficiently, but this may cause performance degradation.' | 'UInt64' | +| 'enable_aggregating_index_scan' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enable scanning aggregating index data while querying.' | 'UInt64' | +| 'enable_bushy_join' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enables generating a bushy join plan with the optimizer.' | 'UInt64' | +| 'enable_cbo' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enables cost-based optimization.' | 'UInt64' | +| 'enable_clickhouse_handler' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enables clickhouse handler.' | 'UInt64' | +| 'enable_distributed_compact' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enable distributed execution of table compaction.' | 'UInt64' | +| 'enable_distributed_copy_into' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enable distributed execution of copy into.' | 'UInt64' | +| 'enable_distributed_merge_into' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enable distributed merge into.' | 'UInt64' | +| 'enable_distributed_recluster' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enable distributed execution of table recluster.' | 'UInt64' | +| 'enable_distributed_replace_into' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enable distributed execution of replace into.' | 'UInt64' | +| 'enable_dphyp' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enables dphyp join order algorithm.' | 'UInt64' | +| 'enable_experimental_merge_into' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enable experimental merge into.' | 'UInt64' | +| 'enable_experimental_rbac_check' | '0' | '0' | '[0, 1]' | 'SESSION' | 'experiment setting disables stage and udf privilege check(disable by default).' | 'UInt64' | +| 'enable_hive_parquet_predict_pushdown' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enable hive parquet predict pushdown by setting this variable to 1, default value: 1' | 'UInt64' | +| 'enable_parquet_page_index' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enables parquet page index' | 'UInt64' | +| 'enable_parquet_prewhere' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enables parquet prewhere' | 'UInt64' | +| 'enable_parquet_rowgroup_pruning' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enables parquet rowgroup pruning' | 'UInt64' | +| 'enable_query_profiling' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enables recording query profile' | 'UInt64' | +| 'enable_query_result_cache' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enables caching query results to improve performance for identical queries.' | 'UInt64' | +| 'enable_recluster_after_write' | '1' | '1' | 'None' | 'SESSION' | 'Enables re-clustering after write(copy/replace-into).' | 'UInt64' | +| 'enable_refresh_aggregating_index_after_write' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Refresh aggregating index after new data written' | 'UInt64' | +| 'enable_refresh_virtual_column_after_write' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Refresh virtual column after new data written' | 'UInt64' | +| 'enable_replace_into_bloom_pruning' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enables bloom pruning for replace-into statement.' | 'UInt64' | +| 'enable_replace_into_partitioning' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enables partitioning for replace-into statement (if table has cluster keys).' | 'UInt64' | +| 'enable_runtime_filter' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Enables runtime filter optimization for JOIN.' | 'UInt64' | +| 'enable_table_lock' | '1' | '1' | '[0, 1]' | 'SESSION' | 'Enables table lock if necessary (enabled by default).' | 'UInt64' | +| 'external_server_connect_timeout_secs' | '10' | '10' | 'None' | 'SESSION' | 'Connection timeout to external server' | 'UInt64' | +| 'external_server_request_timeout_secs' | '180' | '180' | 'None' | 'SESSION' | 'Request timeout to external server' | 'UInt64' | +| 'flight_client_timeout' | '60' | '60' | 'None' | 'SESSION' | 'Sets the maximum time in seconds that a flight client request can be processed.' | 'UInt64' | +| 'group_by_shuffle_mode' | 'before_merge' | 'before_merge' | '["before_partial", "before_merge"]' | 'SESSION' | 'Group by shuffle mode, 'before_partial' is more balanced, but more data needs to exchange.' | 'String' | +| 'group_by_two_level_threshold' | '20000' | '20000' | 'None' | 'SESSION' | 'Sets the number of keys in a GROUP BY operation that will trigger a two-level aggregation.' | 'UInt64' | +| 'hide_options_in_show_create_table' | '1' | '1' | 'None' | 'SESSION' | 'Hides table-relevant information, such as SNAPSHOT_LOCATION and STORAGE_FORMAT, at the end of the result of SHOW TABLE CREATE.' | 'UInt64' | +| 'hive_parquet_chunk_size' | '16384' | '16384' | 'None' | 'SESSION' | 'the max number of rows each read from parquet to databend processor' | 'UInt64' | +| 'http_handler_result_timeout_secs' | '60' | '60' | 'None' | 'SESSION' | 'Set the timeout in seconds that a http query session expires without any polls.' | 'UInt64' | +| 'input_read_buffer_size' | '4194304' | '4194304' | 'None' | 'SESSION' | 'Sets the memory size in bytes allocated to the buffer used by the buffered reader to read data from storage.' | 'UInt64' | +| 'join_spilling_threshold' | '0' | '0' | 'None' | 'SESSION' | 'Maximum amount of memory can use for hash join, 0 is unlimited.' | 'UInt64' | +| 'lazy_read_threshold' | '1000' | '1000' | 'None' | 'SESSION' | 'Sets the maximum LIMIT in a query to enable lazy read optimization. Setting it to 0 disables the optimization.' | 'UInt64' | +| 'load_file_metadata_expire_hours' | '168' | '168' | 'None' | 'SESSION' | 'Sets the hours that the metadata of files you load data from with COPY INTO will expire in.' | 'UInt64' | +| 'max_block_size' | '65536' | '65536' | 'None' | 'SESSION' | 'Sets the maximum byte size of a single data block that can be read.' | 'UInt64' | +| 'max_execute_time_in_seconds' | '0' | '0' | 'None' | 'SESSION' | 'Sets the maximum query execution time in seconds. Setting it to 0 means no limit.' | 'UInt64' | +| 'max_inlist_to_or' | '3' | '3' | 'None' | 'SESSION' | 'Sets the maximum number of values that can be included in an IN expression to be converted to an OR operator.' | 'UInt64' | +| 'max_result_rows' | '0' | '0' | 'None' | 'SESSION' | 'Sets the maximum number of rows that can be returned in a query result when no specific row count is specified. Setting it to 0 means no limit.' | 'UInt64' | +| 'merge_into_static_filter_partition_threshold' | '1500' | '1500' | 'None' | 'SESSION' | 'Max number of partitions allowed for static filtering of merge into statement' | 'UInt64' | +| 'numeric_cast_option' | 'rounding' | 'rounding' | '["rounding", "truncating"]' | 'SESSION' | 'Set numeric cast mode as "rounding" or "truncating".' | 'String' | +| 'parquet_fast_read_bytes' | '0' | '0' | 'None' | 'SESSION' | 'Parquet file with smaller size will be read as a whole file, instead of column by column.' | 'UInt64' | +| 'parquet_max_block_size' | '8192' | '8192' | 'None' | 'SESSION' | 'Max block size for parquet reader' | 'UInt64' | +| 'parquet_uncompressed_buffer_size' | '2097152' | '2097152' | 'None' | 'SESSION' | 'Sets the byte size of the buffer used for reading Parquet files.' | 'UInt64' | +| 'prefer_broadcast_join' | '1' | '1' | 'None' | 'SESSION' | 'Enables broadcast join.' | 'UInt64' | +| 'query_flight_compression' | 'LZ4' | 'LZ4' | '["None", "LZ4", "ZSTD"]' | 'SESSION' | 'flight compression method' | 'String' | +| 'query_result_cache_allow_inconsistent' | '0' | '0' | 'None' | 'SESSION' | 'Determines whether Databend will return cached query results that are inconsistent with the underlying data.' | 'UInt64' | +| 'query_result_cache_max_bytes' | '1048576' | '1048576' | 'None' | 'SESSION' | 'Sets the maximum byte size of cache for a single query result.' | 'UInt64' | +| 'query_result_cache_ttl_secs' | '300' | '300' | 'None' | 'SESSION' | 'Sets the time-to-live (TTL) in seconds for cached query results. Once the TTL for a cached result has expired, the result is considered stale and will not be used for new queries.' | 'UInt64' | +| 'quoted_ident_case_sensitive' | '1' | '1' | 'None' | 'SESSION' | 'Determines whether Databend treats quoted identifiers as case-sensitive.' | 'UInt64' | +| 'recluster_timeout_secs' | '43200' | '43200' | 'None' | 'SESSION' | 'Sets the seconds that recluster final will be timeout.' | 'UInt64' | +| 'replace_into_bloom_pruning_max_column_number' | '4' | '4' | 'None' | 'SESSION' | 'Max number of columns used by bloom pruning for replace-into statement.' | 'UInt64' | +| 'replace_into_shuffle_strategy' | '0' | '0' | 'None' | 'SESSION' | '0 for Block level shuffle, 1 for segment level shuffle' | 'UInt64' | +| 'retention_period' | '12' | '12' | 'None' | 'SESSION' | 'Sets the retention period in hours.' | 'UInt64' | +| 'sandbox_tenant' | '' | '' | 'None' | 'SESSION' | 'Injects a custom 'sandbox_tenant' into this session. This is only for testing purposes and will take effect only when 'internal_enable_sandbox_tenant' is turned on.' | 'String' | +| 'sort_spilling_bytes_threshold_per_proc' | '0' | '0' | 'None' | 'SESSION' | 'Sets the maximum amount of memory in bytes that a sorter can use before spilling data to storage during query execution.' | 'UInt64' | +| 'sort_spilling_memory_ratio' | '0' | '0' | 'None' | 'SESSION' | 'Sets the maximum memory ratio in bytes that a sorter can use before spilling data to storage during query execution.' | 'UInt64' | +| 'sql_dialect' | 'PostgreSQL' | 'PostgreSQL' | '["PostgreSQL", "MySQL", "Experimental", "Hive"]' | 'SESSION' | 'Sets the SQL dialect. Available values include "PostgreSQL", "MySQL", "Experimental", and "Hive".' | 'String' | +| 'storage_fetch_part_num' | '2' | '2' | 'None' | 'SESSION' | 'Sets the number of partitions that are fetched in parallel from storage during query execution.' | 'UInt64' | +| 'storage_io_max_page_bytes_for_read' | '524288' | '524288' | 'None' | 'SESSION' | 'Sets the maximum byte size of data pages that can be read from storage in a single I/O operation.' | 'UInt64' | +| 'storage_io_min_bytes_for_seek' | '48' | '48' | 'None' | 'SESSION' | 'Sets the minimum byte size of data that must be read from storage in a single I/O operation when seeking a new location in the data file.' | 'UInt64' | +| 'storage_read_buffer_size' | '1048576' | '1048576' | 'None' | 'SESSION' | 'Sets the byte size of the buffer used for reading data into memory.' | 'UInt64' | +| 'table_lock_expire_secs' | '10' | '10' | 'None' | 'SESSION' | 'Sets the seconds that the table lock will expire in.' | 'UInt64' | +| 'timezone' | 'UTC' | 'UTC' | 'None' | 'SESSION' | 'Sets the timezone.' | 'String' | +| 'unquoted_ident_case_sensitive' | '0' | '0' | 'None' | 'SESSION' | 'Determines whether Databend treats unquoted identifiers as case-sensitive.' | 'UInt64' | +| 'use_parquet2' | '0' | '0' | '[0, 1]' | 'SESSION' | 'Use parquet2 instead of parquet_rs when infer_schema().' | 'UInt64' | ++------------------------------------------------+----------------+----------------+---------------------------------------------------+-----------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------+ diff --git a/src/query/settings/src/lib.rs b/src/query/settings/src/lib.rs index 73604fd7cdf09..a5ee1beca0a8a 100644 --- a/src/query/settings/src/lib.rs +++ b/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; diff --git a/src/query/settings/src/settings.rs b/src/query/settings/src/settings.rs index afa8b9a4fc154..41a13ec6a7e8c 100644 --- a/src/query/settings/src/settings.rs +++ b/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,6 +116,7 @@ pub struct SettingsItem { pub desc: &'static str, pub user_value: UserSettingValue, pub default_value: UserSettingValue, + pub range: Option, } pub struct SettingsIter<'a> { @@ -155,6 +157,7 @@ impl<'a> Iterator for SettingsIter<'a> { desc: default_value.desc, user_value: default_value.value.clone(), default_value: default_value.value, + range: default_value.range, }, Some(change_value) => SettingsItem { name: key, @@ -162,6 +165,7 @@ impl<'a> Iterator for SettingsIter<'a> { desc: default_value.desc, user_value: change_value.value.clone(), default_value: default_value.value, + range: default_value.range, }, }), }; diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index 4a4f0390ecb0c..26341775d617e 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -13,6 +13,7 @@ // limitations under the License. use std::collections::HashMap; +use std::fmt::Display; use std::ops::RangeInclusive; use std::sync::Arc; @@ -43,6 +44,15 @@ pub enum SettingRange { String(Vec<&'static str>), } +impl Display for SettingRange { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SettingRange::Numeric(range) => write!(f, "[{}, {}]", range.start(), range.end()), + SettingRange::String(values) => write!(f, "{:?}", values), + } + } +} + impl SettingRange { /// Checks if an integer value is within the numeric range. pub fn is_within_numeric_range(&self, value: u64) -> Result<()> { @@ -51,11 +61,9 @@ impl SettingRange { if range.contains(&value) { Ok(()) } else { - Err(ErrorCode::BadArguments(format!( - "Value {} is not within the range [{}, {}]", - value, - range.start(), - range.end() + Err(ErrorCode::WrongValueForVariable(format!( + "Value {} is not within the range {}", + value, self ))) } } @@ -71,9 +79,9 @@ impl SettingRange { SettingRange::String(values) => { match values.iter().find(|&s| s.eq_ignore_ascii_case(value)) { Some(s) => Ok(s.to_string()), - None => Err(ErrorCode::BadArguments(format!( - "Value {} is not within the allowed values {:?}", - value, values + None => Err(ErrorCode::WrongValueForVariable(format!( + "Value {} is not within the allowed values {:}", + value, self ))), } } @@ -697,10 +705,7 @@ impl DefaultSettings { match setting_value.value { // Numeric value. UserSettingValue::UInt64(_) => { - let u64_val = v.parse::().map_err(|_| { - ErrorCode::BadArguments(format!("{} is not a valid integer value", v)) - })?; - + let u64_val = Self::parse_to_u64(&v)?; Ok((k, UserSettingValue::UInt64(u64_val))) } // String value. @@ -711,9 +716,7 @@ impl DefaultSettings { match range { // Numeric range. SettingRange::Numeric(_) => { - let u64_val = v.parse::().map_err(|_| { - ErrorCode::BadArguments(format!("{} is not a valid integer value", v)) - })?; + let u64_val = Self::parse_to_u64(&v)?; range.is_within_numeric_range(u64_val)?; Ok((k, UserSettingValue::UInt64(u64_val))) @@ -730,6 +733,26 @@ impl DefaultSettings { } } + /// Parses a string value to u64. + /// If the value is not a valid u64, it will be parsed as f64. + /// Used for: + /// set max_memory_usage = 1024*1024*1024*1.5; + fn parse_to_u64(v: &str) -> Result { + match v.parse::() { + Ok(val) => Ok(val), // If it's a valid u64, use it + Err(_) => { + // If not a valid u64, try parsing as f64 + match v.parse::() { + Ok(f) if f.fract() == 0.0 && f >= 0.0 => Ok(f.trunc() as u64), /* Convert to u64 if no fractional part and non-negative */ + _ => Err(ErrorCode::WrongValueForVariable(format!( + "{} is not a valid integer value", + v + ))), + } + } + } + } + pub fn try_get_u64(key: &str) -> Result { match DefaultSettings::instance()?.settings.get(key) { Some(v) => v.value.as_u64(), diff --git a/src/query/settings/src/settings_getter_setter.rs b/src/query/settings/src/settings_getter_setter.rs index 24564bb48010b..fb793525c2fde 100644 --- a/src/query/settings/src/settings_getter_setter.rs +++ b/src/query/settings/src/settings_getter_setter.rs @@ -81,7 +81,7 @@ impl Settings { 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::BadArguments(format!("{}: {}", key, err.message())) + ErrorCode::WrongValueForVariable(format!("{}: {}", key, err.message())) })?; } diff --git a/src/query/settings/tests/it/setting.rs b/src/query/settings/tests/it/setting.rs index ed6e44ee01538..0143e7c5d2ac7 100644 --- a/src/query/settings/tests/it/setting.rs +++ b/src/query/settings/tests/it/setting.rs @@ -22,7 +22,7 @@ fn test_set_settings() { settings.set_max_threads(2).unwrap(); let result = settings.set_max_threads(1025); - let expect = "BadArguments. Code: 1006, Text = max_threads: Value 1025 is not within the range [1, 1024]."; + let expect = "WrongValueForVariable. Code: 2803, Text = max_threads: Value 1025 is not within the range [1, 1024]."; assert_eq!(expect, format!("{}", result.unwrap_err())); } @@ -37,19 +37,25 @@ fn test_set_settings() { .set_setting("enable_table_lock".to_string(), "0".to_string()) .unwrap(); - // Error - let result = settings.set_setting("enable_table_lock".to_string(), "1.0".to_string()); - let expect = "BadArguments. Code: 1006, Text = 1.0 is not a valid integer value."; - assert_eq!(expect, format!("{}", result.unwrap_err())); + // 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 = "BadArguments. Code: 1006, Text = Value 3 is not within the range [0, 1]."; + 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 = "BadArguments. Code: 1006, Text = xx is not a valid integer value."; + let expect = "WrongValueForVariable. Code: 2803, Text = xx is not a valid integer value."; assert_eq!(expect, format!("{}", result.unwrap_err())); } @@ -67,7 +73,7 @@ fn test_set_settings() { // Error let result = settings.set_setting("query_flight_compression".to_string(), "xx".to_string()); - let expect = "BadArguments. Code: 1006, Text = Value xx is not within the allowed values [\"None\", \"LZ4\", \"ZSTD\"]."; + let expect = "WrongValueForVariable. Code: 2803, Text = Value xx is not within the allowed values [\"None\", \"LZ4\", \"ZSTD\"]."; assert_eq!(expect, format!("{}", result.unwrap_err())); } diff --git a/src/query/storages/system/src/settings_table.rs b/src/query/storages/system/src/settings_table.rs index 94eea62566100..f70bc41e3750a 100644 --- a/src/query/storages/system/src/settings_table.rs +++ b/src/query/storages/system/src/settings_table.rs @@ -49,18 +49,29 @@ impl SyncSystemTable for SettingsTable { let mut names: Vec = vec![]; let mut values: Vec = vec![]; let mut defaults: Vec = vec![]; + let mut ranges: Vec = vec![]; let mut levels: Vec = vec![]; let mut descs: Vec = vec![]; let mut types: Vec = 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> = names.iter().map(|x| x.as_bytes().to_vec()).collect(); let values: Vec> = values.iter().map(|x| x.as_bytes().to_vec()).collect(); let defaults: Vec> = defaults.iter().map(|x| x.as_bytes().to_vec()).collect(); + let ranges: Vec> = ranges.iter().map(|x| x.as_bytes().to_vec()).collect(); let levels: Vec> = levels.iter().map(|x| x.as_bytes().to_vec()).collect(); let descs: Vec> = descs.iter().map(|x| x.as_bytes().to_vec()).collect(); let types: Vec> = 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), From e1981870060a9f09ab985523a5c0a64289813e6e Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 17:57:18 +0800 Subject: [PATCH 09/11] fix sql logic test for the settings --- .../suites/base/06_show/06_0003_show_settings.test | 2 +- tests/sqllogictests/suites/base/20+_others/20_0001_planner.test | 1 + tests/suites/0_stateless/05_hints/05_0001_set_var.result | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test b/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test index a600344b51b88..f3024f1e0289f 100644 --- a/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test +++ b/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test @@ -49,7 +49,7 @@ statement ok set max_memory_usage = 1024*1024*1024*1.5 onlyif mysql -statement error 1006 +statement error 1105 set max_memory_usage = 1024*1024*1024*1.3 onlyif mysql diff --git a/tests/sqllogictests/suites/base/20+_others/20_0001_planner.test b/tests/sqllogictests/suites/base/20+_others/20_0001_planner.test index a0732f1c3a953..719a0ff98ffbc 100644 --- a/tests/sqllogictests/suites/base/20+_others/20_0001_planner.test +++ b/tests/sqllogictests/suites/base/20+_others/20_0001_planner.test @@ -1287,6 +1287,7 @@ system settings default String system settings description String system settings level String system settings name String +system settings range String system settings type String system settings value String diff --git a/tests/suites/0_stateless/05_hints/05_0001_set_var.result b/tests/suites/0_stateless/05_hints/05_0001_set_var.result index 8c9f0b3a99487..ca2e0ff47bd28 100644 --- a/tests/suites/0_stateless/05_hints/05_0001_set_var.result +++ b/tests/suites/0_stateless/05_hints/05_0001_set_var.result @@ -2,7 +2,7 @@ x x timezone America/Los_Angeles x x timezone Asia/Shanghai -timezone Asia/Shanghai UTC SESSION Sets the timezone. String +timezone Asia/Shanghai UTC None SESSION Sets the timezone. String storage_read_buffer_size 1048576 timezone UTC storage_read_buffer_size 200 From 596077a769d5a77137e3497b06726b45b58eb498 Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 18:27:37 +0800 Subject: [PATCH 10/11] fix show settings test --- .../suites/base/06_show/06_0003_show_settings.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test b/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test index f3024f1e0289f..bdc3878c2879c 100644 --- a/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test +++ b/tests/sqllogictests/suites/base/06_show/06_0003_show_settings.test @@ -53,7 +53,7 @@ statement error 1105 set max_memory_usage = 1024*1024*1024*1.3 onlyif mysql -statement error 1001 +statement error 1105 set max_memory_usage = true onlyif mysql From 461009cbedea89912dcc022d368537616d3b328e Mon Sep 17 00:00:00 2001 From: BohuTANG Date: Thu, 14 Dec 2023 19:17:10 +0800 Subject: [PATCH 11/11] add settings u64 max check --- src/query/settings/src/settings_default.rs | 8 ++- src/query/settings/tests/it/setting.rs | 69 ++++++++++++++-------- 2 files changed, 49 insertions(+), 28 deletions(-) diff --git a/src/query/settings/src/settings_default.rs b/src/query/settings/src/settings_default.rs index 26341775d617e..2a0d50dd74afe 100644 --- a/src/query/settings/src/settings_default.rs +++ b/src/query/settings/src/settings_default.rs @@ -737,13 +737,15 @@ impl DefaultSettings { /// If the value is not a valid u64, it will be parsed as f64. /// Used for: /// set max_memory_usage = 1024*1024*1024*1.5; - fn parse_to_u64(v: &str) -> Result { + fn parse_to_u64(v: &str) -> Result { match v.parse::() { - Ok(val) => Ok(val), // If it's a valid u64, use it + Ok(val) => Ok(val), Err(_) => { // If not a valid u64, try parsing as f64 match v.parse::() { - Ok(f) if f.fract() == 0.0 && f >= 0.0 => Ok(f.trunc() as u64), /* Convert to u64 if no fractional part and non-negative */ + Ok(f) if f.fract() == 0.0 && f >= 0.0 && f <= u64::MAX as f64 => { + Ok(f.trunc() as u64) /* Convert to u64 if no fractional part, non-negative, and within u64 range */ + } _ => Err(ErrorCode::WrongValueForVariable(format!( "{} is not a valid integer value", v diff --git a/src/query/settings/tests/it/setting.rs b/src/query/settings/tests/it/setting.rs index 0143e7c5d2ac7..c4235603edf4e 100644 --- a/src/query/settings/tests/it/setting.rs +++ b/src/query/settings/tests/it/setting.rs @@ -28,35 +28,54 @@ fn test_set_settings() { // 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("max_memory_usage".to_string(), "1610612736.0".to_string()) - .unwrap(); + // Range than u64. + let result = settings.set_setting( + "max_memory_usage".to_string(), + "161061273600000000000000000000000000000000000000000000000".to_string(), + ); + let expect = "WrongValueForVariable. Code: 2803, Text = 161061273600000000000000000000000000000000000000000000000 is not a valid integer value."; + assert_eq!(expect, format!("{}", result.unwrap_err())); - // Ok with float. - settings - .set_setting("enable_table_lock".to_string(), "1.0".to_string()) - .unwrap(); + // Range with neg. + let result = settings.set_setting("max_memory_usage".to_string(), "-1".to_string()); + let expect = + "WrongValueForVariable. Code: 2803, Text = -1 is not a valid integer value."; + assert_eq!(expect, format!("{}", result.unwrap_err())); + } - // 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())); + { + // 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(); - // 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())); + // 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.