On Fri, 10 Jul 2026 12:48: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: introduce faster code path for LocalDate/LocalDateTime conversion
src/java.sql/share/classes/java/sql/Timestamp.java line 529:
> 527: // Given that we can comfortably state that any dates after the
> unix epoch
> 528: // are AD, we can use a much faster local date time derivation
> for these dates.
> 529: if (getTime() >= 0) {
> We could potentially expand the number of dates covered by this faster
> codepath by using a negative unix time representing a much earlier year.
That's a good idea. I agree that representing some negative time with a magic
number is not as straightforward as `0`, but considering we can retain almost
the same performance for an extra ~1970 years, I think it is a worthwhile
tradeoff. Granted we provide a big enough buffer to account for an arbitrary
time zone offset, and the value is documented properly.
That would pretty much make only the BC years suffer the slowdown, which seems
extremely justifiable since those are exactly the cases where the behavior was
incorrect.
test/jdk/java/sql/test/sql/DateTests.java line 1:
> 1: /*
Since there is good traction on this PR, I think you can go ahead and update
the copyright years for any files that need it. Also, since these tests don't
have individual Jtreg headers, I think it is worth including the JBS bug ID
8272194 in the test comments to better associate the change with the test.
test/jdk/java/sql/test/sql/DateTests.java line 338:
> 336: Date d1 = Date.valueOf(ld1);
> 337: LocalDate ld2 = d1.toLocalDate();
> 338: assertTrue(ld1.equals(ld2), "Error ld1 != ld2");
You might use `assertEquals` here and in the other occurrences as a more
accurate assertion. Perhaps also update the error message to make it clear that
the failed assertion is about negative years.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3561830075
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3561852826
PR Review Comment: https://git.openjdk.org/jdk/pull/31808#discussion_r3561862435
