Skip to content

Commit fd993dc

Browse files
BohuTANGBohuTANG
committed
add f64 to settings and change the settings range check error to WrongValueForVariable
1 parent 32277b6 commit fd993dc

File tree

8 files changed

+153
-104
lines changed

8 files changed

+153
-104
lines changed

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

+1
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,7 @@ DB.Table: 'system'.'columns', Table: columns-table_id:1, ver:0, Engine: SystemCo
279279
| 'query_kind' | 'system' | 'query_log' | 'String' | 'VARCHAR' | '' | '' | 'NO' | '' |
280280
| 'query_start_time' | 'system' | 'query_log' | 'Timestamp' | 'TIMESTAMP' | '' | '' | 'NO' | '' |
281281
| 'query_text' | 'system' | 'query_log' | 'String' | 'VARCHAR' | '' | '' | 'NO' | '' |
282+
| 'range' | 'system' | 'settings' | 'String' | 'VARCHAR' | '' | '' | 'NO' | '' |
282283
| 'referenced_column_name' | 'information_schema' | 'key_column_usage' | 'NULL' | 'NULL' | '' | '' | 'NO' | '' |
283284
| 'referenced_table_name' | 'information_schema' | 'key_column_usage' | 'NULL' | 'NULL' | '' | '' | 'NO' | '' |
284285
| 'referenced_table_schema' | 'information_schema' | 'key_column_usage' | 'NULL' | 'NULL' | '' | '' | 'NO' | '' |

‎src/query/service/tests/it/storages/testdata/settings_table.txt

+80-80
Large diffs are not rendered by default.

‎src/query/settings/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,5 @@ pub use settings::ScopeLevel;
2222
pub use settings::Settings;
2323
pub use settings_default::ReplaceIntoShuffleStrategy;
2424
pub use settings_default::SettingMode;
25+
pub use settings_default::SettingRange;
2526
pub use settings_getter_setter::FlightCompression;

‎src/query/settings/src/settings.rs

+4
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ use itertools::Itertools;
2525

2626
use crate::settings_default::DefaultSettingValue;
2727
use crate::settings_default::DefaultSettings;
28+
use crate::settings_default::SettingRange;
2829
use crate::SettingMode;
2930

3031
#[derive(serde::Serialize, serde::Deserialize, Clone)]
@@ -115,6 +116,7 @@ pub struct SettingsItem {
115116
pub desc: &'static str,
116117
pub user_value: UserSettingValue,
117118
pub default_value: UserSettingValue,
119+
pub range: Option<SettingRange>,
118120
}
119121

120122
pub struct SettingsIter<'a> {
@@ -155,13 +157,15 @@ impl<'a> Iterator for SettingsIter<'a> {
155157
desc: default_value.desc,
156158
user_value: default_value.value.clone(),
157159
default_value: default_value.value,
160+
range: default_value.range,
158161
},
159162
Some(change_value) => SettingsItem {
160163
name: key,
161164
level: change_value.level.clone(),
162165
desc: default_value.desc,
163166
user_value: change_value.value.clone(),
164167
default_value: default_value.value,
168+
range: default_value.range,
165169
},
166170
}),
167171
};

‎src/query/settings/src/settings_default.rs

+38-15
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// limitations under the License.
1414

1515
use std::collections::HashMap;
16+
use std::fmt::Display;
1617
use std::ops::RangeInclusive;
1718
use std::sync::Arc;
1819

