[FLINK-12844][table-planner-blink] Use default conversion class LocalDate/LocalTime/LocalDateTime for DateType/TimeType/TimestampType in blink #8762

JingsongLi · 2019-06-17T08:52:31Z

What is the purpose of the change

Now we still use java.sql.Timestamp to be default conversion class for TimestampType.
We should use design of new Type System to use java.time.Local***.
NOTE, This may affect user behaviour:
eg: UDF with eval(Object o), now will pass a java.time.Local*** instead of java.sql..
Compatibility method: use DataType.bridgedTo(java.sql..class);

Verifying this change

ut

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: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)

JingsongLi · 2019-06-17T08:53:07Z

This PR is rebased on #8757

flinkbot · 2019-06-17T08:55:01Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java

...common/src/main/java/org/apache/flink/table/types/utils/LegacyTypeInfoDataTypeConverter.java

...table-planner-blink/src/main/scala/org/apache/flink/table/codegen/CodeGeneratorContext.scala

...planner-blink/src/main/scala/org/apache/flink/table/sources/tsextractors/ExistingField.scala

tsreaper

LGTM

flinkbot · 2019-07-06T12:21:02Z

CI report for commit 772745b: ERROR Build

wuchong

Thanks @JingsongLi . This is a great improvement for blink planner to have a better handling on timestamps. 👍

I left some comments below.

wuchong · 2019-07-08T02:55:39Z

...common/src/main/java/org/apache/flink/table/types/utils/LegacyTypeInfoDataTypeConverter.java

@@ -99,6 +102,9 @@
 		addMapping(Types.FLOAT, DataTypes.FLOAT().bridgedTo(Float.class));
 		addMapping(Types.DOUBLE, DataTypes.DOUBLE().bridgedTo(Double.class));
 		addMapping(Types.BIG_DEC, createLegacyType(LogicalTypeRoot.DECIMAL, Types.BIG_DEC));
+		addMapping(LocalTimeTypeInfo.LOCAL_DATE, DataTypes.DATE().bridgedTo(LocalDate.class));
+		addMapping(LocalTimeTypeInfo.LOCAL_TIME, DataTypes.TIME(0).bridgedTo(LocalTime.class));
+		addMapping(LocalTimeTypeInfo.LOCAL_DATE_TIME, DataTypes.TIMESTAMP(3).bridgedTo(LocalDateTime.class));


LocalTimeTypeInfo => Types

wuchong · 2019-07-08T03:17:39Z

...table-planner-blink/src/main/scala/org/apache/flink/table/codegen/CodeGeneratorContext.scala

