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.

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

make Timestamp implement IComparable (fixes #4267) #4318

Merged
merged 1 commit into from
Jul 9, 2018
Merged

make Timestamp implement IComparable (fixes #4267) #4318

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
101 changes: 101 additions & 0 deletions csharp/src/Google.Protobuf.Test/WellKnownTypes/TimestampTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,5 +111,106 @@ public void ToString_NonNormalized()
var duration = new Timestamp { Seconds = 1, Nanos = -1 };
Assert.AreEqual("{ \"@warning\": \"Invalid Timestamp\", \"seconds\": \"1\", \"nanos\": -1 }", duration.ToString());
}

[Test]
public void Comparability()
{
Timestamp
a = null,
b = new Timestamp { Seconds = 1, Nanos = 1 },
c = new Timestamp { Seconds = 1, Nanos = 10 },
d = new Timestamp { Seconds = 10, Nanos = 1 },
e = new Timestamp { Seconds = 10, Nanos = 10 };

Assert.IsTrue(b.CompareTo(a) > 0); // null is always first (according to default behavior of Array.Sort)
Assert.IsTrue(b.CompareTo(b) == 0);
Assert.IsTrue(b.CompareTo(b.Clone()) == 0);
Assert.IsTrue(b.CompareTo(c) < 0);
Assert.IsTrue(b.CompareTo(d) < 0);
Assert.IsTrue(b.CompareTo(e) < 0);

Assert.IsTrue(c.CompareTo(a) > 0);
Assert.IsTrue(c.CompareTo(b) > 0);
Assert.IsTrue(c.CompareTo(c) == 0);
Assert.IsTrue(c.CompareTo(c.Clone()) == 0);
Assert.IsTrue(c.CompareTo(d) < 0);
Assert.IsTrue(c.CompareTo(e) < 0);

Assert.IsTrue(d.CompareTo(a) > 0);
Assert.IsTrue(d.CompareTo(b) > 0);
Assert.IsTrue(d.CompareTo(c) > 0);
Assert.IsTrue(d.CompareTo(d) == 0);
Assert.IsTrue(d.CompareTo(d.Clone()) == 0);
Assert.IsTrue(d.CompareTo(e) < 0);

Assert.IsTrue(e.CompareTo(a) > 0);
Assert.IsTrue(e.CompareTo(b) > 0);
Assert.IsTrue(e.CompareTo(c) > 0);
Assert.IsTrue(e.CompareTo(d) > 0);
Assert.IsTrue(e.CompareTo(e) == 0);
Assert.IsTrue(e.CompareTo(e.Clone()) == 0);
}


[Test]
public void ComparabilityOperators()
{
Timestamp
a = null,
b = new Timestamp { Seconds = 1, Nanos = 1 },
c = new Timestamp { Seconds = 1, Nanos = 10 },
d = new Timestamp { Seconds = 10, Nanos = 1 },
e = new Timestamp { Seconds = 10, Nanos = 10 };

#pragma warning disable CS1718 // Comparison made to same variable
Assert.IsTrue(b > a);
Assert.IsTrue(b == b);
Assert.IsTrue(b == b.Clone());
Assert.IsTrue(b < c);
Assert.IsTrue(b < d);
Assert.IsTrue(b < e);

Assert.IsTrue(c > a);
Assert.IsTrue(c > b);
Assert.IsTrue(c == c);
Assert.IsTrue(c == c.Clone());
Assert.IsTrue(c < d);
Assert.IsTrue(c < e);

Assert.IsTrue(d > a);
Assert.IsTrue(d > b);
Assert.IsTrue(d > c);
Assert.IsTrue(d == d);
Assert.IsTrue(d == d.Clone());
Assert.IsTrue(d < e);

Assert.IsTrue(e > a);
Assert.IsTrue(e > b);
Assert.IsTrue(e > c);
Assert.IsTrue(e > d);
Assert.IsTrue(e == e);
Assert.IsTrue(e == e.Clone());

Assert.IsTrue(b >= a);
Assert.IsTrue(b <= c);
Assert.IsTrue(b <= d);
Assert.IsTrue(b <= e);

Assert.IsTrue(c >= a);
Assert.IsTrue(c >= b);
Assert.IsTrue(c <= d);
Assert.IsTrue(c <= e);

Assert.IsTrue(d >= a);
Assert.IsTrue(d >= b);
Assert.IsTrue(d >= c);
Assert.IsTrue(d <= e);

Assert.IsTrue(e >= a);
Assert.IsTrue(e >= b);
Assert.IsTrue(e >= c);
Assert.IsTrue(e >= d);
#pragma warning restore CS1718 // Comparison made to same variable
}
}
}
105 changes: 104 additions & 1 deletion csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

