martin-g commented on code in PR #20816:
URL: https://github.com/apache/datafusion/pull/20816#discussion_r2911492815
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1323,21 +1323,27 @@ impl SessionContext {
}
}
- fn parse_duration(duration: &str) -> Result<Duration> {
+ fn parse_duration(config_name: &str, duration: &str) -> Result<Duration> {
+ if duration.trim().is_empty() {
+ return Err(plan_datafusion_err!(
+ "Duration should not be empty or blank for '{config_name}'"
+ ));
+ }
+
let mut minutes = None;
let mut seconds = None;
for duration in duration.split_inclusive(&['m', 's']) {
let (number, unit) = duration.split_at(duration.len() - 1);
let number: u64 = number.parse().map_err(|_| {
- plan_datafusion_err!("Failed to parse number from duration
'{duration}'")
+ plan_datafusion_err!("Failed to parse number from duration
'{duration}' for '{config_name}'")
})?;
match unit {
"m" if minutes.is_none() && seconds.is_none() => minutes =
Some(number),
"s" if seconds.is_none() => seconds = Some(number),
_ => plan_err!(
- "Invalid duration, unit must be either 'm' (minutes), or
's' (seconds), and be in the correct order"
+ "Invalid duration, unit must be either 'm' (minutes), or
's' (seconds), and be in the correct order for '{config_name}'"
Review Comment:
```suggestion
"Invalid duration unit: '{other}'. The unit must be
either 'm' (minutes), or 's' (seconds), and be in the correct order for
'{config_name}'"
```
##########
datafusion/sqllogictest/test_files/set_variable.slt:
##########
@@ -450,3 +450,9 @@ datafusion.runtime.temp_directory
statement error DataFusion error: Error during planning: Unsupported value Null
SET datafusion.runtime.memory_limit = NULL
+
+statement error DataFusion error: Error during planning: Unsupported value Null
+SET datafusion.runtime.list_files_cache_ttl = NULL
+
+statement error DataFusion error: Error during planning: Duration should not
be empty or blank for 'datafusion.runtime.list_files_cache_ttl'
+SET datafusion.runtime.list_files_cache_ttl = ' '
Review Comment:
please also add a test for blank, i.e. `''`
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1323,21 +1323,27 @@ impl SessionContext {
}
}
- fn parse_duration(duration: &str) -> Result<Duration> {
+ fn parse_duration(config_name: &str, duration: &str) -> Result<Duration> {
+ if duration.trim().is_empty() {
+ return Err(plan_datafusion_err!(
+ "Duration should not be empty or blank for '{config_name}'"
+ ));
+ }
+
let mut minutes = None;
let mut seconds = None;
for duration in duration.split_inclusive(&['m', 's']) {
let (number, unit) = duration.split_at(duration.len() - 1);
let number: u64 = number.parse().map_err(|_| {
- plan_datafusion_err!("Failed to parse number from duration
'{duration}'")
+ plan_datafusion_err!("Failed to parse number from duration
'{duration}' for '{config_name}'")
})?;
match unit {
"m" if minutes.is_none() && seconds.is_none() => minutes =
Some(number),
"s" if seconds.is_none() => seconds = Some(number),
_ => plan_err!(
Review Comment:
```suggestion
other => plan_err!(
```
--
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]
