From f8492c85ae56f711b96a4f2d6784ac31555192be Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 2 Aug 2024 15:51:55 +0300 Subject: [PATCH 1/5] core/datetime: Remove trace calls We should trace in high-level code like VDBE interpreter loop, but not in error handling path of specific SQL functions. --- core/datetime.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/core/datetime.rs b/core/datetime.rs index a3e7c7636..c533d3257 100644 --- a/core/datetime.rs +++ b/core/datetime.rs @@ -1,6 +1,5 @@ use crate::types::OwnedValue; use chrono::{DateTime, NaiveDate, NaiveDateTime, NaiveTime, Timelike, Utc}; -use log::trace; use std::result::Result; use std::{error::Error, fmt::Display}; @@ -93,11 +92,9 @@ pub fn get_date_from_time_value(time_value: &OwnedValue) -> crate::Result { - trace!("Invalid time value: {}", time_value); Ok(String::new()) } DateTimeError::Other(s) => { - trace!("Other date time error: {}", s); Err(crate::error::LimboError::InvalidDate(s)) } } @@ -111,11 +108,9 @@ pub fn get_time_from_datetime_value(time_value: &OwnedValue) -> crate::Result { - trace!("Invalid time value: {}", time_value); Ok(String::new()) } DateTimeError::Other(s) => { - trace!("Other date time error: {}", s); Err(crate::error::LimboError::InvalidTime(s)) } } From 763bf17c9ebfc4b00d4d38da0f36ebf3ce66a6a3 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 2 Aug 2024 15:54:32 +0300 Subject: [PATCH 2/5] core/datetime: Use "cfg(test)" annotation for tests --- core/datetime.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/core/datetime.rs b/core/datetime.rs index c533d3257..938799269 100644 --- a/core/datetime.rs +++ b/core/datetime.rs @@ -259,6 +259,7 @@ fn get_time_from_naive_datetime(value: NaiveDateTime) -> String { value.format("%H:%M:%S").to_string() } +#[cfg(test)] mod tests { use super::*; use std::rc::Rc; From 3412b65b8a900af93b9bf2b7fe5ce049cb38d137 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 2 Aug 2024 15:57:54 +0300 Subject: [PATCH 3/5] core/datetime: Remove TimeUnit ...it's not used for anything much. --- core/datetime.rs | 59 +----------------------------------------------- 1 file changed, 1 insertion(+), 58 deletions(-) diff --git a/core/datetime.rs b/core/datetime.rs index 938799269..7f31c0dab 100644 --- a/core/datetime.rs +++ b/core/datetime.rs @@ -20,63 +20,6 @@ impl Display for DateTimeError { impl Error for DateTimeError {} -#[derive(Debug, Clone, Copy)] -pub enum TimeUnit { - Second, - Minute, - Hour, - Day, - Month, - Year, -} - -/* -** The following table defines various date transformations of the form -** -** 'NNN days' -** -** Where NNN is an arbitrary floating-point number and "days" can be one -** of several units of time. -*/ -impl TimeUnit { - pub fn name(&self) -> &'static str { - match self { - TimeUnit::Second => "second", - TimeUnit::Minute => "minute", - TimeUnit::Hour => "hour", - TimeUnit::Day => "day", - TimeUnit::Month => "month", - TimeUnit::Year => "year", - } - } - - // Maximum value for each unit in Julian calendar - // Each corresponds to ~14713 years in the Julian calendar, which equals - // 10000 years in the Gregorian calendar - pub fn max_value_julian(&self) -> f64 { - match self { - TimeUnit::Second => 4.6427e14, - TimeUnit::Minute => 7.7379e12, - TimeUnit::Hour => 1.2897e11, - TimeUnit::Day => 5373485.0, - TimeUnit::Month => 176546.0, - TimeUnit::Year => 14713.0, - } - } - - // Conversion factor from the unit to seconds - pub fn seconds_conversion(&self) -> f64 { - match self { - TimeUnit::Second => 1.0, - TimeUnit::Minute => 60.0, - TimeUnit::Hour => 3600.0, - TimeUnit::Day => 86400.0, - TimeUnit::Month => 2592000.0, - TimeUnit::Year => 31536000.0, - } - } -} - fn get_max_datetime_exclusive() -> NaiveDateTime { // The maximum date in SQLite is 9999-12-31 NaiveDateTime::new( @@ -224,7 +167,7 @@ fn get_date_time_from_time_value_float(value: f64) -> Result= TimeUnit::Day.max_value_julian() + || value >= 5373484.5 { return Err(DateTimeError::InvalidArgument(format!( "Invalid julian day: {}", From 6ffb03216fffa7faa821e685c28798ac8d46f08f Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 2 Aug 2024 16:03:26 +0300 Subject: [PATCH 4/5] core/datetime: Simplify error handling --- core/datetime.rs | 117 +++++++++++++---------------------------------- 1 file changed, 32 insertions(+), 85 deletions(-) diff --git a/core/datetime.rs b/core/datetime.rs index 7f31c0dab..a5170050c 100644 --- a/core/datetime.rs +++ b/core/datetime.rs @@ -1,24 +1,6 @@ use crate::types::OwnedValue; +use crate::Result; use chrono::{DateTime, NaiveDate, NaiveDateTime, NaiveTime, Timelike, Utc}; -use std::result::Result; -use std::{error::Error, fmt::Display}; - -#[derive(Debug)] -enum DateTimeError { - InvalidArgument(String), - Other(String), -} - -impl Display for DateTimeError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - DateTimeError::InvalidArgument(s) => write!(f, "Invalid argument: {}", s), - DateTimeError::Other(s) => write!(f, "Other error: {}", s), - } - } -} - -impl Error for DateTimeError {} fn get_max_datetime_exclusive() -> NaiveDateTime { // The maximum date in SQLite is 9999-12-31 @@ -28,51 +10,32 @@ fn get_max_datetime_exclusive() -> NaiveDateTime { ) } -pub fn get_date_from_time_value(time_value: &OwnedValue) -> crate::Result { +pub fn get_date_from_time_value(time_value: &OwnedValue) -> Result { let dt = parse_naive_date_time(time_value); - if dt.is_ok() { - return Ok(get_date_from_naive_datetime(dt.unwrap())); - } else { - match dt.unwrap_err() { - DateTimeError::InvalidArgument(_) => { - Ok(String::new()) - } - DateTimeError::Other(s) => { - Err(crate::error::LimboError::InvalidDate(s)) - } - } + match dt { + Some(dt) => Ok(get_date_from_naive_datetime(dt)), + None => Ok(String::new()), } } -pub fn get_time_from_datetime_value(time_value: &OwnedValue) -> crate::Result { +pub fn get_time_from_datetime_value(time_value: &OwnedValue) -> Result { let dt = parse_naive_date_time(time_value); - if dt.is_ok() { - return Ok(get_time_from_naive_datetime(dt.unwrap())); - } else { - match dt.unwrap_err() { - DateTimeError::InvalidArgument(_) => { - Ok(String::new()) - } - DateTimeError::Other(s) => { - Err(crate::error::LimboError::InvalidTime(s)) - } - } + match dt { + Some(dt) => Ok(get_time_from_naive_datetime(dt)), + None => Ok(String::new()), } } -fn parse_naive_date_time(time_value: &OwnedValue) -> Result { +fn parse_naive_date_time(time_value: &OwnedValue) -> Option { match time_value { OwnedValue::Text(s) => get_date_time_from_time_value_string(s), OwnedValue::Integer(i) => get_date_time_from_time_value_integer(*i), OwnedValue::Float(f) => get_date_time_from_time_value_float(*f), - _ => Err(DateTimeError::InvalidArgument(format!( - "Invalid time value: {}", - time_value - ))), + _ => None, } } -fn get_date_time_from_time_value_string(value: &str) -> Result { +fn get_date_time_from_time_value_string(value: &str) -> Option { // Time-value formats: // 1-7. YYYY-MM-DD[THH:MM[:SS[.SSS]]] // 8-10. HH:MM[:SS[.SSS]] @@ -83,7 +46,7 @@ fn get_date_time_from_time_value_string(value: &str) -> Result Result Result Result { +fn parse_datetime_with_optional_tz(value: &str, format: &str) -> Option { // Try parsing with timezone let with_tz_format = format.to_owned() + "%:z"; if let Ok(dt) = DateTime::parse_from_str(value, &with_tz_format) { - return Ok(dt.with_timezone(&Utc).naive_utc()); + return Some(dt.with_timezone(&Utc).naive_utc()); } let mut value_without_tz = value; @@ -147,36 +103,27 @@ fn parse_datetime_with_optional_tz( } // Parse without timezone - NaiveDateTime::parse_from_str(value_without_tz, format) - .map_err(|_| DateTimeError::InvalidArgument(format!("Invalid time value: {}", value))) + if let Ok(dt) = NaiveDateTime::parse_from_str(value_without_tz, format) { + return Some(dt); + } + None } -fn get_date_time_from_time_value_integer(value: i64) -> Result { +fn get_date_time_from_time_value_integer(value: i64) -> Option { i32::try_from(value).map_or_else( - |_| { - Err(DateTimeError::InvalidArgument(format!( - "Invalid julian day: {}", - value - ))) - }, + |_| None, |value| get_date_time_from_time_value_float(value as f64), ) } -fn get_date_time_from_time_value_float(value: f64) -> Result { - if value.is_infinite() - || value.is_nan() - || value < 0.0 - || value >= 5373484.5 - { - return Err(DateTimeError::InvalidArgument(format!( - "Invalid julian day: {}", - value - ))); +fn get_date_time_from_time_value_float(value: f64) -> Option { + if value.is_infinite() || value.is_nan() || value < 0.0 || value >= 5373484.5 { + return None; + } + match julian_day_converter::julian_day_to_datetime(value) { + Ok(dt) => Some(dt), + Err(_) => None, } - let dt = julian_day_converter::julian_day_to_datetime(value) - .map_err(|_| DateTimeError::Other("Failed parsing the julian date".to_string()))?; - Ok(dt) } fn is_leap_second(dt: &NaiveDateTime) -> bool { From 69eb851120b479ae7849b2a194d81a80bd395b04 Mon Sep 17 00:00:00 2001 From: Pekka Enberg Date: Fri, 2 Aug 2024 16:04:39 +0300 Subject: [PATCH 5/5] core/datetime: Move get_max_datetime_exclusive() to bottom Move the function so that it's callers are above the function definition for smoother flow when reading the code. --- core/datetime.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/core/datetime.rs b/core/datetime.rs index a5170050c..afa2ae728 100644 --- a/core/datetime.rs +++ b/core/datetime.rs @@ -2,14 +2,6 @@ use crate::types::OwnedValue; use crate::Result; use chrono::{DateTime, NaiveDate, NaiveDateTime, NaiveTime, Timelike, Utc}; -fn get_max_datetime_exclusive() -> NaiveDateTime { - // The maximum date in SQLite is 9999-12-31 - NaiveDateTime::new( - NaiveDate::from_ymd_opt(10000, 01, 01).unwrap(), - NaiveTime::from_hms_milli_opt(00, 00, 00, 000).unwrap(), - ) -} - pub fn get_date_from_time_value(time_value: &OwnedValue) -> Result { let dt = parse_naive_date_time(time_value); match dt { @@ -149,6 +141,14 @@ fn get_time_from_naive_datetime(value: NaiveDateTime) -> String { value.format("%H:%M:%S").to_string() } +fn get_max_datetime_exclusive() -> NaiveDateTime { + // The maximum date in SQLite is 9999-12-31 + NaiveDateTime::new( + NaiveDate::from_ymd_opt(10000, 01, 01).unwrap(), + NaiveTime::from_hms_milli_opt(00, 00, 00, 000).unwrap(), + ) +} + #[cfg(test)] mod tests { use super::*;