Feat: Implements ord and partialord to timestamp #995

LevBeta · 2024-03-03T03:04:12Z

Related to #993

caspermeijn · 2024-03-08T08:55:04Z

prost-types/src/lib.rs

@@ -907,4 +917,38 @@ mod tests {
        // Must contain at least one "/" character.
        assert_eq!(TypeUrl::new("google.protobuf.Duration"), None);
    }
+
+    #[test]
+    fn check_timestamp_ord() {


Maybe you could add a parameterized test. This way you could easily test many scenarios . Here is an example:

prost/prost-types/src/lib.rs

Lines 733 to 805 in a1ab898

fn check_duration_normalize() {

#[rustfmt::skip] // Don't mangle the table formatting.

let cases = [

// --- Table of test cases ---

// test seconds test nanos expected seconds expected nanos

(line!(), 0, 0, 0, 0),

(line!(), 1, 1, 1, 1),

(line!(), -1, -1, -1, -1),

(line!(), 0, 999_999_999, 0, 999_999_999),

(line!(), 0, -999_999_999, 0, -999_999_999),

(line!(), 0, 1_000_000_000, 1, 0),

(line!(), 0, -1_000_000_000, -1, 0),

(line!(), 0, 1_000_000_001, 1, 1),

(line!(), 0, -1_000_000_001, -1, -1),

(line!(), -1, 1, 0, -999_999_999),

(line!(), 1, -1, 0, 999_999_999),

(line!(), -1, 1_000_000_000, 0, 0),

(line!(), 1, -1_000_000_000, 0, 0),

(line!(), i64::MIN , 0, i64::MIN , 0),

(line!(), i64::MIN + 1, 0, i64::MIN + 1, 0),

(line!(), i64::MIN , 1, i64::MIN + 1, -999_999_999),

(line!(), i64::MIN , 1_000_000_000, i64::MIN + 1, 0),

(line!(), i64::MIN , -1_000_000_000, i64::MIN , -999_999_999),

(line!(), i64::MIN + 1, -1_000_000_000, i64::MIN , 0),

(line!(), i64::MIN + 2, -1_000_000_000, i64::MIN + 1, 0),

(line!(), i64::MIN , -1_999_999_998, i64::MIN , -999_999_999),

(line!(), i64::MIN + 1, -1_999_999_998, i64::MIN , -999_999_998),

(line!(), i64::MIN + 2, -1_999_999_998, i64::MIN + 1, -999_999_998),

(line!(), i64::MIN , -1_999_999_999, i64::MIN , -999_999_999),

(line!(), i64::MIN + 1, -1_999_999_999, i64::MIN , -999_999_999),

(line!(), i64::MIN + 2, -1_999_999_999, i64::MIN + 1, -999_999_999),

(line!(), i64::MIN , -2_000_000_000, i64::MIN , -999_999_999),

(line!(), i64::MIN + 1, -2_000_000_000, i64::MIN , -999_999_999),

(line!(), i64::MIN + 2, -2_000_000_000, i64::MIN , 0),

(line!(), i64::MIN , -999_999_998, i64::MIN , -999_999_998),

(line!(), i64::MIN + 1, -999_999_998, i64::MIN + 1, -999_999_998),

(line!(), i64::MAX , 0, i64::MAX , 0),

(line!(), i64::MAX - 1, 0, i64::MAX - 1, 0),

(line!(), i64::MAX , -1, i64::MAX - 1, 999_999_999),

(line!(), i64::MAX , 1_000_000_000, i64::MAX , 999_999_999),

(line!(), i64::MAX - 1, 1_000_000_000, i64::MAX , 0),

(line!(), i64::MAX - 2, 1_000_000_000, i64::MAX - 1, 0),

(line!(), i64::MAX , 1_999_999_998, i64::MAX , 999_999_999),

(line!(), i64::MAX - 1, 1_999_999_998, i64::MAX , 999_999_998),

(line!(), i64::MAX - 2, 1_999_999_998, i64::MAX - 1, 999_999_998),

(line!(), i64::MAX , 1_999_999_999, i64::MAX , 999_999_999),

(line!(), i64::MAX - 1, 1_999_999_999, i64::MAX , 999_999_999),

(line!(), i64::MAX - 2, 1_999_999_999, i64::MAX - 1, 999_999_999),

(line!(), i64::MAX , 2_000_000_000, i64::MAX , 999_999_999),

(line!(), i64::MAX - 1, 2_000_000_000, i64::MAX , 999_999_999),

(line!(), i64::MAX - 2, 2_000_000_000, i64::MAX , 0),

(line!(), i64::MAX , 999_999_998, i64::MAX , 999_999_998),

(line!(), i64::MAX - 1, 999_999_998, i64::MAX - 1, 999_999_998),

];

for case in cases.iter() {

let mut test_duration = Duration {

seconds: case.1,

nanos: case.2,

};

test_duration.normalize();

assert_eq!(

test_duration,

Duration {

seconds: case.3,

nanos: case.4,

},

"test case on line {} doesn't match",

case.0,

);

}

}

caspermeijn · 2024-03-08T08:56:41Z

prost-types/src/protobuf.rs

@@ -2293,7 +2293,7 @@ impl NullValue {
 /// the time format spec '%Y-%m-%dT%H:%M:%S.%fZ'. Likewise, in Java, one can use
 /// the Joda Time's [`ISODateTimeFormat.dateTime()`](<http://www.joda.org/joda-time/apidocs/org/joda/time/format/ISODateTimeFormat.html#dateTime%2D%2D>) to obtain a formatter capable of generating timestamps in this format.
 #[allow(clippy::derive_partial_eq_without_eq)]
-#[derive(Clone, PartialEq, ::prost::Message)]
+#[derive(Clone, PartialEq, PartialOrd,::prost::Message)]


This file is generated, so your changes will be overwritten. Maybe you could add this to the generator so that all types can benefit from comparisons.

I believe this is the reason the CI tests fail

caspermeijn · 2024-03-08T08:59:09Z

prost-types/src/lib.rs

@@ -395,6 +395,16 @@ impl std::hash::Hash for Timestamp {
    }
 }

+#[cfg(feature = "std")]
+impl Ord for Timestamp {


I believe you could just #[derive(PartialOrd, Ord)] on struct Timestamp. At least your unit test succeeds.

I noticed a potential issue in both the comparison logic when dealing with timestamps. For instance, when comparing:
Timestamp { seconds: 1, nanos: 0 }

to

Timestamp { seconds: 0, nanos: 1_000_000_000 }

The current/(and #[derive(Ord)]) implementation considers the first timestamp as greater than the second one. However, it's crucial to recognize that the second timestamp actually represents the same duration.

There is a normalize function in Timestamp. I believe that does what you are looking for. https://docs.rs/prost-types/latest/prost_types/struct.Timestamp.html#method.normalize

LevBeta added 3 commits March 3, 2024 02:51

add ord and partialOrd to Timestamp

86eb768

FMT

6eb00b3

Fix no-std

a1ab898

caspermeijn reviewed Mar 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Implements ord and partialord to timestamp #995

Feat: Implements ord and partialord to timestamp #995

LevBeta commented Mar 3, 2024

caspermeijn Mar 8, 2024

caspermeijn Mar 8, 2024 •

edited

caspermeijn Mar 8, 2024

LevBeta Mar 8, 2024 •

edited

caspermeijn Mar 8, 2024

	fn check_duration_normalize() {
	#[rustfmt::skip] // Don't mangle the table formatting.
	let cases = [
	// --- Table of test cases ---
	// test seconds test nanos expected seconds expected nanos
	(line!(), 0, 0, 0, 0),
	(line!(), 1, 1, 1, 1),
	(line!(), -1, -1, -1, -1),
	(line!(), 0, 999_999_999, 0, 999_999_999),
	(line!(), 0, -999_999_999, 0, -999_999_999),
	(line!(), 0, 1_000_000_000, 1, 0),
	(line!(), 0, -1_000_000_000, -1, 0),
	(line!(), 0, 1_000_000_001, 1, 1),
	(line!(), 0, -1_000_000_001, -1, -1),
	(line!(), -1, 1, 0, -999_999_999),
	(line!(), 1, -1, 0, 999_999_999),
	(line!(), -1, 1_000_000_000, 0, 0),
	(line!(), 1, -1_000_000_000, 0, 0),
	(line!(), i64::MIN , 0, i64::MIN , 0),
	(line!(), i64::MIN + 1, 0, i64::MIN + 1, 0),
	(line!(), i64::MIN , 1, i64::MIN + 1, -999_999_999),
	(line!(), i64::MIN , 1_000_000_000, i64::MIN + 1, 0),
	(line!(), i64::MIN , -1_000_000_000, i64::MIN , -999_999_999),
	(line!(), i64::MIN + 1, -1_000_000_000, i64::MIN , 0),
	(line!(), i64::MIN + 2, -1_000_000_000, i64::MIN + 1, 0),
	(line!(), i64::MIN , -1_999_999_998, i64::MIN , -999_999_999),
	(line!(), i64::MIN + 1, -1_999_999_998, i64::MIN , -999_999_998),
	(line!(), i64::MIN + 2, -1_999_999_998, i64::MIN + 1, -999_999_998),
	(line!(), i64::MIN , -1_999_999_999, i64::MIN , -999_999_999),
	(line!(), i64::MIN + 1, -1_999_999_999, i64::MIN , -999_999_999),
	(line!(), i64::MIN + 2, -1_999_999_999, i64::MIN + 1, -999_999_999),
	(line!(), i64::MIN , -2_000_000_000, i64::MIN , -999_999_999),
	(line!(), i64::MIN + 1, -2_000_000_000, i64::MIN , -999_999_999),
	(line!(), i64::MIN + 2, -2_000_000_000, i64::MIN , 0),
	(line!(), i64::MIN , -999_999_998, i64::MIN , -999_999_998),
	(line!(), i64::MIN + 1, -999_999_998, i64::MIN + 1, -999_999_998),
	(line!(), i64::MAX , 0, i64::MAX , 0),
	(line!(), i64::MAX - 1, 0, i64::MAX - 1, 0),
	(line!(), i64::MAX , -1, i64::MAX - 1, 999_999_999),
	(line!(), i64::MAX , 1_000_000_000, i64::MAX , 999_999_999),
	(line!(), i64::MAX - 1, 1_000_000_000, i64::MAX , 0),
	(line!(), i64::MAX - 2, 1_000_000_000, i64::MAX - 1, 0),
	(line!(), i64::MAX , 1_999_999_998, i64::MAX , 999_999_999),
	(line!(), i64::MAX - 1, 1_999_999_998, i64::MAX , 999_999_998),
	(line!(), i64::MAX - 2, 1_999_999_998, i64::MAX - 1, 999_999_998),
	(line!(), i64::MAX , 1_999_999_999, i64::MAX , 999_999_999),
	(line!(), i64::MAX - 1, 1_999_999_999, i64::MAX , 999_999_999),
	(line!(), i64::MAX - 2, 1_999_999_999, i64::MAX - 1, 999_999_999),
	(line!(), i64::MAX , 2_000_000_000, i64::MAX , 999_999_999),
	(line!(), i64::MAX - 1, 2_000_000_000, i64::MAX , 999_999_999),
	(line!(), i64::MAX - 2, 2_000_000_000, i64::MAX , 0),
	(line!(), i64::MAX , 999_999_998, i64::MAX , 999_999_998),
	(line!(), i64::MAX - 1, 999_999_998, i64::MAX - 1, 999_999_998),
	];

	for case in cases.iter() {
	let mut test_duration = Duration {
	seconds: case.1,
	nanos: case.2,
	};
	test_duration.normalize();

	assert_eq!(
	test_duration,
	Duration {
	seconds: case.3,
	nanos: case.4,
	},
	"test case on line {} doesn't match",
	case.0,
	);
	}
	}

Feat: Implements ord and partialord to timestamp #995

Are you sure you want to change the base?

Feat: Implements ord and partialord to timestamp #995

Conversation

LevBeta commented Mar 3, 2024

caspermeijn Mar 8, 2024

Choose a reason for hiding this comment

caspermeijn Mar 8, 2024 • edited

Choose a reason for hiding this comment

caspermeijn Mar 8, 2024

Choose a reason for hiding this comment

LevBeta Mar 8, 2024 • edited

Choose a reason for hiding this comment

caspermeijn Mar 8, 2024

Choose a reason for hiding this comment

caspermeijn Mar 8, 2024 •

edited

LevBeta Mar 8, 2024 •

edited