Demonstrating that compareTo() is still not transitive

[kabutz]

Here is another test that demonstrates that the compareTo() method is still not transitive, the way it has been written.

#77 should be re-opened.

[dsgrieve]

Given the way DateTimeStamp is implemented, compareTo() will never be transitive. About the only thing that could be done is to throw away the ZonedDateTime part and just stick with the time stamp.

However, a log file should always have only a date stamp, or only a time stamp, or both a date and a time stamp, or neither. In other words, these cases where there is compare between a DateTimeStamp created with the ISO date string and a DateTimeStamp created with a double should never arise.

[kabutz]

"should never arise" - ok, can we perhaps add code to stop the creation of a DateTimeStamp that would result in an incorrect comparison?

Having an incorrect compareTo() method can lead to very strange exceptions during sorting.

[brunoborges]

@dsgrieve should we close this PR? It's been a while

[kabutz]

Bugs don’t fix themselves. This demonstrates a bug, which should be fixed.

[ghost]

DateTimeStamp has been rewritten and is now transitive as it should be.

[kabutz]

Which version is supposed to be correct? The one in main or message?

[kabutz]

Actually, both versions fail this simple transitivity test:

@Test public void compareToTransitivityWithEqualTimeStamps() { DateTimeStamp stamp1 = new DateTimeStamp("2021-09-01T11:12:13.111-0100", 100); DateTimeStamp stamp2 = new DateTimeStamp((String)null, 100); DateTimeStamp stamp3 = new DateTimeStamp("2021-08-31T11:12:13.111-0100", 100); int comp1To2 = stamp1.compareTo(stamp2); int comp2To3 = stamp2.compareTo(stamp3); assertEquals(comp1To2, comp2To3); int comp1To3 = stamp1.compareTo(stamp3); assertEquals(comp1To2, comp1To3, "compareTo() is not transitive"); }

[ghost]

In light of the comments I created this table of all of the possible combinations of date time. The confusion is that preunified can have a mix of times with dates and times without dates in a single log. The edge case is where there is neither a date nor a time but in that case no useful analysis can be performed so those DateTimeStamps never get created.

Table of possible states, P - value is present, M - value is missing, last two lines indicate if the combination is possible in a GC log

date stamp 0 | P | P | P | P | M | M | M | M
time stamp 0 | P | M | M | M | P | M | M | M
date stamp 1 | P | P | M | M | P | P | M | M
time stamp 1 | P | P | P | M | P | P | P | M
Pre-Unified  | Y | N | N | N | Y | N | N | Y
Unified      | Y | N | N | N | N | N | N | Y

    public void compareToTransitivityWithEqualTimeStamps() {
        DateTimeStamp stamp1 = new DateTimeStamp("2021-09-01T11:12:13.111-0100", 100);
        DateTimeStamp stamp2 = new DateTimeStamp((String)null, 100);
        DateTimeStamp stamp3 = new DateTimeStamp("2021-08-31T11:12:13.111-0100", 100);
        int comp1To2 = stamp1.compareTo(stamp2);
        int comp2To3 = stamp2.compareTo(stamp3);
        assertEquals(comp1To2, comp2To3);
        int comp1To3 = stamp1.compareTo(stamp3);
        assertEquals(comp1To2, comp1To3, "compareTo() is not transitive");
    }

You test consists of 3 DateTimeStamps {P,P}, {M,P}, {P,P} and I think the comparisons below should yield the results below DateTimeStamp{P,P}, DateTimeStamp{M,P} == 0 DateTimeStamp{M,P}, DateTimeStamp{P,P} == 0 DateTimeStamp{P,P}, DateTimeStamp{P,P} == 0

Lets make this test pass then.