martin-g commented on code in PR #20372:
URL: https://github.com/apache/datafusion/pull/20372#discussion_r2844825969
##########
benchmarks/src/util/options.rs:
##########
@@ -116,20 +116,20 @@ impl CommonOpt {
}
}
-/// Parse memory limit from string to number of bytes
-/// e.g. '1.5G', '100M' -> 1572864
-fn parse_memory_limit(limit: &str) -> Result<usize, String> {
+/// Parse capacity limit from string to number of bytes by allowing units: K,
M and G.
+/// Supports formats like '1.5G', '100M' -> 1572864
Review Comment:
Do you have an idea what `1572864` is supposed to be here ?
It is neither 1.5G nor 100M. It is 1.5M.
##########
benchmarks/src/util/options.rs:
##########
@@ -116,20 +116,20 @@ impl CommonOpt {
}
}
-/// Parse memory limit from string to number of bytes
-/// e.g. '1.5G', '100M' -> 1572864
-fn parse_memory_limit(limit: &str) -> Result<usize, String> {
+/// Parse capacity limit from string to number of bytes by allowing units: K,
M and G.
+/// Supports formats like '1.5G', '100M' -> 1572864
+fn parse_capacity_limit(limit: &str) -> Result<usize, String> {
let (number, unit) = limit.split_at(limit.len() - 1);
let number: f64 = number
.parse()
- .map_err(|_| format!("Failed to parse number from memory limit
'{limit}'"))?;
+ .map_err(|_| format!("Failed to parse number from capacity limit
'{limit}'"))?;
Review Comment:
Add validation for NaN, Inf and negative numbers.
##########
benchmarks/src/util/options.rs:
##########
@@ -116,20 +116,20 @@ impl CommonOpt {
}
}
-/// Parse memory limit from string to number of bytes
-/// e.g. '1.5G', '100M' -> 1572864
-fn parse_memory_limit(limit: &str) -> Result<usize, String> {
+/// Parse capacity limit from string to number of bytes by allowing units: K,
M and G.
+/// Supports formats like '1.5G', '100M' -> 1572864
+fn parse_capacity_limit(limit: &str) -> Result<usize, String> {
let (number, unit) = limit.split_at(limit.len() - 1);
Review Comment:
```suggestion
if limit.is_empty() {
return Err("Capacity limit cannot be empty".to_string());
}
let (number, unit) = limit.split_at(limit.len() - 1);
```
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1236,33 +1236,38 @@ impl SessionContext {
Ok(())
}
- /// Parse memory limit from string to number of bytes
+ /// Parse capacity limit from string to number of bytes by allowing units:
K, M and G.
/// Supports formats like '1.5G', '100M', '512K'
///
/// # Examples
/// ```
/// use datafusion::execution::context::SessionContext;
///
/// assert_eq!(
- /// SessionContext::parse_memory_limit("1M").unwrap(),
+ ///
SessionContext::parse_capacity_limit("datafusion.runtime.memory_limit",
"1M").unwrap(),
/// 1024 * 1024
/// );
/// assert_eq!(
- /// SessionContext::parse_memory_limit("1.5G").unwrap(),
+ ///
SessionContext::parse_capacity_limit("datafusion.runtime.memory_limit",
"1.5G").unwrap(),
/// (1.5 * 1024.0 * 1024.0 * 1024.0) as usize
/// );
/// ```
- pub fn parse_memory_limit(limit: &str) -> Result<usize> {
Review Comment:
Maybe keep the old method around as deprecated for a few releases ?!
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1236,33 +1236,38 @@ impl SessionContext {
Ok(())
}
- /// Parse memory limit from string to number of bytes
+ /// Parse capacity limit from string to number of bytes by allowing units:
K, M and G.
/// Supports formats like '1.5G', '100M', '512K'
///
/// # Examples
/// ```
/// use datafusion::execution::context::SessionContext;
///
/// assert_eq!(
- /// SessionContext::parse_memory_limit("1M").unwrap(),
+ ///
SessionContext::parse_capacity_limit("datafusion.runtime.memory_limit",
"1M").unwrap(),
/// 1024 * 1024
/// );
/// assert_eq!(
- /// SessionContext::parse_memory_limit("1.5G").unwrap(),
+ ///
SessionContext::parse_capacity_limit("datafusion.runtime.memory_limit",
"1.5G").unwrap(),
/// (1.5 * 1024.0 * 1024.0 * 1024.0) as usize
/// );
/// ```
- pub fn parse_memory_limit(limit: &str) -> Result<usize> {
+ pub fn parse_capacity_limit(config_name: &str, limit: &str) ->
Result<usize> {
let (number, unit) = limit.split_at(limit.len() - 1);
let number: f64 = number.parse().map_err(|_| {
Review Comment:
This allows parsing values like `NaN`, `Inf` and negative ones. IMO it would
be good to add validation before the `match` below.
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1236,33 +1236,38 @@ impl SessionContext {
Ok(())
}
- /// Parse memory limit from string to number of bytes
+ /// Parse capacity limit from string to number of bytes by allowing units:
K, M and G.
/// Supports formats like '1.5G', '100M', '512K'
///
/// # Examples
/// ```
/// use datafusion::execution::context::SessionContext;
///
/// assert_eq!(
- /// SessionContext::parse_memory_limit("1M").unwrap(),
+ ///
SessionContext::parse_capacity_limit("datafusion.runtime.memory_limit",
"1M").unwrap(),
/// 1024 * 1024
/// );
/// assert_eq!(
- /// SessionContext::parse_memory_limit("1.5G").unwrap(),
+ ///
SessionContext::parse_capacity_limit("datafusion.runtime.memory_limit",
"1.5G").unwrap(),
/// (1.5 * 1024.0 * 1024.0 * 1024.0) as usize
/// );
/// ```
- pub fn parse_memory_limit(limit: &str) -> Result<usize> {
+ pub fn parse_capacity_limit(config_name: &str, limit: &str) ->
Result<usize> {
let (number, unit) = limit.split_at(limit.len() - 1);
Review Comment:
This would panic on empty `limit`. Let's improve it and return an error
instead.
```suggestion
if limit.is_empty() {
return plan_datafusion_err!("Empty limit value for
'{config_name}'");
}
let (number, unit) = limit.split_at(limit.len() - 1);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
