Skip to content

Implement timestamp unit infer for other types #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Viicos wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Implement timestamp unit infer for other types #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ impl DateTimeConfigBuilder {
pub fn build(self) -> DateTimeConfig {
DateTimeConfig {
timestamp_unit: self.timestamp_unit.unwrap_or_default(),
time_config: self.time_config.unwrap_or_default(),
time_config: self
.time_config
.unwrap_or_else(|| TimeConfigBuilder::new().timestamp_unit(TimestampUnit::Second).build()),

@Viicos Viicos Oct 6, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests pass also if I use the default (which will use TimestampUnit::Infer), but I'm trying to be extra safe here and make the config match the existing default behavior (see below).

@Viicos Viicos Oct 6, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the time config attribute of the datetime config should only allow TimestampUnit::second..

@davidhewitt davidhewitt Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also just remove timestamp_unit from DateTimeConfig so that it's not possible for there to be inconsistency? Would be breaking, but I think not too bad for users to adapt to.

Infer and Second are presumably equivalent for Time parsing, because the watershed is far out of range of a valid 24 hours.

}
}
}
Expand All @@ -105,6 +107,7 @@ impl DateTimeConfig {
#[derive(Debug, Clone, Default, PartialEq)]
pub struct TimeConfig {
pub microseconds_precision_overflow_behavior: MicrosecondsPrecisionOverflowBehavior,
pub timestamp_unit: TimestampUnit,
pub unix_timestamp_offset: Option<i32>,
}

Expand All @@ -117,6 +120,7 @@ impl TimeConfig {
#[derive(Debug, Clone, Default)]
pub struct TimeConfigBuilder {
microseconds_precision_overflow_behavior: Option<MicrosecondsPrecisionOverflowBehavior>,
timestamp_unit: Option<TimestampUnit>,
unix_timestamp_offset: Option<i32>,
}

Expand All @@ -131,13 +135,18 @@ impl TimeConfigBuilder {
self.microseconds_precision_overflow_behavior = Some(microseconds_precision_overflow_behavior);
self
}
pub fn timestamp_unit(mut self, timestamp_unit: TimestampUnit) -> Self {
self.timestamp_unit = Some(timestamp_unit);
self
}
pub fn unix_timestamp_offset(mut self, unix_timestamp_offset: Option<i32>) -> Self {
self.unix_timestamp_offset = unix_timestamp_offset;
self
}
pub fn build(self) -> TimeConfig {
TimeConfig {
microseconds_precision_overflow_behavior: self.microseconds_precision_overflow_behavior.unwrap_or_default(),
timestamp_unit: self.timestamp_unit.unwrap_or_default(),
unix_timestamp_offset: self.unix_timestamp_offset,
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/date.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ impl Date {
/// assert_eq!(d.to_string(), "2022-06-07");
/// ```
pub fn from_timestamp(timestamp: i64, require_exact: bool, config: &DateConfig) -> Result<Self, ParseError> {
let (seconds, microseconds) = timestamp_to_seconds_micros(timestamp, config.timestamp_unit)?;
let (seconds, microseconds) = timestamp_to_seconds_micros(timestamp, config.timestamp_unit, MS_WATERSHED)?;
let (d, remaining_seconds) = Self::from_timestamp_calc(seconds)?;
if require_exact && (remaining_seconds != 0 || microseconds != 0) {
return Err(ParseError::DateNotExact);
Expand Down
3 changes: 2 additions & 1 deletion src/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,8 @@ impl DateTime {
timestamp_microsecond: u32,
config: &DateTimeConfig,
) -> Result<Self, ParseError> {
let (mut second, extra_microsecond) = timestamp_to_seconds_micros(timestamp, config.timestamp_unit)?;
let (mut second, extra_microsecond) =
timestamp_to_seconds_micros(timestamp, config.timestamp_unit, MS_WATERSHED)?;

@Viicos Viicos Oct 6, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..If you expand below, you'll see that we call Time::from_timestamp_with_config() for the time component (which before this PR expected seconds as input). That's why as per the previous comment we need to make sure TimestampUnit::Second is used.

@davidhewitt davidhewitt Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could have Time::from_seconds_timestamp_with_config as a private API to avoid that possible wrinkle (which would basically be the implementation before this PR, and from_timestamp_with_config is a public wrapper which processes the value and calls from_seconds_timestamp_with_config).

let mut total_microsecond = timestamp_microsecond
.checked_add(extra_microsecond)
.ok_or(ParseError::TimeTooLarge)?;
Expand Down
23 changes: 15 additions & 8 deletions src/time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ use std::fmt;
use std::str::FromStr;

use crate::config::TimeConfigBuilder;
use crate::util::timestamp_to_seconds_micros;
use crate::{get_digit, get_digit_unchecked, ConfigError, ParseError, TimeConfig};

pub(crate) const TIME_MS_WATERSHED: i64 = 86_400;

/// A Time
///
/// Allowed formats:
Expand Down Expand Up @@ -259,7 +262,7 @@ impl Time {
///
/// # Arguments
///
/// * `timestamp_second` - timestamp in seconds
/// * `timestamp` - timestamp in seconds
/// * `timestamp_microsecond` - microseconds fraction of a second timestamp
/// * `config` - the `TimeConfig` to use
///
Expand All @@ -274,17 +277,21 @@ impl Time {
/// assert_eq!(d.to_string(), "01:02:20.000123");
/// ```
pub fn from_timestamp_with_config(
timestamp_second: u32,
timestamp: u32,
timestamp_microsecond: u32,
config: &TimeConfig,
) -> Result<Self, ParseError> {
let mut second = timestamp_second;
let mut microsecond = timestamp_microsecond;
if microsecond >= 1_000_000 {
let (mut second, extra_microsecond) =
timestamp_to_seconds_micros(timestamp.into(), config.timestamp_unit, TIME_MS_WATERSHED)?;
let mut total_microsecond = timestamp_microsecond
.checked_add(extra_microsecond)
.ok_or(ParseError::TimeTooLarge)?;

if total_microsecond >= 1_000_000 {
second = second
.checked_add(microsecond / 1_000_000)
.checked_add(total_microsecond as i64 / 1_000_000)
.ok_or(ParseError::TimeTooLarge)?;
microsecond %= 1_000_000;
total_microsecond %= 1_000_000;
}
if second >= 86_400 {
return Err(ParseError::TimeTooLarge);
Expand All @@ -293,7 +300,7 @@ impl Time {
hour: (second / 3600) as u8,
minute: ((second % 3600) / 60) as u8,
second: (second % 60) as u8,
microsecond,
microsecond: total_microsecond,
tz_offset: config.unix_timestamp_offset,
})
}
Expand Down
13 changes: 8 additions & 5 deletions src/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use crate::date::MS_WATERSHED;
use crate::{ParseError, TimestampUnit};

pub(crate) fn timestamp_watershed(timestamp: i64) -> Result<(i64, u32), ParseError> {
pub(crate) fn timestamp_watershed(timestamp: i64, infer_threshold: i64) -> Result<(i64, u32), ParseError> {
let ts_abs = timestamp.checked_abs().ok_or(ParseError::DateTooSmall)?;
if ts_abs <= MS_WATERSHED {
if ts_abs <= infer_threshold {
return Ok((timestamp, 0));
}
let mut seconds = timestamp / 1_000;
Expand All @@ -15,7 +14,11 @@ pub(crate) fn timestamp_watershed(timestamp: i64) -> Result<(i64, u32), ParseErr
Ok((seconds, microseconds as u32))
}

pub fn timestamp_to_seconds_micros(timestamp: i64, unit: TimestampUnit) -> Result<(i64, u32), ParseError> {
pub fn timestamp_to_seconds_micros(
timestamp: i64,
unit: TimestampUnit,
infer_threshold: i64,
) -> Result<(i64, u32), ParseError> {
match unit {
TimestampUnit::Second => Ok((timestamp, 0)),
TimestampUnit::Millisecond => {
Expand All @@ -27,6 +30,6 @@ pub fn timestamp_to_seconds_micros(timestamp: i64, unit: TimestampUnit) -> Resul
}
Ok((seconds, microseconds as u32))
}
TimestampUnit::Infer => timestamp_watershed(timestamp),
TimestampUnit::Infer => timestamp_watershed(timestamp, infer_threshold),
}
}
2 changes: 2 additions & 0 deletions tests/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1543,6 +1543,7 @@ fn test_time_config_builder() {
TimeConfigBuilder::new().build(),
TimeConfig {
microseconds_precision_overflow_behavior: MicrosecondsPrecisionOverflowBehavior::Error,
timestamp_unit: TimestampUnit::Infer,
unix_timestamp_offset: None,
}
);
Expand All @@ -1568,6 +1569,7 @@ fn test_datetimetime_config_builder() {
timestamp_unit: TimestampUnit::Infer,
time_config: TimeConfig {
microseconds_precision_overflow_behavior: MicrosecondsPrecisionOverflowBehavior::Error,
timestamp_unit: TimestampUnit::Second,
unix_timestamp_offset: None,
}
}
Expand Down
Loading