hanyuzheng7 commented on code in PR #25759:
URL: https://github.com/apache/flink/pull/25759#discussion_r1887309875
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/TimeFunctionsITCase.java:
##########
@@ -732,6 +764,39 @@ private Stream<TestSetSpec> floorTestCases() {
$("f2").floor(TimeIntervalUnit.MILLENNIUM),
"FLOOR(f2 TO MILLENNIUM)",
LocalDateTime.of(2001, 1, 1, 0, 0),
- TIMESTAMP().nullable()));
+ TIMESTAMP().nullable())
+ .testResult(
+ $("f2").cast(TIMESTAMP_LTZ(3))
+ .floor(TimeIntervalUnit.SECOND)
+ .cast(STRING()),
+ "CAST(FLOOR(CAST(f2 AS TIMESTAMP_LTZ(3)) TO
SECOND) AS STRING)",
+ formatAsTimestamp(LocalDateTime.of(2020, 2,
29, 1, 56, 59, 0)),
+ STRING().nullable())
+ .testResult(
+ $("f2").cast(TIMESTAMP_LTZ(3))
+ .floor(TimeIntervalUnit.MINUTE)
+ .cast(STRING()),
+ "CAST(FLOOR(CAST(f2 AS TIMESTAMP_LTZ(3)) TO
MINUTE) AS STRING)",
+ formatAsTimestamp(LocalDateTime.of(2020, 2,
29, 1, 56, 0, 0)),
+ STRING().nullable())
+ .testResult(
+ $("f2").cast(TIMESTAMP_LTZ(3))
+ .floor(TimeIntervalUnit.HOUR)
+ .cast(STRING()),
+ "CAST(FLOOR(CAST(f2 AS TIMESTAMP_LTZ(3)) TO
HOUR) AS STRING)",
+ formatAsTimestamp(LocalDateTime.of(2020, 2,
29, 1, 0, 0, 0)),
+ STRING().nullable())
+ .testResult(
+ $("f2").cast(TIMESTAMP_LTZ(3))
+ .floor(TimeIntervalUnit.MILLISECOND)
+ .cast(STRING()),
+ "CAST(FLOOR(CAST(f2 AS TIMESTAMP_LTZ(3)) TO
MILLISECOND) AS STRING)",
+ formatAsTimestamp(
+ LocalDateTime.of(2020, 2, 29, 1, 56,
59, 987_000_000)),
+ STRING().nullable()));
+ }
+
+ private static String formatAsTimestamp(LocalDateTime dateTime) {
+ return dateTime.format(DateTimeFormatter.ofPattern("yyyy-MM-dd'
'HH:mm:ss.SSS"));
Review Comment:
Thank you for your suggestion. I have a question: if we extract
`DateTimeFormatter.ofPattern("yyyy-MM-dd' 'HH:mm:ss.SSS")` as a separate
constant, should we still keep the `formatAsTimestamp` method?
For example, is it better to write:
```
private static final DateTimeFormatter TIMESTAMP_FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd' 'HH:mm:ss.SSS");
private static String formatAsTimestamp(LocalDateTime dateTime) {
return dateTime.format(TIMESTAMP_FORMATTER);
}
.testResult(
$("f2").cast(TIMESTAMP_LTZ(3))
.floor(TimeIntervalUnit.MILLISECOND)
.cast(STRING()),
"CAST(FLOOR(CAST(f2 AS TIMESTAMP_LTZ(3)) TO MILLISECOND) AS STRING)",
formatAsTimestamp(
LocalDateTime.of(2020, 2, 29, 1, 56, 59, 987_000_000)
),
STRING().nullable()
);
```
Or should we directly use TIMESTAMP_FORMATTER like this:
```
private static final DateTimeFormatter TIMESTAMP_FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd' 'HH:mm:ss.SSS");
.testResult(
$("f2").cast(TIMESTAMP_LTZ(3))
.floor(TimeIntervalUnit.MILLISECOND)
.cast(STRING()),
"CAST(FLOOR(CAST(f2 AS TIMESTAMP_LTZ(3)) TO MILLISECOND) AS STRING)",
LocalDateTime.of(2020, 2, 29, 1, 56, 59, 987_000_000)
.format(TIMESTAMP_FORMATTER),
STRING().nullable()
);
```
However, I prefer a third approach: passing DateTimeFormatter as a parameter
to the formatAsTimestamp method. This keeps the method flexible and reusable
while encapsulating the formatting logic. For example:
```
private static String formatAsTimestamp(LocalDateTime dateTime,
DateTimeFormatter formatter) {
return dateTime.format(formatter);
}
private static final DateTimeFormatter TIMESTAMP_FORMATTER =
DateTimeFormatter.ofPattern("yyyy-MM-dd' 'HH:mm:ss.SSS");
.testResult(
$("f2").cast(TIMESTAMP_LTZ(3))
.floor(TimeIntervalUnit.MILLISECOND)
.cast(STRING()),
"CAST(FLOOR(CAST(f2 AS TIMESTAMP_LTZ(3)) TO MILLISECOND) AS STRING)",
formatAsTimestamp(
LocalDateTime.of(2020, 2, 29, 1, 56, 59, 987_000_000),
TIMESTAMP_FORMATTER
),
STRING().nullable()
);
```
This approach offers better flexibility, as it allows us to use different
formatters for varying requirements without needing to redefine the logic. It
also makes the code more extensible for future changes.
--
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]
