libenchao commented on code in PR #22832:
URL: https://github.com/apache/flink/pull/22832#discussion_r1242159866
##########
flink-table/flink-sql-gateway-api/src/main/java/org/apache/flink/table/gateway/api/results/ResultSetImpl.java:
##########
@@ -92,6 +117,10 @@ public RowDataToStringConverter getConverter() {
return converter;
}
+ public ZoneId getZoneId() {
Review Comment:
Adding public method to implementation class is usually not good practice,
even more, `ResultSetImpl` is marked as `Internal`, I'm not sure it's suitable
to put it in `flink-sql-gateway-api` module.
##########
flink-table/flink-sql-jdbc-driver/src/main/java/org/apache/flink/table/jdbc/utils/DatabaseMetaDataUtils.java:
##########
@@ -66,7 +67,8 @@ public static FlinkResultSet createCatalogsResultSet(
return new FlinkResultSet(
statement,
new CollectionResultIterator(catalogs.iterator()),
- ResolvedSchema.of(TABLE_CAT_COLUMN));
+ ResolvedSchema.of(TABLE_CAT_COLUMN),
+ ZoneId.systemDefault());
Review Comment:
For `createCatalogsResultSet` and `createSchemasResultSet`, if it's no need
to set the zone id (I guess there is no `timestamp` type for these two cases,
right?), it's better to add a comment to say it.
##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/serde/ResultInfo.java:
##########
@@ -57,19 +58,24 @@ public class ResultInfo {
private final List<ColumnInfo> columnInfos;
private final List<RowData> data;
private final RowFormat rowFormat;
+ private final ZoneId zoneId;
- ResultInfo(List<ColumnInfo> columnInfos, List<RowData> data, RowFormat
rowFormat) {
+ ResultInfo(
+ List<ColumnInfo> columnInfos, List<RowData> data, RowFormat
rowFormat, ZoneId zoneId) {
this.columnInfos = columnInfos;
this.data = data;
this.rowFormat = rowFormat;
+ this.zoneId = zoneId;
}
public static ResultInfo createResultInfo(ResultSet resultSet, RowFormat
rowFormat) {
Preconditions.checkArgument(resultSet.getResultType() !=
ResultSet.ResultType.NOT_READY);
List<RowData> data = resultSet.getData();
+ ZoneId zoneId = ZoneId.systemDefault();
switch (rowFormat) {
case JSON:
+ zoneId = ((ResultSetImpl) resultSet).getZoneId();
Review Comment:
Why `PLAIN_TEXT` does not need to care about zone id?
##########
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/gateway/StatementResult.java:
##########
@@ -110,4 +117,8 @@ public boolean hasNext() {
public RowData next() {
return resultProvider.next();
}
+
+ public ZoneId getZoneId() {
Review Comment:
Looking at this I realized that we are using `flink-sql-client` module which
is not designed to be used as a maven dependency, do you think it's possible to
get rid of this module for `flink-jdbc-driver` in the future?
--
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]
