You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:58:48 UTC

[GitHub] [zeppelin] zjffdu opened a new pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

zjffdu opened a new pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137


   
   ### What is this PR for?
   
   Add timezone support for flink interpreter, It is only available for flink 1.13, previous version are not supported.
   
   
   ### What type of PR is it?
   [ Improvement ]
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   * https://issues.apache.org/jira/browse/ZEPPELIN-5392
   
   ### How should this be tested?
   * Manually tested.
   
   ### Screenshots (if appropriate)
   ![image](https://user-images.githubusercontent.com/164491/121799463-d8ba1d00-cc5e-11eb-8a45-776b94496bd1.png)
   
   ### Questions:
   * Does the licenses files need update? No
   * Is there breaking changes for older versions? No
   * Does this needs documentation? No
   


-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137#discussion_r654284105



##########
File path: flink/flink-scala-parent/src/main/java/org/apache/zeppelin/flink/sql/AbstractStreamSqlJob.java
##########
@@ -99,7 +104,7 @@ private static TableSchema removeTimeAttributes(TableSchema schema) {
   protected abstract String getType();
 
   public String run(String st) throws IOException {
-    Table table = stenv.sqlQuery(st);
+    this.table = stenv.sqlQuery(st);
     String tableName = "UnnamedTable_" +
             "_" + SQL_INDEX.getAndIncrement();
     return run(table, tableName);

Review comment:
       Good point @cuspymd I have set table in this run method as well.




-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
cuspymd commented on a change in pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137#discussion_r650643037



##########
File path: flink/flink1.10-shims/src/main/java/org/apache/zeppelin/flink/Flink110Shims.java
##########
@@ -319,4 +319,22 @@ public Map extractTableConfigOptions() {
     }
     return configOptions;
   }
+
+  @Override
+  public String[] row2String(Object row, Object table, Object tableConfig) {
+    return rowToString((Row) row);
+  }

Review comment:
       Wouldn't it be better to declare the parameter's type as a more specific type than `Object`?
   And it would be better to use either `row2String` or `rowToString` consistently.




-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137#issuecomment-865702474


   Will merge if no more comment


-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
cuspymd commented on a change in pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137#discussion_r654276703



##########
File path: flink/flink-scala-parent/src/main/java/org/apache/zeppelin/flink/sql/AbstractStreamSqlJob.java
##########
@@ -99,7 +104,7 @@ private static TableSchema removeTimeAttributes(TableSchema schema) {
   protected abstract String getType();
 
   public String run(String st) throws IOException {
-    Table table = stenv.sqlQuery(st);
+    this.table = stenv.sqlQuery(st);
     String tableName = "UnnamedTable_" +
             "_" + SQL_INDEX.getAndIncrement();
     return run(table, tableName);

Review comment:
       `this.table` member variable can not be set normally if `run(Table table, String tableName)` method be called directly.
   Is it OK in this scenario?

##########
File path: flink/flink-scala-parent/src/main/java/org/apache/zeppelin/flink/sql/AbstractStreamSqlJob.java
##########
@@ -99,7 +104,7 @@ private static TableSchema removeTimeAttributes(TableSchema schema) {
   protected abstract String getType();
 
   public String run(String st) throws IOException {
-    Table table = stenv.sqlQuery(st);
+    this.table = stenv.sqlQuery(st);
     String tableName = "UnnamedTable_" +
             "_" + SQL_INDEX.getAndIncrement();
     return run(table, tableName);

Review comment:
       `this.table` member variable can not be set normally if `run(Table table, String tableName)` method be called directly. Is it OK in this scenario?




-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137#discussion_r651932441



##########
File path: flink/flink-scala-parent/src/main/java/org/apache/zeppelin/flink/sql/AbstractStreamSqlJob.java
##########
@@ -197,6 +201,18 @@ protected void processRecord(Tuple2<Boolean, Row> change) {
 
   protected abstract String buildResult();
 
+  protected String table2String(List<Row> rows) {
+    StringBuilder builder = new StringBuilder();
+    for (Row row : rows) {
+      String[] fields = flinkShims.row2String(row, table, stenv.getConfig());

Review comment:
       Fixed




-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] cuspymd commented on a change in pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
cuspymd commented on a change in pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137#discussion_r651431593



##########
File path: flink/flink-scala-parent/src/main/java/org/apache/zeppelin/flink/sql/AbstractStreamSqlJob.java
##########
@@ -197,6 +201,18 @@ protected void processRecord(Tuple2<Boolean, Row> change) {
 
   protected abstract String buildResult();
 
+  protected String table2String(List<Row> rows) {
+    StringBuilder builder = new StringBuilder();
+    for (Row row : rows) {
+      String[] fields = flinkShims.row2String(row, table, stenv.getConfig());

Review comment:
       `rowToString`?




-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on a change in pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137#discussion_r651419884



##########
File path: flink/flink1.10-shims/src/main/java/org/apache/zeppelin/flink/Flink110Shims.java
##########
@@ -319,4 +319,22 @@ public Map extractTableConfigOptions() {
     }
     return configOptions;
   }
+
+  @Override
+  public String[] row2String(Object row, Object table, Object tableConfig) {
+    return rowToString((Row) row);
+  }

Review comment:
       Thanks for the review @cuspymd I have rename method name to be rowToString. Regarding the parameter type, it is not possible to use the specific type because the api inconsistent across different flink versions, so I have to use type Object and let each version of `FlinkShims` to do the type casting




-- 
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:
users@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4137: [ZEPPELIN-5392] Timezone is not applied in the timestamp field in flink interpreter

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4137:
URL: https://github.com/apache/zeppelin/pull/4137


   


-- 
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:
users@infra.apache.org