wuchong commented on a change in pull request #15658:
URL: https://github.com/apache/flink/pull/15658#discussion_r615533331
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/PrintUtils.java
##########
@@ -309,9 +561,9 @@ public static String genBorderLine(int[] colWidths) {
* <p>And for java.sql.Timestamp, the number of digits after point
will be precision except
* when precision is 0. In that case, the format would be 'uuuu-MM-dd
HH:mm:ss.0'
*/
- private static int timestampTypeColumnWidth(int precision) {
+ private static int timestampTypeColumnWidth(int precision, Class<?>
conversionClass) {
int base = 19; // length of uuuu-MM-dd HH:mm:ss
- if (precision == 0) {
+ if (precision == 0 &&
java.sql.Timestamp.class.isAssignableFrom(conversionClass)) {
Review comment:
It seems it already considered `java.sql.Timestamp` according to the
Javadoc, do we need the additional check in condition?
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliUtils.java
##########
@@ -115,4 +118,10 @@ public static boolean createFile(final Path filePath) {
return false;
}
}
+
+ /** Get time zone from the the given session config. */
+ public static ZoneId getSessionTimeZone(ReadableConfig sessionConfig) {
+ final String zone =
sessionConfig.get(TableConfigOptions.LOCAL_TIME_ZONE);
+ return "default".equals(zone) ? ZoneId.systemDefault() :
ZoneId.of(zone);
Review comment:
Use `TableConfigOptions.LOCAL_TIME_ZONE.defaultValue()` instead of
`"default"`.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliTableResultView.java
##########
@@ -298,9 +299,19 @@ private void updatePage() {
return;
}
+ final ZoneId sessionTimeZone =
+ CliUtils.getSessionTimeZone(
+
client.getExecutor().getSessionConfig(client.getSessionId()));
Review comment:
Move the `sessionTimeZone` to constructor to keep align with
`CliTableauResultView`?
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -142,6 +145,7 @@ public CliClient(
this.executor = executor;
this.inputTransformer = inputTransformer;
this.historyFilePath = historyFilePath;
+ this.sessionTimeZone =
CliUtils.getSessionTimeZone(executor.getSessionConfig(sessionId));
Review comment:
Session time zone might be changed during the session, it should be
derived when been used.
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/PrintUtils.java
##########
@@ -174,27 +215,238 @@ public static void printAsTableauForm(
printWriter.flush();
}
- public static String[] rowToString(Row row) {
- return rowToString(row, NULL_COLUMN, false);
+ public static String[] rowToString(
+ Row row, ResolvedSchema resolvedSchema, ZoneId sessionTimeZone) {
+ return rowToString(row, NULL_COLUMN, false, resolvedSchema,
sessionTimeZone);
}
- public static String[] rowToString(Row row, String nullColumn, boolean
printRowKind) {
+ public static String[] rowToString(
+ Row row,
+ String nullColumn,
+ boolean printRowKind,
+ ResolvedSchema resolvedSchema,
+ ZoneId sessionTimeZone) {
final int len = printRowKind ? row.getArity() + 1 : row.getArity();
final List<String> fields = new ArrayList<>(len);
if (printRowKind) {
fields.add(row.getKind().shortString());
}
for (int i = 0; i < row.getArity(); i++) {
final Object field = row.getField(i);
+ final LogicalType fieldType =
+
resolvedSchema.getColumnDataTypes().get(i).getLogicalType();
if (field == null) {
fields.add(nullColumn);
} else {
- fields.add(StringUtils.arrayAwareToString(field));
+ fields.add(
+ StringUtils.arrayAwareToString(
+ normalizeTimestamp(field, fieldType,
sessionTimeZone)));
}
}
return fields.toArray(new String[0]);
}
+ /**
+ * Normalizes field that contains TIMESTAMP and TIMESTAMP_LTZ type data.
+ *
+ * <p>This method supports array type and nested type.
+ */
+ private static Object normalizeTimestamp(
Review comment:
The logic is quite huge, would be better to extract the logic into a
`DataStructureStringWriter`, similar to `DataStructureConverter`. We also have
another issue FLINK-21456 discussed about normalizing all data types, not only
timestamps. I think this can be good start point.
##########
File path:
flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientITCase.java
##########
@@ -91,9 +90,10 @@
File dir = firstFile.getParentFile();
final List<String> paths = new ArrayList<>();
final FilenameFilter filter = new PatternFilenameFilter(".*\\.q$");
- for (File f : Util.first(dir.listFiles(filter), new File[0])) {
- paths.add(f.getAbsolutePath().substring(commonPrefixLength));
- }
+ // for (File f : Util.first(dir.listFiles(filter), new
File[0])) {
+ //
paths.add(f.getAbsolutePath().substring(commonPrefixLength));
+ // }
+ paths.add("sql/select.q");
Review comment:
revert.
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliChangelogResultView.java
##########
@@ -115,6 +116,10 @@ protected void refresh() {
return;
}
+ final ZoneId sessionTimeZone =
+ CliUtils.getSessionTimeZone(
+
client.getExecutor().getSessionConfig(client.getSessionId()));
Review comment:
Move the `sessionTimeZone` to constructor to keep align with
`CliTableauResultView`?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