@@ -43,6 +44,15 @@ pub enum SettingRange {
4344
String(Vec<&'static str>),
4445
}
4546

47+
impl Display for SettingRange {
48+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
49+
match self {
50+
SettingRange::Numeric(range) => write!(f, "[{}, {}]", range.start(), range.end()),
51+
SettingRange::String(values) => write!(f, "{:?}", values),
52+
}
53+
}
54+
}
55+
4656
impl SettingRange {
4757
/// Checks if an integer value is within the numeric range.
4858
pub fn is_within_numeric_range(&self, value: u64) -> Result<()> {
@@ -51,11 +61,9 @@ impl SettingRange {
5161
if range.contains(&value) {
5262
Ok(())
5363
} else {
54-
Err(ErrorCode::BadArguments(format!(
55-
"Value {} is not within the range [{}, {}]",
56-
value,
57-
range.start(),
58-
range.end()
64+
Err(ErrorCode::WrongValueForVariable(format!(
65+
"Value {} is not within the range {}",
66+
value, self
5967
)))
6068
}
6169
}
@@ -71,9 +79,9 @@ impl SettingRange {
7179
SettingRange::String(values) => {
7280
match values.iter().find(|&s| s.eq_ignore_ascii_case(value)) {
7381
Some(s) => Ok(s.to_string()),
74-
None => Err(ErrorCode::BadArguments(format!(
75-
"Value {} is not within the allowed values {:?}",
76-
value, values
82+
None => Err(ErrorCode::WrongValueForVariable(format!(
83+
"Value {} is not within the allowed values {:}",
84+
value, self
7785
))),
7886
}
7987
}
@@ -697,10 +705,7 @@ impl DefaultSettings {
697705
match setting_value.value {
698706
// Numeric value.
699707
UserSettingValue::UInt64(_) => {
700-
let u64_val = v.parse::<u64>().map_err(|_| {
701-
ErrorCode::BadArguments(format!("{} is not a valid integer value", v))
702-
})?;
703-
708+
let u64_val = Self::parse_to_u64(&v)?;
704709
Ok((k, UserSettingValue::UInt64(u64_val)))
705710
}
706711
// String value.
@@ -711,9 +716,7 @@ impl DefaultSettings {
711716
match range {
712717
// Numeric range.
713718
SettingRange::Numeric(_) => {
714-
let u64_val = v.parse::<u64>().map_err(|_| {
715-
ErrorCode::BadArguments(format!("{} is not a valid integer value", v))
716-
})?;
719+
let u64_val = Self::parse_to_u64(&v)?;
717720
range.is_within_numeric_range(u64_val)?;
718721

719722
Ok((k, UserSettingValue::UInt64(u64_val)))
@@ -730,6 +733,26 @@ impl DefaultSettings {
730733
}
731734
}
732735

736+
/// Parses a string value to u64.
737+
/// If the value is not a valid u64, it will be parsed as f64.
738+
/// Used for:
739+
/// set max_memory_usage = 1024*1024*1024*1.5;
740+
fn parse_to_u64(v: &str) -> Result<u64> {
741+
match v.parse::<u64>() {
742+
Ok(val) => Ok(val), // If it's a valid u64, use it
743+
Err(_) => {
744+
// If not a valid u64, try parsing as f64
745+
match v.parse::<f64>() {
746+
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 */
747+
_ => Err(ErrorCode::WrongValueForVariable(format!(
748+
"{} is not a valid integer value",
749+
v
750+
))),
751+
}
752+
}
753+
}
754+
}
755+
733756
pub fn try_get_u64(key: &str) -> Result<u64> {
734757
match DefaultSettings::instance()?.settings.get(key) {
735758
Some(v) => v.value.as_u64(),

‎src/query/settings/src/settings_getter_setter.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ impl Settings {
8181
if let Some(range) = &setting_value.range {
8282
// Check if the value falls within the numeric range
8383
range.is_within_numeric_range(val).map_err(|err| {
84-
ErrorCode::BadArguments(format!("{}: {}", key, err.message()))
84+
ErrorCode::WrongValueForVariable(format!("{}: {}", key, err.message()))
8585
})?;
8686
}
8787

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

+14-8
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ fn test_set_settings() {
2222
settings.set_max_threads(2).unwrap();
2323

2424
let result = settings.set_max_threads(1025);
25-
let expect = "BadArguments. Code: 1006, Text = max_threads: Value 1025 is not within the range [1, 1024].";
25+
let expect = "WrongValueForVariable. Code: 2803, Text = max_threads: Value 1025 is not within the range [1, 1024].";
2626
assert_eq!(expect, format!("{}", result.unwrap_err()));
2727
}
2828

@@ -37,19 +37,25 @@ fn test_set_settings() {
3737
.set_setting("enable_table_lock".to_string(), "0".to_string())
3838
.unwrap();
3939

40-
// Error
41-
let result = settings.set_setting("enable_table_lock".to_string(), "1.0".to_string());
42-
let expect = "BadArguments. Code: 1006, Text = 1.0 is not a valid integer value.";
43-
assert_eq!(expect, format!("{}", result.unwrap_err()));
40+
// Ok with float.
41+
settings
42+
.set_setting("max_memory_usage".to_string(), "1610612736.0".to_string())
43+
.unwrap();
44+
45+
// Ok with float.
46+
settings
47+
.set_setting("enable_table_lock".to_string(), "1.0".to_string())
48+
.unwrap();
4449

4550
// Error
4651
let result = settings.set_setting("enable_table_lock".to_string(), "3".to_string());
47-
let expect = "BadArguments. Code: 1006, Text = Value 3 is not within the range [0, 1].";
52+
let expect =
53+
"WrongValueForVariable. Code: 2803, Text = Value 3 is not within the range [0, 1].";
4854
assert_eq!(expect, format!("{}", result.unwrap_err()));
4955

5056
// Error
5157
let result = settings.set_setting("enable_table_lock".to_string(), "xx".to_string());
52-
let expect = "BadArguments. Code: 1006, Text = xx is not a valid integer value.";
58+
let expect = "WrongValueForVariable. Code: 2803, Text = xx is not a valid integer value.";
5359
assert_eq!(expect, format!("{}", result.unwrap_err()));
5460
}
5561

@@ -67,7 +73,7 @@ fn test_set_settings() {
6773

6874
// Error
6975
let result = settings.set_setting("query_flight_compression".to_string(), "xx".to_string());
70-
let expect = "BadArguments. Code: 1006, Text = Value xx is not within the allowed values [\"None\", \"LZ4\", \"ZSTD\"].";
76+
let expect = "WrongValueForVariable. Code: 2803, Text = Value xx is not within the allowed values [\"None\", \"LZ4\", \"ZSTD\"].";
7177
assert_eq!(expect, format!("{}", result.unwrap_err()));
7278
}
7379

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

+14
Original file line numberDiff line numberDiff line change
@@ -49,18 +49,29 @@ impl SyncSystemTable for SettingsTable {
4949
let mut names: Vec<String> = vec![];
5050
let mut values: Vec<String> = vec![];
5151
let mut defaults: Vec<String> = vec![];
52+
let mut ranges: Vec<String> = vec![];
5253
let mut levels: Vec<String> = vec![];
5354
let mut descs: Vec<String> = vec![];
5455
let mut types: Vec<String> = vec![];
5556
for item in settings.into_iter() {
5657
// Name.
5758
names.push(item.name);
59+
5860
// Value.
5961
values.push(escape(format!("{:?}", item.user_value).as_str()).to_string());
62+
6063
// Default Value.
6164
defaults.push(escape(format!("{:?}", item.default_value).as_str()).to_string());
65+
66+
// Range Value.
67+
match item.range {
68+
Some(range) => ranges.push(format!("{}", range)),
69+
None => ranges.push("None".to_string()),
70+
}
71+
6272
// Scope level.
6373
levels.push(format!("{:?}", item.level));
74+
6475
// Desc.
6576
descs.push(item.desc.to_string());
6677

@@ -75,6 +86,7 @@ impl SyncSystemTable for SettingsTable {
7586
let names: Vec<Vec<u8>> = names.iter().map(|x| x.as_bytes().to_vec()).collect();
7687
let values: Vec<Vec<u8>> = values.iter().map(|x| x.as_bytes().to_vec()).collect();
7788
let defaults: Vec<Vec<u8>> = defaults.iter().map(|x| x.as_bytes().to_vec()).collect();
89+
let ranges: Vec<Vec<u8>> = ranges.iter().map(|x| x.as_bytes().to_vec()).collect();
7890
let levels: Vec<Vec<u8>> = levels.iter().map(|x| x.as_bytes().to_vec()).collect();
7991
let descs: Vec<Vec<u8>> = descs.iter().map(|x| x.as_bytes().to_vec()).collect();
8092
let types: Vec<Vec<u8>> = types.iter().map(|x| x.as_bytes().to_vec()).collect();
@@ -83,6 +95,7 @@ impl SyncSystemTable for SettingsTable {
8395
StringType::from_data(names),
8496
StringType::from_data(values),
8597
StringType::from_data(defaults),
98+
StringType::from_data(ranges),
8699
StringType::from_data(levels),
87100
StringType::from_data(descs),
88101
StringType::from_data(types),
@@ -96,6 +109,7 @@ impl SettingsTable {
96109
TableField::new("name", TableDataType::String),
97110
TableField::new("value", TableDataType::String),
98111
TableField::new("default", TableDataType::String),
112+
TableField::new("range", TableDataType::String),
99113
TableField::new("level", TableDataType::String),
100114
TableField::new("description", TableDataType::String),
101115
TableField::new("type", TableDataType::String),

0 commit comments

Comments
 (0)