davidradl commented on code in PR #29030:
URL: https://github.com/apache/flink/pull/29030#discussion_r3872325372
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -184,7 +185,7 @@ public static String buildShowCreateMaterializedTableRow(
.ifPresent(partitionedBy ->
sb.append(formatPartitionedBy(partitionedBy)));
extractFormattedOptions(table.getOptions(), PRINT_INDENT,
additionalSensitiveKeys)
.ifPresent(v -> sb.append("WITH
(\n").append(v).append("\n)\n"));
- sb.append(extractStartMode(table, timeZoneId)).append("\n");
+ sb.append(extractStartMode(table, timeZoneId,
Clock.systemUTC())).append("\n");
Review Comment:
My AI said
1. LocalDateTime.now(clock).minus(amount).toInstant(ZoneOffset.UTC) — is the
zone still correct?
The fix calls LocalDateTime.now(clock) where clock is Clock.systemUTC().
LocalDateTime.now(Clock.systemUTC()) returns UTC wall time as a LocalDateTime
(no zone info). Then .toInstant(ZoneOffset.UTC) reinterprets it as UTC — which
is consistent. This is correct for the UTC clock case.
However, if someone ever passes a non-UTC clock (e.g. for a different
locale), the zone mislabeling would re-emerge silently. A more robust fix would
be Instant.now(clock).minus(amount) followed by direct formatting, avoiding
LocalDateTime entirely since it's zone-naive. This is a minor concern given
Clock.systemUTC() is hardcoded at the call site, but worth a comment or a
follow-up.
2. The test uses ZoneOffset.UTC as the timeZoneId — does the production call
path ever use a non-UTC zone?
Looking at buildShowCreateMaterializedTableRow, timeZoneId is passed in from
a session config. If the session is non-UTC, the test doesn't cover that path.
Not a blocker, but a test with a non-UTC zone would strengthen confidence.
3. The 1-month case change (1970-01-02 12:34:56 → 1969-10-29 12:34:56):
Subtracting 1 month from 1970-01-02 gives 1969-12-02, not 1969-10-29. This
suggests the fixed clock used in that test case is not 1970-01-02 — it's a
different anchor. This is likely fine (the test fixtures use a mocked fixed
time), but reviewers should confirm the expected value is correct for the
fixture's clock value. The PR description doesn't explicitly state what fixed
time that parameterized case uses.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
