From 715202162011e6b0ceee80ee2ebebfa4ad433911 Mon Sep 17 00:00:00 2001 From: Dustin Ray <40841027+drcapybara@users.noreply.github.com> Date: Thu, 27 Jun 2024 10:37:52 -0700 Subject: [PATCH] refactor: reorganize timestamp (#30) # Rationale for this change The timestamp module is large enough now to warrant its own submodule. It will grow in size with the addition of the intermediate timestamp conversions. # What changes are included in this PR? Moves various components of timestamp into appropriate modules. # Are these changes tested? yes --- .../src/base/commitment/column_bounds.rs | 2 +- .../commitment/column_commitment_metadata.rs | 2 +- .../src/base/commitment/committable_column.rs | 7 +- .../arrow_array_to_column_conversion.rs | 2 +- .../proof-of-sql/src/base/database/column.rs | 2 +- .../src/base/database/literal_value.rs | 2 +- .../database/owned_and_arrow_conversions.rs | 2 +- .../src/base/database/owned_column.rs | 2 +- .../src/base/database/owned_table_test.rs | 2 +- .../owned_table_test_accessor_test.rs | 2 +- .../src/base/database/owned_table_utility.rs | 5 +- crates/proof-of-sql/src/base/time/mod.rs | 4 +- .../proof-of-sql/src/base/time/timestamp.rs | 135 ----------------- crates/proof-of-sql/src/base/time/timezone.rs | 142 ++++++++++++++++++ 14 files changed, 162 insertions(+), 149 deletions(-) create mode 100644 crates/proof-of-sql/src/base/time/timezone.rs diff --git a/crates/proof-of-sql/src/base/commitment/column_bounds.rs b/crates/proof-of-sql/src/base/commitment/column_bounds.rs index 22106c930..0badc567f 100644 --- a/crates/proof-of-sql/src/base/commitment/column_bounds.rs +++ b/crates/proof-of-sql/src/base/commitment/column_bounds.rs @@ -292,7 +292,7 @@ mod tests { database::OwnedColumn, math::decimal::Precision, scalar::Curve25519Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; use itertools::Itertools; diff --git a/crates/proof-of-sql/src/base/commitment/column_commitment_metadata.rs b/crates/proof-of-sql/src/base/commitment/column_commitment_metadata.rs index 9d3fce8cd..2e0d9876d 100644 --- a/crates/proof-of-sql/src/base/commitment/column_commitment_metadata.rs +++ b/crates/proof-of-sql/src/base/commitment/column_commitment_metadata.rs @@ -169,7 +169,7 @@ mod tests { database::OwnedColumn, math::decimal::Precision, scalar::Curve25519Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; #[test] diff --git a/crates/proof-of-sql/src/base/commitment/committable_column.rs b/crates/proof-of-sql/src/base/commitment/committable_column.rs index a6fcb89dd..e8fc9dc16 100644 --- a/crates/proof-of-sql/src/base/commitment/committable_column.rs +++ b/crates/proof-of-sql/src/base/commitment/committable_column.rs @@ -3,7 +3,7 @@ use crate::base::{ math::decimal::Precision, ref_into::RefInto, scalar::Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; #[cfg(feature = "blitzar")] use blitzar::sequence::Sequence; @@ -194,7 +194,10 @@ impl<'a, 'b> From<&'a CommittableColumn<'b>> for Sequence<'a> { #[cfg(all(test, feature = "blitzar"))] mod tests { use super::*; - use crate::{base::scalar::Curve25519Scalar, proof_primitive::dory::DoryScalar}; + use crate::{ + base::{scalar::Curve25519Scalar, time::timezone::PoSQLTimeZone}, + proof_primitive::dory::DoryScalar, + }; use blitzar::compute::compute_curve25519_commitments; use curve25519_dalek::ristretto::CompressedRistretto; diff --git a/crates/proof-of-sql/src/base/database/arrow_array_to_column_conversion.rs b/crates/proof-of-sql/src/base/database/arrow_array_to_column_conversion.rs index 91bb4b6c1..cc3b78cb8 100644 --- a/crates/proof-of-sql/src/base/database/arrow_array_to_column_conversion.rs +++ b/crates/proof-of-sql/src/base/database/arrow_array_to_column_conversion.rs @@ -4,7 +4,7 @@ use crate::{ database::Column, math::decimal::Precision, scalar::Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }, sql::parse::ConversionError, }; diff --git a/crates/proof-of-sql/src/base/database/column.rs b/crates/proof-of-sql/src/base/database/column.rs index 22890aaf3..fb5dd908c 100644 --- a/crates/proof-of-sql/src/base/database/column.rs +++ b/crates/proof-of-sql/src/base/database/column.rs @@ -2,7 +2,7 @@ use super::{LiteralValue, TableRef}; use crate::base::{ math::decimal::{scale_scalar, Precision}, scalar::Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; use arrow::datatypes::{DataType, Field, TimeUnit as ArrowTimeUnit}; use bumpalo::Bump; diff --git a/crates/proof-of-sql/src/base/database/literal_value.rs b/crates/proof-of-sql/src/base/database/literal_value.rs index 76bc41865..e7ced4b93 100644 --- a/crates/proof-of-sql/src/base/database/literal_value.rs +++ b/crates/proof-of-sql/src/base/database/literal_value.rs @@ -2,7 +2,7 @@ use crate::base::{ database::ColumnType, math::decimal::Precision, scalar::Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; use serde::{Deserialize, Serialize}; diff --git a/crates/proof-of-sql/src/base/database/owned_and_arrow_conversions.rs b/crates/proof-of-sql/src/base/database/owned_and_arrow_conversions.rs index 3e551b89e..e0710e0ed 100644 --- a/crates/proof-of-sql/src/base/database/owned_and_arrow_conversions.rs +++ b/crates/proof-of-sql/src/base/database/owned_and_arrow_conversions.rs @@ -20,7 +20,7 @@ use crate::base::{ }, math::decimal::Precision, scalar::Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; use arrow::{ array::{ diff --git a/crates/proof-of-sql/src/base/database/owned_column.rs b/crates/proof-of-sql/src/base/database/owned_column.rs index 466c40871..ce1a3e321 100644 --- a/crates/proof-of-sql/src/base/database/owned_column.rs +++ b/crates/proof-of-sql/src/base/database/owned_column.rs @@ -6,7 +6,7 @@ use super::ColumnType; use crate::base::{ math::decimal::Precision, scalar::Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; #[derive(Debug, PartialEq, Clone, Eq)] #[non_exhaustive] diff --git a/crates/proof-of-sql/src/base/database/owned_table_test.rs b/crates/proof-of-sql/src/base/database/owned_table_test.rs index 617fda933..adbec791c 100644 --- a/crates/proof-of-sql/src/base/database/owned_table_test.rs +++ b/crates/proof-of-sql/src/base/database/owned_table_test.rs @@ -2,7 +2,7 @@ use crate::{ base::{ database::{owned_table_utility::*, OwnedColumn, OwnedTable, OwnedTableError}, scalar::Curve25519Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }, proof_primitive::dory::DoryScalar, }; diff --git a/crates/proof-of-sql/src/base/database/owned_table_test_accessor_test.rs b/crates/proof-of-sql/src/base/database/owned_table_test_accessor_test.rs index 0c70a1ade..5364cbb99 100644 --- a/crates/proof-of-sql/src/base/database/owned_table_test_accessor_test.rs +++ b/crates/proof-of-sql/src/base/database/owned_table_test_accessor_test.rs @@ -5,7 +5,7 @@ use super::{ use crate::base::{ database::owned_table_utility::*, scalar::{compute_commitment_for_testing, Curve25519Scalar}, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; use blitzar::proof::InnerProductProof; diff --git a/crates/proof-of-sql/src/base/database/owned_table_utility.rs b/crates/proof-of-sql/src/base/database/owned_table_utility.rs index 0b2131b30..7867e918e 100644 --- a/crates/proof-of-sql/src/base/database/owned_table_utility.rs +++ b/crates/proof-of-sql/src/base/database/owned_table_utility.rs @@ -16,7 +16,7 @@ use super::{OwnedColumn, OwnedTable}; use crate::base::{ scalar::Scalar, - time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}, + time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone}, }; use core::ops::Deref; use proof_of_sql_parser::Identifier; @@ -212,7 +212,8 @@ pub fn decimal75( /// ``` /// use proof_of_sql::base::{database::owned_table_utility::*, /// scalar::Curve25519Scalar, -/// time::timestamp::{PoSQLTimeUnit, PoSQLTimeZone}}; +/// time::{timestamp::PoSQLTimeUnit, timezone::PoSQLTimeZone} +/// }; /// use chrono_tz::Europe::London; /// /// let result = owned_table::([ diff --git a/crates/proof-of-sql/src/base/time/mod.rs b/crates/proof-of-sql/src/base/time/mod.rs index 2775c0048..5ed29b81a 100644 --- a/crates/proof-of-sql/src/base/time/mod.rs +++ b/crates/proof-of-sql/src/base/time/mod.rs @@ -1,2 +1,4 @@ -/// Stores all functionality relelvant to timestamps +/// Stores all functionality relelvant to timestamps and time units pub mod timestamp; +/// Stores functionality relevant to timezones +pub mod timezone; diff --git a/crates/proof-of-sql/src/base/time/timestamp.rs b/crates/proof-of-sql/src/base/time/timestamp.rs index 3ae5d5b8c..6c13f95ce 100644 --- a/crates/proof-of-sql/src/base/time/timestamp.rs +++ b/crates/proof-of-sql/src/base/time/timestamp.rs @@ -1,9 +1,6 @@ -use crate::base::database::{ArrowArrayToColumnConversionError, OwnedArrowConversionError}; use arrow::datatypes::TimeUnit as ArrowTimeUnit; -use chrono_tz::Tz; use core::fmt; use serde::{Deserialize, Serialize}; -use std::{str::FromStr, sync::Arc}; /// A wrapper around i64 to mitigate conflicting From /// implementations @@ -15,66 +12,6 @@ pub struct Time { pub unit: PoSQLTimeUnit, } -/// A typed TimeZone for a [`TimeStamp`]. It is optionally -/// used to define a timezone other than UTC for a new TimeStamp. -/// It exists as a wrapper around chrono-tz because chrono-tz does -/// not implement uniform bit distribution -#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, Serialize, Deserialize)] -pub struct PoSQLTimeZone(Tz); - -impl PoSQLTimeZone { - /// Convenience constant for the UTC timezone - pub const UTC: PoSQLTimeZone = PoSQLTimeZone(Tz::UTC); -} - -impl PoSQLTimeZone { - /// Create a new ProofsTimeZone from a chrono TimeZone - pub fn new(tz: Tz) -> Self { - PoSQLTimeZone(tz) - } -} - -impl From<&PoSQLTimeZone> for Arc { - fn from(timezone: &PoSQLTimeZone) -> Self { - Arc::from(timezone.0.name()) - } -} - -impl From for PoSQLTimeZone { - fn from(tz: Tz) -> Self { - PoSQLTimeZone(tz) - } -} - -impl fmt::Display for PoSQLTimeZone { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -impl TryFrom>> for PoSQLTimeZone { - type Error = &'static str; - - fn try_from(value: Option>) -> Result { - match value { - Some(arc_str) => Tz::from_str(&arc_str) - .map(PoSQLTimeZone) - .map_err(|_| "Invalid timezone string"), - None => Ok(PoSQLTimeZone(Tz::UTC)), // Default to UTC - } - } -} - -impl TryFrom<&str> for PoSQLTimeZone { - type Error = &'static str; - - fn try_from(value: &str) -> Result { - Tz::from_str(value) - .map(PoSQLTimeZone) - .map_err(|_| "Invalid timezone string") - } -} - /// Specifies different units of time measurement relative to the Unix epoch. It is essentially /// a wrapper over [arrow::datatypes::TimeUnit] so that we can derive Copy and implement custom traits /// such as bit distribution and Hash. @@ -123,81 +60,9 @@ impl From for PoSQLTimeUnit { } } -impl From<&'static str> for OwnedArrowConversionError { - fn from(error: &'static str) -> Self { - OwnedArrowConversionError::InvalidTimezone(error.to_string()) - } -} - -impl From<&'static str> for ArrowArrayToColumnConversionError { - fn from(error: &'static str) -> Self { - ArrowArrayToColumnConversionError::TimezoneConversionError(error.to_string()) - } -} - #[cfg(test)] mod tests { use super::*; - use chrono_tz::Tz; - - #[test] - fn valid_timezones_convert_correctly() { - let valid_timezones = ["Europe/London", "America/New_York", "Asia/Tokyo", "UTC"]; - - for tz_str in &valid_timezones { - let arc_tz = Arc::new(tz_str.to_string()); - // Convert Arc to Arc by dereferencing to &str then creating a new Arc - let arc_tz_str: Arc = Arc::from(&**arc_tz); - let timezone = PoSQLTimeZone::try_from(Some(arc_tz_str)); - assert!(timezone.is_ok(), "Timezone should be valid: {}", tz_str); - assert_eq!( - timezone.unwrap().0, - Tz::from_str(tz_str).unwrap(), - "Timezone mismatch for {}", - tz_str - ); - } - } - - #[test] - fn test_edge_timezone_strings() { - let edge_timezones = ["Etc/GMT+12", "Etc/GMT-14", "America/Argentina/Ushuaia"]; - for tz_str in &edge_timezones { - let arc_tz = Arc::from(*tz_str); - let result = PoSQLTimeZone::try_from(Some(arc_tz)); - assert!(result.is_ok(), "Edge timezone should be valid: {}", tz_str); - assert_eq!( - result.unwrap().0, - Tz::from_str(tz_str).unwrap(), - "Mismatch for edge timezone {}", - tz_str - ); - } - } - - #[test] - fn test_empty_timezone_string() { - let empty_tz = Arc::from(""); - let result = PoSQLTimeZone::try_from(Some(empty_tz)); - assert!(result.is_err(), "Empty timezone string should fail"); - } - - #[test] - fn test_unicode_timezone_strings() { - let unicode_tz = Arc::from("Europe/Paris\u{00A0}"); // Non-breaking space character - let result = PoSQLTimeZone::try_from(Some(unicode_tz)); - assert!( - result.is_err(), - "Unicode characters should not be valid in timezone strings" - ); - } - - #[test] - fn test_null_option() { - let result = PoSQLTimeZone::try_from(None); - assert!(result.is_ok(), "None should convert without error"); - assert_eq!(result.unwrap().0, Tz::UTC, "None should default to UTC"); - } #[test] fn we_can_convert_from_arrow_time_units() { diff --git a/crates/proof-of-sql/src/base/time/timezone.rs b/crates/proof-of-sql/src/base/time/timezone.rs new file mode 100644 index 000000000..cf875081c --- /dev/null +++ b/crates/proof-of-sql/src/base/time/timezone.rs @@ -0,0 +1,142 @@ +use crate::base::database::{ArrowArrayToColumnConversionError, OwnedArrowConversionError}; +use chrono_tz::Tz; +use core::fmt; +use serde::{Deserialize, Serialize}; +use std::{str::FromStr, sync::Arc}; + +/// A typed TimeZone for a [`TimeStamp`]. It is optionally +/// used to define a timezone other than UTC for a new TimeStamp. +/// It exists as a wrapper around chrono-tz because chrono-tz does +/// not implement uniform bit distribution +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, Serialize, Deserialize)] +pub struct PoSQLTimeZone(Tz); + +impl PoSQLTimeZone { + /// Convenience constant for the UTC timezone + pub const UTC: PoSQLTimeZone = PoSQLTimeZone(Tz::UTC); +} + +impl PoSQLTimeZone { + /// Create a new ProofsTimeZone from a chrono TimeZone + pub fn new(tz: Tz) -> Self { + PoSQLTimeZone(tz) + } +} + +impl From<&PoSQLTimeZone> for Arc { + fn from(timezone: &PoSQLTimeZone) -> Self { + Arc::from(timezone.0.name()) + } +} + +impl From for PoSQLTimeZone { + fn from(tz: Tz) -> Self { + PoSQLTimeZone(tz) + } +} + +impl fmt::Display for PoSQLTimeZone { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } +} + +impl TryFrom>> for PoSQLTimeZone { + type Error = &'static str; + + fn try_from(value: Option>) -> Result { + match value { + Some(arc_str) => Tz::from_str(&arc_str) + .map(PoSQLTimeZone) + .map_err(|_| "Invalid timezone string"), + None => Ok(PoSQLTimeZone(Tz::UTC)), // Default to UTC + } + } +} + +impl TryFrom<&str> for PoSQLTimeZone { + type Error = &'static str; + + fn try_from(value: &str) -> Result { + Tz::from_str(value) + .map(PoSQLTimeZone) + .map_err(|_| "Invalid timezone string") + } +} + +impl From<&'static str> for OwnedArrowConversionError { + fn from(error: &'static str) -> Self { + OwnedArrowConversionError::InvalidTimezone(error.to_string()) + } +} + +impl From<&'static str> for ArrowArrayToColumnConversionError { + fn from(error: &'static str) -> Self { + ArrowArrayToColumnConversionError::TimezoneConversionError(error.to_string()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono_tz::Tz; + + #[test] + fn valid_timezones_convert_correctly() { + let valid_timezones = ["Europe/London", "America/New_York", "Asia/Tokyo", "UTC"]; + + for tz_str in &valid_timezones { + let arc_tz = Arc::new(tz_str.to_string()); + // Convert Arc to Arc by dereferencing to &str then creating a new Arc + let arc_tz_str: Arc = Arc::from(&**arc_tz); + let timezone = PoSQLTimeZone::try_from(Some(arc_tz_str)); + assert!(timezone.is_ok(), "Timezone should be valid: {}", tz_str); + assert_eq!( + timezone.unwrap().0, + Tz::from_str(tz_str).unwrap(), + "Timezone mismatch for {}", + tz_str + ); + } + } + + #[test] + fn test_edge_timezone_strings() { + let edge_timezones = ["Etc/GMT+12", "Etc/GMT-14", "America/Argentina/Ushuaia"]; + for tz_str in &edge_timezones { + let arc_tz = Arc::from(*tz_str); + let result = PoSQLTimeZone::try_from(Some(arc_tz)); + assert!(result.is_ok(), "Edge timezone should be valid: {}", tz_str); + assert_eq!( + result.unwrap().0, + Tz::from_str(tz_str).unwrap(), + "Mismatch for edge timezone {}", + tz_str + ); + } + } + + #[test] + fn test_empty_timezone_string() { + let empty_tz = Arc::from(""); + let result = PoSQLTimeZone::try_from(Some(empty_tz)); + assert!(result.is_err(), "Empty timezone string should fail"); + } + + #[test] + fn test_unicode_timezone_strings() { + let unicode_tz = Arc::from("Europe/Paris\u{00A0}"); // Non-breaking space character + let result = PoSQLTimeZone::try_from(Some(unicode_tz)); + assert!( + result.is_err(), + "Unicode characters should not be valid in timezone strings" + ); + } + + #[test] + fn test_null_option() { + let result = PoSQLTimeZone::try_from(None); + assert!(result.is_ok(), "None should convert without error"); + assert_eq!(result.unwrap().0, Tz::UTC, "None should default to UTC"); + } +}