Minor: more flexible pool size setting for datafusion-cli #7454
Conversation
| "-100", | ||
| Err("Negative memory pool size value is not allowed '-100'".to_string()), | ||
| ); | ||
|
|
There was a problem hiding this comment.
| assert_conversion( | |
| "12k12k", | |
| Err("Invalid memory pool size '12k12k'".to_string()), | |
| ); |
There was a problem hiding this comment.
This case should have failed, but it passed.
There was a problem hiding this comment.
I'm not sure if I understand you correctly. Do you mean the case "-100" shouldn't fail with the Err "Negative memory pool size value is not allowed '-100'"?
There was a problem hiding this comment.
No, we should add a test for cases like 12k12k .
There was a problem hiding this comment.
Nice catch! Fixed in the latest commit.
| #[derive(Debug, Clone, Copy)] | ||
| enum ByteUnit { | ||
| Byte, | ||
| KiB, | ||
| MiB, | ||
| GiB, | ||
| TiB, | ||
| } | ||
|
|
||
| impl ByteUnit { | ||
| fn multiplier(&self) -> usize { | ||
| match self { | ||
| ByteUnit::Byte => 1, | ||
| ByteUnit::KiB => 1 << 10, | ||
| ByteUnit::MiB => 1 << 20, | ||
| ByteUnit::GiB => 1 << 30, | ||
| ByteUnit::TiB => 1 << 40, | ||
| } | ||
| } | ||
| } |
datafusion-cli/src/main.rs
Outdated
| if num_str.starts_with('-') { | ||
| return Err(format!( | ||
| "Negative memory pool size value is not allowed '{}'", | ||
| size | ||
| )); | ||
| } | ||
|
|
||
| let num = num_str.parse::<usize>().map_err(|_| { | ||
| format!("Invalid numeric value in memory pool size '{}'", size) | ||
| })?; | ||
|
|
There was a problem hiding this comment.
Can we simplify this? If num is of type usize, then it is guaranteed to be a positive value.
There was a problem hiding this comment.
We need to guard against values larger than usize::MAX?
There was a problem hiding this comment.
| if num_str.starts_with('-') { | |
| return Err(format!( | |
| "Negative memory pool size value is not allowed '{}'", | |
| size | |
| )); | |
| } | |
| let num = num_str.parse::<usize>().map_err(|_| { | |
| format!("Invalid numeric value in memory pool size '{}'", size) | |
| })?; | |
| let num = num_str.parse::<usize>().map_err(|_| { | |
| format!("Negative memory pool size value is not allowed '{}'", size) | |
| })?; |
Sorry for the confusion, could we test for negative numbers like this?
There was a problem hiding this comment.
I have no strong opinion on this.
I changed the code above to parse numeric directly to usize for simplicity as suggested, but the caveat is the new error message is less informative, which I suppose should be fine in this case.
3 participants
Which issue does this PR close?
re #7424.
Rationale for this change
Find the memory pool size setting string in datafusion-cli during performance testing, and we could make the byte string more flexible.
What changes are included in this PR?
Parse the pool byte string as the number of bytes.
Are these changes tested?
Add unit test and tested locally.
Are there any user-facing changes?
No.