On Tue, 14 Jul 2026 18:42:10 GMT, Sholto <[email protected]> wrote: >> Also covers: [8249280](https://bugs.openjdk.org/browse/JDK-8249280) >> >> I will first give a quick summary of the problem. >> Put simply, the `LocalDate` form of the `java.sql.Date` is derived using the >> `getYear` method of `java.util.Date`. This in turn returns the year of the >> normalised internal calendar. >> However, the internal calendar `getYear` has an extra layer of complexity. >> The calendar has an additional era field, which captures BC/AD. >> `getYear` therefore just returns the year _of that era_. >> For example, the year 6BC and the year 6AD both return `getYear` as 6. >> >> **This means that for BC dates, our `LocalDate` conversion loses the sign of >> the year.** >> >> This leads to additional problems down the line, as the year 1BC is for >> calculations sake is considered to be year 0 (and 2BC us considered year -1 >> and so on). As a result, the various leap year calculations are WRONG for >> these years, causing year format validation failures in situations like >> marshalling/unmarshalling the dates with a DB. >> >> There are two seemingly obvious fixes here, however I will attempt to >> explain why I did not proceed with them. >> >> Firstly, it seems sensible is to derive the `LocalDate` from an `Instant` >> created from the millisecond representation of the `Date`. After all, why we >> are having to use the deprecated `getYear`, `getMonth` and `getDay` methods >> anyway? >> The answer lies in [8061577](https://bugs.openjdk.org/browse/JDK-8061577). >> The underlying millisecond representation between `java.time.Instant` and >> `java.util.Date` is fundamentally different. Read that ticket for a greater >> explanation. >> Ultimately though, it means that the for older dates, the only real way to >> bridge between the two calendar systems is to use these year/month/day >> methods. >> >> This is where the second possible solution appears. >> The underlying calendar representation that `java.util.Date` uses actually >> does have a year method which gives you the correctly signed year, that >> being `getNormalizedYear`. >> In fact, `java.util.Date` uses the setter counterpart `setNormalizedYear` is >> its `setYear` method. >> Given this, it seems natural that `getYear` should similarly call >> `getNormalizedYear`. >> I think this would be my ideal solution, however I recognise that `get`Year >> only returning a positive year is very long standing behaviour. Given how >> widely spread `java.util.Date` is, I felt it was perhaps better not to rock >> the boat too much. >> >> I have therefore taken the decision to add an ... > > Sholto has updated the pull request incrementally with one additional commit > since the last revision: > > 8272194: only use created calendar for BC check in LocalDate/LocalDateTime > conversion
Thanks for your changes so far. These are the only remaining issues I spotted. src/java.sql/share/classes/java/sql/Timestamp.java line 567: > 565: getMinutes(), > 566: getSeconds(), > 567: getNanos()); // We have to use nanos > here as the calendar does not support nano precision This comment is stale and can be removed. test/jdk/java/sql/test/sql/DateTests.java line 334: > 332: * Ensures that the fix for 8272194 has not regressed. > 333: */ > 334: @ParameterizedTest(autoCloseArguments = false) I understand you are following the existing test convention, but I would remove the `autoCloseArguments = false` here and in the other occurrences since the parameter does not implement `AutoCloseable`, so it's just noise. (It is added to those other tests even though not needed as part of an earlier migration to conservatively match TestNG semantics.) ------------- PR Review: https://git.openjdk.org/jdk/pull/31808#pullrequestreview-4698282197 PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3582413297 PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3582452309
