AndrewJSchofield commented on code in PR #18611:
URL: https://github.com/apache/kafka/pull/18611#discussion_r1923511981


##########
connect/api/src/test/java/org/apache/kafka/connect/data/ValuesTest.java:
##########
@@ -901,8 +902,12 @@ public void shouldConvertDateValues() {
         assertEquals(currentDate, d2);
 
         // ISO8601 strings - accept a string matching pattern "yyyy-MM-dd"
+        // Values.convertToDate convert the "yyyy-MM-dd" string will miss the 
time and timezone information, 
+        // so we need to adjust the date from the current timezone to UTC0
+        TimeZone tz = TimeZone.getDefault();
+        java.util.Date currentDate3 = new java.util.Date(current.getTime() - 
currentMillis - tz.getOffset(current.getTime()));
         java.util.Date d3 = Values.convertToDate(Date.SCHEMA, 
LocalDate.ofEpochDay(days).format(DateTimeFormatter.ISO_LOCAL_DATE));
-        assertEquals(currentDate, d3);

Review Comment:
   @divijvaidya is correct that `java.util.Date` is timezone-agnostic. However, 
the `LocalDate` here is timezone-ignorant and the timezone offset is not 
accounted for. The methods available for manipulating dates and times with 
timezones are a bit limited, and I think that it's best to stop using 
`LocalDate.ofEpochDay(long)` and moving to `LocalDateTime.ofInstant(Instant, 
ZoneId)`.
   
   I suggest replacing the first line of the method with:
   ```
           LocalDateTime localTime = LocalDateTime.now();
           LocalDateTime localTimeTruncated = 
localTime.truncatedTo(ChronoUnit.DAYS);
           ZoneOffset zoneOffset = 
ZoneId.systemDefault().getRules().getOffset(localTime);
           java.util.Date current = new 
java.util.Date(localTime.toEpochSecond(zoneOffset) * 1000);
   ```
   
   And then the check becomes:
   ```
           // ISO8601 strings - accept a string matching pattern "yyyy-MM-dd"
           java.util.Date d3 = Values.convertToDate(Date.SCHEMA, 
LocalDate.ofEpochDay(days).format(DateTimeFormatter.ISO_LOCAL_DATE));
           LocalDateTime date3 = 
LocalDateTime.ofInstant(Instant.ofEpochMilli(d3.getTime()), 
ZoneId.systemDefault());
           assertEquals(localTimeTruncated, date3);
   ```
   
   I tried this with various timezones locally and it seems to work properly. 
Essentially, the time information is discarded in a way that a person in the 
timezone would expect looking at a clock.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to