[FLINK-31091][Table SQL/Client] Add Ser/de for Interval types #21945
Conversation
| @@ -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") | |||
There was a problem hiding this comment.
Please also add a test about the serialization/deserialization of the type.
There was a problem hiding this comment.
Added tests for interval types
| @@ -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"}) | |||
There was a problem hiding this comment.
Could you also modify the JSON in the text to make sure others will not break the serialization results in the future?
| @@ -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 | |||
There was a problem hiding this 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
There was a problem hiding this comment.
+1 for the alignment with datatype api, it can be easier to understand
There was a problem hiding this 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?
There was a problem hiding this 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
There was a problem hiding this 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
…son_format and result_info_plain_text_format, add tests for interval types
done |
| @@ -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)); | |||
There was a problem hiding this comment.
We should move the cases into the types. In line 225, we will create test cases with or without nullbility.
snuyanzin
force-pushed
the
flink31091
branch
from
1d0bda4
to
35a38af
Compare
|
@flinkbot run azure |
What is the purpose of the change
The adds support for serialization/deserialization of interval types on sql gateway level to make queries
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:
@Public(Evolving): ( no)Documentation