Conversation
…ndar Timestamp validated day-of-month with proleptic Gregorian leap rules but converted to and from epoch milliseconds through java.util.Date, which applies the Julian calendar before 1582-10-15. 1582-10-05T00:00:00Z and 1582-10-15T00:00:00Z shared one epoch millisecond, so compareTo called them equal while equals did not, and every earlier date denoted an instant days away from the one its text names (amazon-ion#165). Both directions now use exact proleptic Gregorian day arithmetic, with no Calendar allocated, which is the strategy amazon-ion#165 asked for after the Calendar-backed attempt was reverted for heap usage. calendarValue() moves the Julian cutover out of range so its fields agree. Two existing expectations encoded the Julian value and are corrected; a new test pins the cutover dates as distinct instants.
1c3b971 to
de78754
Compare
toddjonker
left a comment
toddjonker
left a comment
There was a problem hiding this comment.
Have the spec considerations that I noted in 2018 been addressed?
How do other Ion libraries behave in similar circumstances?
Can we add coverage for this issue in the ion-tests suite so that any divergence between implementations are known?
| private static long epochDayFromCivil(int year, int month, int day) | ||
| { | ||
| long y = year - (month <= 2 ? 1 : 0); | ||
| long era = (y >= 0 ? y : y - 399) / 400; | ||
| long yearOfEra = y - era * 400; // [0, 399] | ||
| long dayOfYear = (153 * (month + (month > 2 ? -3 : 9)) + 2) / 5 + day - 1; // [0, 365] | ||
| long dayOfEra = yearOfEra * 365 + yearOfEra / 4 - yearOfEra / 100 + dayOfYear; | ||
| return era * 146097 + dayOfEra - 719468; | ||
| } | ||
|
|
||
| /** | ||
| * Inverse of {@link #epochDayFromCivil(int, int, int)}; fills year, month and day. | ||
| */ | ||
| private static void civilFromEpochDay(long epochDay, int[] yearMonthDay) | ||
| { | ||
| long z = epochDay + 719468; | ||
| long era = (z >= 0 ? z : z - 146096) / 146097; | ||
| long dayOfEra = z - era * 146097; // [0, 146096] | ||
| long yearOfEra = (dayOfEra - dayOfEra / 1460 + dayOfEra / 36524 - dayOfEra / 146096) / 365; | ||
| long y = yearOfEra + era * 400; | ||
| long dayOfYear = dayOfEra - (365 * yearOfEra + yearOfEra / 4 - yearOfEra / 100); | ||
| long monthPrime = (5 * dayOfYear + 2) / 153; // [0, 11] | ||
| long day = dayOfYear - (153 * monthPrime + 2) / 5 + 1; // [1, 31] | ||
| long month = monthPrime + (monthPrime < 10 ? 3 : -9); // [1, 12] | ||
| yearMonthDay[0] = (int) (y + (month <= 2 ? 1 : 0)); | ||
| yearMonthDay[1] = (int) month; | ||
| yearMonthDay[2] = (int) day; | ||
| } |
There was a problem hiding this comment.
This is going to be effectively unmaintainable without a lot more documentation. Way too many magic numbers. Are we sure we cannot use JDK APIs to handle this math?
If not, I have to wonder whether this edge-case bug is worth fixing. Timestamp has been one of the most bug-prone components of the whole library, and this change smells of increasing brittleness, or at least of making future debugging more difficult.
There was a problem hiding this comment.
To be a little more clear: to the extent that I have any authority as original maintainer, I would reject this change as currently written, for the simple reason that I cannot read and understand the code. And if it could, I could not easily verify its correctness.
If you wish to continue, please:
- Add links to documentation/explanation of the algorithm and math
- Reduce the use of magic numbers
- Do what you can to make it easy for future maintainers to understand, and to make it "obviously correct".
Thanks!
There was a problem hiding this comment.
The conversion now goes through java.time (LocalDateTime, whose ISO chronology is proleptic Gregorian), so the hand-written arithmetic and its constants are gone and the Javadoc points at IsoChronology; each call builds a transient LocalDateTime, as master's Date.UTC builds a calendar date and a Date, and nothing is retained per Timestamp. For callers, millis before 1582-10-15 now agree with Instant both ways (master's forEpochSecond(i.getEpochSecond(), i.getNano(), 0), documented as equivalent to Instant i, turns 1582-10-05 into 1582-09-25), forMillis rejects the two days of millis master accepted before 0001-01-01, and calendarValue() has its cutover moved out of range, so it no longer equals a default GregorianCalendar. If that is still not worth it, fine to close.
The epoch conversion now goes through LocalDateTime, whose ISO chronology is the proleptic Gregorian calendar, instead of hand-written day arithmetic. Math.floorDiv replaces the local floorDiv, and requireByte, no longer used, is removed.
|
The spec still names no calendar, but ion-java already validates days by proleptic Gregorian rules (it rejects |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1165 +/- ##
============================================
+ Coverage 67.23% 67.93% +0.69%
- Complexity 5484 5666 +182
============================================
Files 159 160 +1
Lines 23025 23359 +334
Branches 4126 4203 +77
============================================
+ Hits 15481 15868 +387
+ Misses 6262 6190 -72
- Partials 1282 1301 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
2 participants
Issue #, if available: #165
Description of changes:
Timestampvalidates day-of-month with proleptic Gregorian leap rules but converted to and from epoch milliseconds throughjava.util.Date, which applies the Julian calendar before 1582-10-15.1582-10-05T00:00:00Zand1582-10-15T00:00:00Zshared one epoch millisecond, socompareTocalled them equal whileequalsdid not.Both directions now go through
java.time'sLocalDateTime(ISO chronology, proleptic Gregorian), with noCalendarallocated. Before 1582-10-15,getMillis()andforMillis()agree withInstant, andforMillis()rejects the two days of millis below 0001-01-01;calendarValue()moves its cutover out of range for every date, so it no longerequalsa defaultGregorianCalendar. Two Julian-value test expectations are corrected, and a new test pins the cutover dates as distinct instants; it fails on master../gradlew buildpasses.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.