On Mon, 13 Jul 2026 12:07:04 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: expand dates covered by faster code path in
> LocalDate/LocalDateTime conversion
src/java.sql/share/classes/java/sql/Timestamp.java line 561:
> 559: year = 1 - calendar.get(Calendar.YEAR);
> 560: } else {
> 561: year = calendar.get(Calendar.YEAR);
`getYear()` is still bypassed on this path. The `calendar` should only be used
to determine the era. The year value should continue to be obtained through
`getYear()`.
test/jdk/java/sql/test/sql/TimestampTests.java line 727:
> 725: LocalDateTime.of(-4, 9, 8, 23, 11, 59, 999_999_999),
> 726: LocalDateTime.of(-1000, 3, 22, 8, 30, 20, 0)
> 727: );
Could you make this a JUnit parameterized test, using `@ParameterizedTest`,
rather than iterating over a List? Please also add dates/date-times immediately
around the cutoff to verify the conversion there, particularly that values at
or after the cutoff are AD.
test/jdk/java/sql/test/sql/TimestampTests.java line 733:
> 731: assertTrue(bcTimestamp.toLocalDateTime().getYear() <= 0,
> "Expected a BC year.");
> 732: assertEquals(bcTimestamp.toLocalDateTime(), bcLocalDateTime,
> "The LocalDateTime created from the Timestamp does not match the original BC
> LocalDateTime.");
> 733:
> assertEquals(Timestamp.valueOf(bcTimestamp.toLocalDateTime()), bcTimestamp,
> "The BC Timestamp did not yield the expected result on a round trip Timestamp
> / LocalDateTime conversion.");
The `expected` and `actual` arguments to `assertEquals` appear to be reversed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3572910531
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3574816978
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3574846188
