You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2023/02/15 21:35:43 UTC

[GitHub] [flink] snuyanzin opened a new pull request, #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

snuyanzin opened a new pull request, #21945:
URL: https://github.com/apache/flink/pull/21945

   ## What is the purpose of the change
   
   The adds support for serialization/deserialization of interval types on sql gateway level to make queries
   ```sql
   SELECT INTERVAL '1' DAY;
   SELECT INTERVAL '2' YEAR;
   ```
   working again.
   
   Also it hardens modifiers for tests since in case of junit5 package private is enough
   
   ## Verifying this change
   
   There were added tests for month year and date time intervals
   
   ## Does this pull request potentially affect one of the following parts:
   
     - Dependencies (does it add or upgrade a dependency): ( no)
     - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: ( no)
     - The serializers: (yes )
     - The runtime per-record code paths (performance sensitive): ( no)
     - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: ( no )
     - The S3 file system connector: ( no)
   
   ## Documentation
   
     - Does this pull request introduce a new feature? ( no)
     - If yes, how is the feature documented? (not applicable)
   


-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1111999618


##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/serde/ResultInfoJsonSerDeTest.java:
##########
@@ -123,7 +131,7 @@ public void testResultInfoSerDeWithNullValues(RowFormat rowFormat) throws Except
 
     @ParameterizedTest
     @ValueSource(strings = {"result_info_json_format.txt", "result_info_plain_text_format.txt"})

Review Comment:
   added



-- 
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

[GitHub] [flink] fsk119 commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "fsk119 (via GitHub)" <gi...@apache.org>.
fsk119 commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1112502607


##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerializer.java:
##########
@@ -87,6 +89,10 @@ public final class LogicalTypeJsonSerializer extends StdSerializer<LogicalType>
     public static final String FIELD_NAME_CLASS = "class";
     public static final String FIELD_NAME_SERIALIZER = "serializer";
 
+    // INTERVAL

Review Comment:
   `INTERVAL` is a kind of `DataType` in the doc[1]. I think the API here aims at the DataType rather than LogicalType in the design[2].
   
   [1] https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/types/#list-of-data-types
   [2] https://cwiki.apache.org/confluence/display/FLINK/FLIP-91%3A+Support+SQL+Gateway#FLIP91:SupportSQLGateway-SerializeanddeserializetheResultSet



-- 
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

[GitHub] [flink] fsk119 merged pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "fsk119 (via GitHub)" <gi...@apache.org>.
fsk119 merged PR #21945:
URL: https://github.com/apache/flink/pull/21945


-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1112846597


##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerializer.java:
##########
@@ -87,6 +89,10 @@ public final class LogicalTypeJsonSerializer extends StdSerializer<LogicalType>
     public static final String FIELD_NAME_CLASS = "class";
     public static final String FIELD_NAME_SERIALIZER = "serializer";
 
+    // INTERVAL

Review Comment:
   ~~updated usage of `precision` with usage of `yearPrecision` and `dayPrecision` based on offline discussion in slack~~
   
   After another round of discussion it was decided not to do it



-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1111999321


##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerDeTest.java:
##########
@@ -71,23 +72,32 @@
 
 /** Tests for {@link LogicalType} serialization and deserialization. */
 @Execution(CONCURRENT)