@@ -500,7 +527,7 @@ class CodeGeneratorContext(val tableConfig: TableConfig) {
    * Adds a reusable TimeZone to the member area of the generated class.
    */
  def addReusableTimeZone(): String = {


Rename this method to addReusableLocalTimeZone or addReusableDefaultTimeZone? To have a better understanding from the method name that the time zone is not from TableConfig.getTimeZone but is JVM default time zone.

I think it should read tableConfig.getTimeZone and used by next support for Timestamp with local zone.
addReusableLocalDateTime should just read TimeZone.getDefault.

@JingsongLi Maybe we should update TableConfig and use ZoneId instead of the old TimeZone class. I haven't looked in to the details but I read:
A {@code ZoneId} is used to identify the rules used to convert between an {@link Instant} and a {@link LocalDateTime}.

@twalthr good suggestion, +1, ZoneId for java 8 classes.
TimeZone is an outdated class and troublesome.

wuchong · 2019-07-08T03:28:09Z

...scala/org/apache/flink/table/plan/nodes/physical/stream/StreamExecGroupWindowAggregate.scala

@@ -302,7 +302,7 @@ class StreamExecGroupWindowAggregate(
    val builder = WindowOperatorBuilder
      .builder()
      .withInputFields(inputFields.toArray)
-    val timeZoneOffset = -config.getTimeZone.getOffset(Calendar.ZONE_OFFSET)
+    val timeZoneOffset = 0


Could you also remove the following constructor parameter for time zone? I think they are not needed anymore.

wuchong · 2019-07-08T03:34:20Z

...able-planner-blink/src/test/scala/org/apache/flink/table/expressions/TemporalTypesTest.scala

@@ -399,6 +400,7 @@ class TemporalTypesTest extends ExpressionTestBase {
    )
  }

+  @Ignore // TODO support timestamp with local time zone


Will we support this before 1.9 release? Is that mean, the timeZone option in TableConfig will not work for blink planner?

In next PR to support timestamp with local zone.

wuchong · 2019-07-08T03:49:07Z

...e-runtime-blink/src/main/java/org/apache/flink/table/runtime/functions/SqlDateTimeUtils.java

@@ -160,24 +160,16 @@ public TimeZone getNewInstance(String tz) {
 	 * type used for UDF parameters ({@link java.sql.Timestamp}).
 	 *
 	 * <p>The internal long represents the time milliseconds since January 1, 1970, 00:00:00 GMT.
-	 * So we don't need to take TimeZone into account.
+	 * we need a TimeZone.


Remove this comment or add more details why we need time zone explicitly ?

I'll change it to calcite's implementation.

wuchong · 2019-07-08T03:51:15Z

...table-planner-blink/src/main/scala/org/apache/flink/table/codegen/calls/BuiltInMethods.scala

@@ -232,24 +237,43 @@ object BuiltInMethods {
  val NOW_OFFSET = Types.lookupMethod(
    classOf[SqlDateTimeUtils], "now", classOf[Long])

-  val DATE_FORMAT_STRING_STRING_STRING = Types.lookupMethod(
+  val DATE_FORMAT_STRING_STRING_STRING_TIME_ZONE = Types.lookupMethod(


We can remove all the TIME_ZONE suffix methods in this file. They are not used anymore.

I think them will be used in next PR to support timestamp with local zone.

wuchong · 2019-07-08T03:54:00Z

...able-runtime-blink/src/main/java/org/apache/flink/table/dataformat/DataFormatConverters.java

@@ -128,8 +135,21 @@
 	 *                   lost its specific Java format. Only DataType retains all its
 	 *                   Java format information.
 	 */
-	@SuppressWarnings("unchecked")
+	@Deprecated


Please add a javadoc to declare which method should we use instead.
And we should replace all the places where this method is used with the new method?

wuchong · 2019-07-08T03:57:18Z

...able-runtime-blink/src/main/java/org/apache/flink/table/dataformat/DataFormatConverters.java

+	 * @param context context for converter.
+	 */
+	@SuppressWarnings("unchecked")
+	public static DataFormatConverter getConverterForDataType(DataType originDataType, Context context) {


Can we pass a TimeZone instead of Context ? Will we add more parameters in the future?
To me, the way to construct a context is verbose.

wuchong · 2019-07-08T04:01:23Z

...able-runtime-blink/src/main/java/org/apache/flink/table/dataformat/DataFormatConverters.java

 	public static DataFormatConverter getConverterForDataType(DataType originDataType) {
+		return getConverterForDataType(originDataType, new Context(DEFAULT_TIME_ZONE));


Shouldn't we use TableConfig.getTimeZone as the conversion time zone instead of local time zone in flink framework?

After offline discussion with @wuchong , java.sql.Timestamp is a special format, valueOf and toString in sql.Timestamp will read the TimeZone.getDefault, So there, we should just use TimeZone.getDefault to correct valueOf and toString.

KurtYoung · 2019-07-08T05:21:46Z

flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/api/Types.scala

@@ -23,7 +23,7 @@ import org.apache.flink.api.java.typeutils.{MapTypeInfo, MultisetTypeInfo, Objec
 import org.apache.flink.table.typeutils.TimeIntervalTypeInfo
 import org.apache.flink.types.Row

-import _root_.java.{lang, math, sql, util}
+import _root_.java.{lang, math, sql, time, util}


I think this file can be deleted

Yes, I will delete it as first commit.

flinkbot · 2019-07-08T07:47:44Z

CI report for commit 3bdf6c3: SUCCESS Build

JingsongLi · 2019-07-08T09:13:21Z

@wuchong @KurtYoung Updated, PTAL

flinkbot · 2019-07-08T10:01:52Z

CI report for commit 5f0e633: FAILURE Build

flinkbot · 2019-07-08T11:30:58Z

CI report for commit 7fe600a: FAILURE Build

flinkbot · 2019-07-08T11:31:02Z

CI report for commit 869712b: FAILURE Build

…Date/LocalTime/LocalDateTime for DateType/TimeType/TimestampType in blink

flinkbot · 2019-07-08T12:31:46Z

CI report for commit da4c415: FAILURE Build

rmetzger added review=description? component=TableSQL/Planner labels Jun 17, 2019

twalthr requested changes Jun 17, 2019

View reviewed changes

flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java Outdated Show resolved Hide resolved

JingsongLi mentioned this pull request Jun 21, 2019

[FLINK-12888] [table-planner-blink] Introduce planner rule to push filter into TableSource #8782

Closed

JingsongLi force-pushed the time branch 2 times, most recently from ec88425 to 3dfcf7b Compare June 24, 2019 02:52

JingsongLi force-pushed the time branch 3 times, most recently from 276bcfe to 58d7331 Compare July 5, 2019 06:02

tsreaper reviewed Jul 5, 2019

View reviewed changes

JingsongLi force-pushed the time branch 2 times, most recently from 85bd1dd to c833a38 Compare July 5, 2019 08:13

tsreaper approved these changes Jul 5, 2019

View reviewed changes

JingsongLi force-pushed the time branch from 772745b to 3bdf6c3 Compare July 8, 2019 01:50

wuchong reviewed Jul 8, 2019

View reviewed changes

KurtYoung reviewed Jul 8, 2019

View reviewed changes

JingsongLi force-pushed the time branch from 3bdf6c3 to 5f0e633 Compare July 8, 2019 09:10

JingsongLi force-pushed the time branch from 869712b to 7fe600a Compare July 8, 2019 10:08

JingsongLi added 4 commits July 8, 2019 19:54

[hotfix] Remove blink Types class

cd09299

[FLINK-12844][table-planner-blink] Use default conversion class Local…

a36050a

…Date/LocalTime/LocalDateTime for DateType/TimeType/TimestampType in blink

Fix wuchong's comments

4e20064

Fix compile

78da048

Fix cases

da4c415

JingsongLi force-pushed the time branch from 7fe600a to da4c415 Compare July 8, 2019 11:57

KurtYoung closed this in 2e09d94 Jul 8, 2019

JingsongLi deleted the time branch July 10, 2019 08:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-12844][table-planner-blink] Use default conversion class LocalDate/LocalTime/LocalDateTime for DateType/TimeType/TimestampType in blink #8762

[FLINK-12844][table-planner-blink] Use default conversion class LocalDate/LocalTime/LocalDateTime for DateType/TimeType/TimestampType in blink #8762

JingsongLi commented Jun 17, 2019

JingsongLi commented Jun 17, 2019

flinkbot commented Jun 17, 2019 •

edited

tsreaper left a comment

flinkbot commented Jul 6, 2019

wuchong left a comment

wuchong Jul 8, 2019

JingsongLi Jul 8, 2019

wuchong Jul 8, 2019

JingsongLi Jul 8, 2019

twalthr Jul 8, 2019 •

edited

JingsongLi Jul 8, 2019

wuchong Jul 8, 2019

JingsongLi Jul 8, 2019

wuchong Jul 8, 2019

JingsongLi Jul 8, 2019

wuchong Jul 8, 2019

JingsongLi Jul 8, 2019

wuchong Jul 8, 2019

JingsongLi Jul 8, 2019

wuchong Jul 8, 2019

wuchong Jul 8, 2019

wuchong Jul 8, 2019

JingsongLi Jul 8, 2019

KurtYoung Jul 8, 2019

JingsongLi Jul 8, 2019

flinkbot commented Jul 8, 2019

JingsongLi commented Jul 8, 2019

flinkbot commented Jul 8, 2019

flinkbot commented Jul 8, 2019

flinkbot commented Jul 8, 2019

flinkbot commented Jul 8, 2019

		public static DataFormatConverter getConverterForDataType(DataType originDataType) {
		return getConverterForDataType(originDataType, new Context(DEFAULT_TIME_ZONE));

[FLINK-12844][table-planner-blink] Use default conversion class LocalDate/LocalTime/LocalDateTime for DateType/TimeType/TimestampType in blink #8762

[FLINK-12844][table-planner-blink] Use default conversion class LocalDate/LocalTime/LocalDateTime for DateType/TimeType/TimestampType in blink #8762

Conversation

JingsongLi commented Jun 17, 2019

What is the purpose of the change

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

JingsongLi commented Jun 17, 2019

flinkbot commented Jun 17, 2019 • edited

Review Progress

tsreaper left a comment

Choose a reason for hiding this comment

flinkbot commented Jul 6, 2019

wuchong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twalthr Jul 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flinkbot commented Jul 8, 2019

JingsongLi commented Jul 8, 2019

flinkbot commented Jul 8, 2019

flinkbot commented Jul 8, 2019

flinkbot commented Jul 8, 2019

flinkbot commented Jul 8, 2019

flinkbot commented Jun 17, 2019 •

edited

twalthr Jul 8, 2019 •

edited