[GitHub] [flink] libenchao commented on a diff in pull request #22832: [FLINK-32396][jdbc-driver] Support timestamp and timestamp_ltz for jdbc driver and sql gateway

["libenchao (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org