namespace Google.Protobuf.WellKnownTypes
{
public partial class Timestamp : ICustomDiagnosticMessage
public partial class Timestamp : ICustomDiagnosticMessage, IComparable<Timestamp>
{
private static readonly DateTime UnixEpoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
// Constants determined programmatically, but then hard-coded so they can be constant expressions.
Expand Down Expand Up @@ -222,6 +222,109 @@ internal static string ToJson(long seconds, int nanoseconds, bool diagnosticOnly
}
}

/// <summary>
/// Given another timestamp, returns 0 if the timestamps are equivalent, -1 if this timestamp precedes the other, and 1 otherwise
/// </summary>
/// <remarks>
/// Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results.
/// </remarks>
/// <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)
{
return other == null ? 1
: Seconds < other.Seconds ? -1
: Seconds > other.Seconds ? 1
: Nanos < other.Nanos ? -1
: Nanos > other.Nanos ? 1
: 0;
}

/// <summary>
/// Compares two timestamps and returns whether the first is less than (chronologically precedes) the second
/// </summary>
/// <remarks>
/// Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results.
/// </remarks>
/// <param name="a"></param>
/// <param name="b"></param>
/// <returns>true if a precedes b</returns>
public static bool operator <(Timestamp a, Timestamp b)
{
return a.CompareTo(b) < 0;
}

/// <summary>
/// Compares two timestamps and returns whether the first is greater than (chronologically follows) the second
/// </summary>
/// <remarks>
/// Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results.
/// </remarks>
/// <param name="a"></param>
/// <param name="b"></param>
/// <returns>true if a follows b</returns>
public static bool operator >(Timestamp a, Timestamp b)
{
return a.CompareTo(b) > 0;
}

/// <summary>
/// Compares two timestamps and returns whether the first is less than (chronologically precedes) the second
/// </summary>
/// <remarks>
/// Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results.
/// </remarks>
/// <param name="a"></param>
/// <param name="b"></param>
/// <returns>true if a precedes b</returns>
public static bool operator <=(Timestamp a, Timestamp b)
{
return a.CompareTo(b) <= 0;
}

/// <summary>
/// Compares two timestamps and returns whether the first is greater than (chronologically follows) the second
/// </summary>
/// <remarks>
/// Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results.
/// </remarks>
/// <param name="a"></param>
/// <param name="b"></param>
/// <returns>true if a follows b</returns>
public static bool operator >=(Timestamp a, Timestamp b)
{
return a.CompareTo(b) >= 0;
}


/// <summary>
/// Returns whether two timestamps are equivalent
/// </summary>
/// <remarks>
/// Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results.
/// </remarks>
/// <param name="a"></param>
/// <param name="b"></param>
/// <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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

/// <summary>
/// Returns whether two timestamps differ
/// </summary>
/// <remarks>
/// Make sure the timestamps are normalized. Comparing non-normalized timestamps is not specified and may give unexpected results.
/// </remarks>
/// <param name="a"></param>
/// <param name="b"></param>
/// <returns>true if the two timestamps differ</returns>
public static bool operator !=(Timestamp a, Timestamp b)
{
return !(a == b);
}

/// <summary>
/// Returns a string representation of this <see cref="Timestamp"/> for diagnostic purposes.
/// </summary>
Expand Down