chia7712 commented on code in PR #18611:
URL: https://github.com/apache/kafka/pull/18611#discussion_r2318146619
##########
connect/api/src/test/java/org/apache/kafka/connect/data/ValuesTest.java:
##########
@@ -901,8 +903,10 @@ public void shouldConvertDateValues() {
assertEquals(currentDate, d2);
// 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));
Review Comment:
Sorry for raising this question on the merged PR. This test case is intended
to ensure the parser works for `ISO8601`. That said, is it really necessary to
use `days` here? It seems to me that relying on `days` could introduce a risk:
since the granularity is at the "day" level, it my case an off-by-one issue if
the local time falls on a different "day" than UTC. For example, when the local
time is 9/1 01:00 (UTC+8)
Perhaps we could instead use
`localTime.format(DateTimeFormatter.ISO_LOCAL_DATE)` to avoid the conversion
risk
@AndrewJSchofield WDYT?
@AndrewJSchofield WDYT?
##########
connect/api/src/test/java/org/apache/kafka/connect/data/ValuesTest.java:
##########
@@ -901,8 +903,10 @@ public void shouldConvertDateValues() {
assertEquals(currentDate, d2);
// 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));
Review Comment:
Sorry for raising this question on the merged PR. This test case is intended
to ensure the parser works for `ISO8601`. That said, is it really necessary to
use `days` here? It seems to me that relying on `days` could introduce a risk:
since the granularity is at the "day" level, it my case an off-by-one issue if
the local time falls on a different "day" than UTC. For example, when the local
time is 9/1 01:00 (UTC+8)
Perhaps we could instead use
`localTime.format(DateTimeFormatter.ISO_LOCAL_DATE)` to avoid the conversion
risk
@AndrewJSchofield WDYT?
--
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
