On Tue, 14 Jul 2026 22:25:32 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 two additional commits
> since the last revision:
>
> - 8272194: remove autoCloseArguments from Date/Timestamp
> LocalDate/LocalDateTime conversion tests
> - 8272194: remove outdated comment in Timestamp LocalDateTime conversion
After the PR is approved, we generally wait around 24-hours and if there are no
further comments, the author can comment `/integrate` to proceed with
integrating the pull request. After which, one of us can sponsor the change.
But before you do that, a few final suggestions =). Following some offline
discussion, we think that the core logic is in good shape, but there is
opportunity to reduce some duplication. Please see the other comments I left.
src/java.sql/share/classes/java/sql/Date.java line 287:
> 285: * providing a faster code path for almost 1970 years.
> 286: */
> 287: private static final long TWO_AD_AT_UTC_EPOCH_MILLIS =
> -62104233600000L;
We can make this package-private and have `Timestamp` use it as well to reduce
some duplication.
src/java.sql/share/classes/java/sql/Date.java line 324:
> 322: // 0002-01-01 are AD, we can use a much faster local date
> derivation
> 323: // for these dates.
> 324: if (getTime() >= TWO_AD_AT_UTC_EPOCH_MILLIS) {
We can pull out the shared logic here and in `Timestamp` and create a static
package-private helper in `java.sql.Date`.
Something like
static int toProlepticYear(long millis, int year) {
if slow path
set calendar using setTimeInMillis
if calendar is in BC
adjust year
return year;
}
The helper can call `GregorianCalendar.setTimeInMillis(...)` instead of
`GregorianCalendar.setTime(...)`. That lets us pass its millisecond value via
`getTime()` rather than having to pass `this` as a `java.util.Date` to the
helper method. I also think within the helper, you can inverse the `getTime()
>= TWO_AD_AT_UTC_EPOCH_MILLIS` conditional to simplify the logic some more.
After that, `toLocalDate` need only look something like,
return LocalDate.of(
toProlepticYear(getTime(), getYear() + 1900),
getMonth() + 1,
getDate());
-------------
Changes requested by jlu (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/31808#pullrequestreview-4716719450
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3598074086
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3598143658