-public class LogicalTypeJsonSerDeTest {
+class LogicalTypeJsonSerDeTest {
 
     private final ObjectMapper mapper = buildObjectMapper();
 
     @ParameterizedTest
     @MethodSource("generateTestData")

Review Comment:
   Added tests for interval types



-- 
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

[GitHub] [flink] flinkbot commented on pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #21945:
URL: https://github.com/apache/flink/pull/21945#issuecomment-1432071120

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "ecb22e32c2c9d129d2e20311140fbc9c9c0b166c",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "ecb22e32c2c9d129d2e20311140fbc9c9c0b166c",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * ecb22e32c2c9d129d2e20311140fbc9c9c0b166c UNKNOWN
   
   <details>
   <summary>Bot commands</summary>
     The @flinkbot bot supports the following commands:
   
    - `@flinkbot run azure` re-run the last Azure build
   </details>


-- 
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

[GitHub] [flink] lincoln-lil commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "lincoln-lil (via GitHub)" <gi...@apache.org>.
lincoln-lil commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1111621068


##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerializer.java:
##########
@@ -87,6 +89,10 @@ public final class LogicalTypeJsonSerializer extends StdSerializer<LogicalType>
     public static final String FIELD_NAME_CLASS = "class";
     public static final String FIELD_NAME_SERIALIZER = "serializer";
 
+    // INTERVAL

Review Comment:
   +1 for the alignment with datatype api, it can be easier to understand 



-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1111855663


##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerializer.java:
##########
@@ -87,6 +89,10 @@ public final class LogicalTypeJsonSerializer extends StdSerializer<LogicalType>
     public static final String FIELD_NAME_CLASS = "class";
     public static final String FIELD_NAME_SERIALIZER = "serializer";
 
+    // INTERVAL

Review Comment:
   From one side I'm ok to add `upper/lowerResolution`
   however from another side current ser/de is based on `LogicalTypeRoot`. And `INTERVAL` is NOT a `LogicalTypeRoot`... it is just a `LogicalTypeFamily`. So it will require extra parsing to differentiate between `YearMonthIntervalType` and `DayTimeIntervalType`.
   Or am I missing something here?



-- 
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

[GitHub] [flink] fsk119 commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "fsk119 (via GitHub)" <gi...@apache.org>.
fsk119 commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1112507939


##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerDeTest.java:
##########
@@ -219,6 +231,25 @@ private static List<LogicalType> generateTestData() {
         // ignore nullable for NullType
         testTypes.add(new NullType());
 
+        // interval types
+        testTypes.add(new YearMonthIntervalType(YearMonthIntervalType.YearMonthResolution.MONTH));

Review Comment:
   We should move the cases into the `types`. In line 225, we will create test cases with or without nullbility.



-- 
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

[GitHub] [flink] snuyanzin commented on pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #21945:
URL: https://github.com/apache/flink/pull/21945#issuecomment-1438475319

   @flinkbot run azure


-- 
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

[GitHub] [flink] fsk119 commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "fsk119 (via GitHub)" <gi...@apache.org>.
fsk119 commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1111508444


##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerializer.java:
##########
@@ -87,6 +89,10 @@ public final class LogicalTypeJsonSerializer extends StdSerializer<LogicalType>
     public static final String FIELD_NAME_CLASS = "class";
     public static final String FIELD_NAME_SERIALIZER = "serializer";
 
+    // INTERVAL

Review Comment:
   I just wonder whether it's better to serialize the INTERVAL type as follows:
   
   ```
   {
       "type": "INTERVAL",
       "upperResolution": {
             "precision": ...,
             "intervalUnit": "..."
       },
       "lowerResolution": ...
   }
   ```
   
   So we can align with the API in `DataTypes#INTERVAL`. WDYT? @twalthr @lincoln-lil 



##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerDeTest.java:
##########
@@ -71,23 +72,32 @@
 
 /** Tests for {@link LogicalType} serialization and deserialization. */
 @Execution(CONCURRENT)
-public class LogicalTypeJsonSerDeTest {
+class LogicalTypeJsonSerDeTest {
 
     private final ObjectMapper mapper = buildObjectMapper();
 
     @ParameterizedTest
     @MethodSource("generateTestData")

Review Comment:
   Please also add a test about the serialization/deserialization of the type.



##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/serde/ResultInfoJsonSerDeTest.java:
##########
@@ -123,7 +131,7 @@ public void testResultInfoSerDeWithNullValues(RowFormat rowFormat) throws Except
 
     @ParameterizedTest
     @ValueSource(strings = {"result_info_json_format.txt", "result_info_plain_text_format.txt"})

Review Comment:
   Could you also modify the JSON in the text to make sure others will not break the serialization results 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

[GitHub] [flink] snuyanzin commented on pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #21945:
URL: https://github.com/apache/flink/pull/21945#issuecomment-1437074621

   >Could you add an ITCase about the case in the jira to verify we solve the problem?
   
   done


-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1112846597


##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerializer.java:
##########
@@ -87,6 +89,10 @@ public final class LogicalTypeJsonSerializer extends StdSerializer<LogicalType>
     public static final String FIELD_NAME_CLASS = "class";
     public static final String FIELD_NAME_SERIALIZER = "serializer";
 
+    // INTERVAL

Review Comment:
   updated usage of `precision` with usage of `yearPrecision` and `dayPrecision` based on offline discussion in slack



-- 
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

[GitHub] [flink] snuyanzin commented on a diff in pull request #21945: [FLINK-31091][Table SQL/Client] Add Ser/de for Interval types

Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #21945:
URL: https://github.com/apache/flink/pull/21945#discussion_r1112572254


##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/rest/serde/LogicalTypeJsonSerDeTest.java:
##########
@@ -219,6 +231,25 @@ private static List<LogicalType> generateTestData() {
         // ignore nullable for NullType
         testTypes.add(new NullType());
 
+        // interval types
+        testTypes.add(new YearMonthIntervalType(YearMonthIntervalType.YearMonthResolution.MONTH));

Review Comment:
   done



-- 
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