Skip to content
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.

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

feat: implement inequality post processing for timestamps #164

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Dustin-Ray
Copy link
Contributor

@Dustin-Ray Dustin-Ray commented Sep 17, 2024

Rationale for this change

Postprocessing was left out as a fast-follow for the TimestampTZ provable datatype. This PR introduces support for it, as a mitigation to #149.

What changes are included in this PR?

  • add a timezone normalization function
  • add timeunit normalization function
  • element_wise_eq
  • element_wise_ge
  • element_wise_le
  • slice_eq_with_casting_and_timeunit
  • slice_le_with_casting_and_timeunit
  • slice_ge_with_casting_and_timeunit

Are these changes tested?

  • test timeunit normalization
  • test timezone normalization
  • test element_wise_eq
  • test element_wise_le
  • test element_wise_ge
  • test slice_eq_with_casting_and_timeunit
  • test slice_le_with_casting_and_timeunit
  • test slice_ge_with_casting_and_timeunit

@Dustin-Ray Dustin-Ray force-pushed the feat/timestamp-inequality-postprocessing branch from bd31555 to d6335cc Compare September 17, 2024 01:21
@Dustin-Ray Dustin-Ray force-pushed the feat/timestamp-inequality-postprocessing branch from 80cf850 to 1ddefc3 Compare September 17, 2024 01:34
@Dustin-Ray Dustin-Ray marked this pull request as ready for review September 17, 2024 01:34
@Dustin-Ray Dustin-Ray self-assigned this Sep 17, 2024
@Dustin-Ray Dustin-Ray force-pushed the feat/timestamp-inequality-postprocessing branch from 1ddefc3 to 5a68319 Compare September 17, 2024 01:47
@Dustin-Ray Dustin-Ray requested a review from iajoiner September 17, 2024 01:48
(PoSQLTimeUnit::Second, PoSQLTimeUnit::Nanosecond) => timestamp * 1_000_000_000,

// Conversions from Milliseconds
(PoSQLTimeUnit::Millisecond, PoSQLTimeUnit::Second) => timestamp / 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm if we accept rounding then 10AM and 10AM plus 1ms will be identical if we normalize to s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok no rounding, got it


#[test]
fn test_normalize_timeunit_milliseconds_to_seconds() {
let timestamp = 1231006505000; // milliseconds
Copy link
Contributor

@iajoiner iajoiner Sep 17, 2024

Choose a reason for hiding this comment

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

What about 1231006505001? normalize_timeunit(1231006505000, MS, S) == normalize_timeunit(1231006505001, MS, S)

@Dustin-Ray Dustin-Ray force-pushed the feat/timestamp-inequality-postprocessing branch from 3d8ed68 to 9f20edd Compare September 21, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing ge, eq, ineq postprocessing implementations for TimeStampTZ
2 participants