make Timestamp implement IComparable (fixes #4267) #4318

warrenfalk · 2018-02-19T13:03:56Z

I've implemented IComparable<Timestamp> for the Timestamp class.

I have also implemented comparison operators <, >, <=, >=, ==, !=.

This fixes #4267 and is intended to make usage much more natural:

if (thing.Happened > setting.Deadline)
{
    // This is late
}

var inChronologicalOrder = listOfEvents.OrderBy(e => e.WhenItHappened);

grpc-kokoro · 2018-02-19T13:03:57Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

grpc-kokoro · 2018-02-19T13:03:57Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

anandolee · 2018-03-23T22:21:41Z

csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs

+        /// <returns>an integer indicating whether this timestamp precedes or follows the other</returns>
+        public int CompareTo(Timestamp other)
+        {
+            return other is null ? 1


Should consider that Timestamp may not normalized

anandolee · 2018-06-25T22:24:04Z

Friendly ping @warrenfalk, are you still working on this PR?

warrenfalk · 2018-06-25T22:54:31Z

Oh yeah, forgot about this. Yep give me a few minutes.

warrenfalk · 2018-06-25T23:06:06Z

Thanks for the ping. This has been updated to handle non-normalized Timestamps.

anandolee · 2018-06-29T22:28:10Z

csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs

+        /// <returns>true if the two timestamps refer to the same nanosecond</returns>
+        public static bool operator ==(Timestamp a, Timestamp b)
+        {
+            return ReferenceEquals(a, b) || (a is null ? (b is null ? true : false) : a.Equals(b));


For a.Equals(b): Maybe I missed somewhere but didn't see Equals() implementation in this file. Shouldn't use a.CompareTo(b)==0 instead? Maybe add a test to compare two equal timestamps which are not normalized

OK. I think normalization at this level is a mistake and recommend reconsidering removing it. Let me answer your questions and explain my reasoning, and you can decide what you want to do.

Equals() is already implemented in the other, generated half of the partial class. I can't change the implementation there, and I don't think we want to because ultimately Timestamp is a protocol buffer structure and it should follow protocol buffer semantics where "equals" means "structurally all fields are equal".

So if a user wants "time equality" instead of just "protocol buffer" equality then the user must (already, today) ensure that her structures are all normalized.

Keep in mind that equality and comparison are semantically separate things, which is why IEquatable and IComparable are separate interfaces. "Comparison" deals with ordering/sorting. So CompareTo(a) == 0 does not mean the same thing as Equals(a) == true and they do not necessarily have to match.

Additionally the operators don't match each other either. The < > <= >= operators are not necessarily related to ==. So it cannot be asserted that !(a < b) && !(a > b) == (a == b).

But of course people expect them to match, so I think we should make them match.

If you want them to match, however, you have to give up on normalization because Equals ignores it and can't be changed. So if you want "time comparison" then you have to pre-normalize, just like you have to already pre-normalize if you want "time equality".

So the bottom line is you can either have comparisons that normalize, or comparisons that match equality, but not both. You have to choose.

Agree, we should make them match. I talked with our tech lead, we think it make sense to not normalize for all comparison. Add comments that if the timestamp is not normalized the result may not correct in the API documentation

anandolee · 2018-06-29T22:30:17Z

csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs

+        {
+            if (!IsNormalized(Seconds, Nanos))
+                return Normalize(Seconds, Nanos).CompareTo(other);
+            if (other == null)


Better put null check in the first place (before normalize this timestamp).
Why compare to null should return 1? Is any other well known compare do like this that consider null as the smallest?

The BCL treats null this way. Consider: Debug.Assert(1 == "bcl".CompareTo(null));

See my other comment on normalization and how I think we should reconsider not doing it, but if we keep it, here are my thoughts here:

Doing a null check first will do the null check twice. Since the only Normalize() function we have creates an entirely new Timestamp instance, we just call that one's CompareTo() which will be doing its own null check.

I would understand if you wanted to do it anyway, considering un-normalized to be the exceptional case, and normalization to be the more expensive operation. So let me know. But I recommend we just not normalize in the first place.

anandolee · 2018-06-29T22:36:28Z

csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs

+        /// </summary>
+        /// <param name="other">Timestamp to compare</param>
+        /// <returns>an integer indicating whether this timestamp precedes or follows the other</returns>
+        public int CompareTo(Timestamp other)


Does CompareTo needs to be public? I'd prefer to make it private if possible and only expose < == operations etc. to users

Well, we are implementing IComparable, so does it have to be public? It does have to be exposed, yes. We could make the implementation explicit, but it will always be public on the interface and therefore always exposed to users and so there's no way around exposing it. Note that for reference, these are always public in the BCL, as far as I can tell.

warrenfalk · 2018-07-03T17:28:05Z

OK, added remarks to documentation about possible unexpected results when comparing non-normalized values, and removed normalization from comparison implementation.

anandolee

Just change the comments a little bit

anandolee · 2018-07-06T18:43:06Z

csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs

+        /// Given another timestamp, returns 0 if the timestamps are equivalent, -1 if this timestamp precedes the other, and 1 otherwise
+        /// </summary>
+        /// <remarks>
+        /// The result of comparing non-normalized timestamps is not specified and may give unexpected results


"Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results."

googlebot added the cla: yes label Feb 19, 2018

anandolee self-requested a review March 19, 2018 20:28

anandolee self-assigned this Mar 19, 2018

anandolee reviewed Mar 23, 2018

View reviewed changes

anandolee added the wait for user action label Apr 9, 2018

xfxyjwf added the c# label Jun 22, 2018

warrenfalk force-pushed the timestamp-comparison-operators branch from 783ca37 to f50064e Compare June 25, 2018 23:03

anandolee reviewed Jun 29, 2018

View reviewed changes

warrenfalk force-pushed the timestamp-comparison-operators branch 2 times, most recently from c905c3a to 8c0d098 Compare July 3, 2018 17:23

anandolee approved these changes Jul 6, 2018

View reviewed changes

implement IComparable and comparison operators on Timestamp

611e68d

warrenfalk force-pushed the timestamp-comparison-operators branch from 8c0d098 to 611e68d Compare July 9, 2018 11:26

anandolee approved these changes Jul 9, 2018

View reviewed changes

anandolee added the kokoro:run label Jul 9, 2018

kokoro-team removed the kokoro:run label Jul 9, 2018

anandolee merged commit 96833b8 into protocolbuffers:master Jul 9, 2018

google-admin unassigned anandolee Aug 21, 2018

xfxyjwf assigned anandolee Aug 24, 2018

anandolee mentioned this pull request Sep 24, 2018

#3965 well known rich comparisons #5168

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make Timestamp implement IComparable (fixes #4267) #4318

make Timestamp implement IComparable (fixes #4267) #4318

warrenfalk commented Feb 19, 2018

grpc-kokoro commented Feb 19, 2018

grpc-kokoro commented Feb 19, 2018

anandolee Mar 23, 2018

anandolee commented Jun 25, 2018

warrenfalk commented Jun 25, 2018

warrenfalk commented Jun 25, 2018

anandolee Jun 29, 2018

warrenfalk Jun 30, 2018

anandolee Jul 2, 2018

anandolee Jun 29, 2018

warrenfalk Jun 30, 2018

anandolee Jun 29, 2018

warrenfalk Jun 30, 2018

warrenfalk commented Jul 3, 2018

anandolee left a comment

anandolee Jul 6, 2018

make Timestamp implement IComparable (fixes #4267) #4318

make Timestamp implement IComparable (fixes #4267) #4318

Conversation

warrenfalk commented Feb 19, 2018

grpc-kokoro commented Feb 19, 2018

grpc-kokoro commented Feb 19, 2018

Choose a reason for hiding this comment

anandolee commented Jun 25, 2018

warrenfalk commented Jun 25, 2018

warrenfalk commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warrenfalk commented Jul 3, 2018

anandolee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment