MINOR: Fix an off-by-one issue in ValuesTest #20520
Conversation
Signed-off-by: Alex <[email protected]>
github-actions
bot
added
triage
There was a problem hiding this comment.
Pull Request Overview
Fixes an off-by-one error in the ValuesTest that occurred when local time falls on a different day than UTC time. The test was using LocalDate.ofEpochDay(days) which could result in incorrect date conversion when timezone differences cause day boundaries to differ between local and UTC time.
- Replaces
LocalDate.ofEpochDay(days)withlocalTime.format(DateTimeFormatter.ISO_LOCAL_DATE)to ensure consistent local time handling - Removes unused
LocalDateimport
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -903,7 +901,7 @@ public void shouldConvertDateValues() { | |||
|
|
|||
| // ISO8601 strings - accept a string matching pattern "yyyy-MM-dd" | |||
| LocalDateTime localTimeTruncated = localTime.truncatedTo(ChronoUnit.DAYS); | |||
| java.util.Date d3 = Values.convertToDate(Date.SCHEMA, LocalDate.ofEpochDay(days).format(DateTimeFormatter.ISO_LOCAL_DATE)); | |||
| java.util.Date d3 = Values.convertToDate(Date.SCHEMA, localTime.format(DateTimeFormatter.ISO_LOCAL_DATE)); | |||
There was a problem hiding this comment.
The variable days is referenced in the original code but not visible in this diff. If days was calculated from localTime, this change may not fix the off-by-one issue as intended, since localTime.format(DateTimeFormatter.ISO_LOCAL_DATE) will still use the same local date that could differ from UTC. Consider using a fixed date string or ensuring the test uses UTC time consistently.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
this change may not fix the off-by-one issue as intended, since localTime.format(DateTimeFormatter.ISO_LOCAL_DATE) will still use the same local date that could differ from UTC.
@WenHsuanYu What do you think for this comment?
Signed-off-by: Alex <[email protected]>
This test case ensures that the parser can convert ISO8601 correctly.
However, when the local time falls on a different day than the UTC time,
there will be an off-by-one issue.
I changed the test to convert the local time and then compare it with
the expected local time. This should fix the off-by-one issue.
Reference
link
Reviewers: Andrew Schofield [email protected]