[FLINK-11921][table] Upgrade to calcite 1.19 #8324

walterddr · 2019-04-30T16:59:33Z

What is the purpose of the change

This change upgrades apache calcite dependencies to 1.19.0 release.

Brief change log

Updated pom.xml file to include calcite 1.19.0
Updated all test cases that includes literals which now includes type in digest.
Updated the 2 pull-in files DateTimeUtils and AuxiliaryConverter and updated the depending calcite JIRAs which will eventually allow us to remove these two pull-ins.
Updated the FlinkLogicalTableSourceScan to include a new computeDigest override, this allows predicator push-down rules like PushFilterIntoTableSourceScanRule and PushProjectIntoTableSourceScanRule to find correct entry in digestToRelMap (see CALCITE-2454, and CALCITE PR #1002)

Verifying this change

This change is already covered by existing tests
Verified via mvn dependeny:tree that no extra dependencies were pulled in.

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

Dependencies (does it add or upgrade a dependency): yes
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
If yes, how is the feature documented? n/a

flinkbot · 2019-04-30T17:00:30Z

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.
- Needs attention by @KurtYoung, @twalthr [PMC], @wuchong [committer]
❓ 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

walterddr · 2019-05-02T04:39:33Z

@flinkbot attention @twalthr

walterddr · 2019-05-09T01:50:51Z

@flinkbot attention @wuchong

walterddr · 2019-05-09T01:51:13Z

@wuchong can you also help take a look at the upgrade? Thanks

walterddr · 2019-05-14T16:51:01Z

@flinkbot attention @KurtYoung

KurtYoung · 2019-05-15T01:41:59Z

@danny0405 Could you help to review this PR?

danny0405 · 2019-05-15T02:19:03Z

@danny0405 Could you help to review this PR?

Sure, i will try to finish this reviewing AFAIK.

danny0405 · 2019-05-15T05:28:55Z

flink-table/flink-table-planner/pom.xml


-				[INFO] +- org.apache.calcite:calcite-core:jar:1.18.0:compile
+				[INFO] +- org.apache.calcite:calcite-core:jar:1.19.0:compile
 				[INFO] |  +- org.apache.calcite.avatica:avatica-core:jar:1.13.0:compile


calcite-avatica 1.15.0 has released recently, do we have plan to upgrade ?

I think we will use whatever version calcite-core is using. thus avatica 1.15.0 will be upgraded when upgrade to calcite 1.20.0

danny0405 · 2019-05-15T05:29:39Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+//		int d = da - (m + 4) * 153 / 5 + 122;
+//		int year = y - 4800 + (m + 2) / 12;
+//		int month = (m + 2) % 12 + 1;
+//		int day = d + 1;


Useless comments

this is explicitly commented out on purpose.

danny0405 · 2019-05-15T05:30:37Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+		int m = (da * 5 + 308) / 153 - 2;
+		// number of days elapsed since day 1 of the month
+		int d = da - (m + 4) * 153 / 5 + 122;
+		int year = y - 4800 + (m + 2) / 12;


I have no idea whether this logic is right or not, let's just add enough Unit tests.

this is copied from avatica 1.14.0 release

I didn't see the necessity to copy a code snippet and comments it out. If we comments out existed code, i agree to keep these comments, but they are very new codes.

danny0405 · 2019-05-15T05:31:10Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+//			+ 365 * y
+//			+ y / 4
+//			- y / 100
+//			+ y / 400


Useless comments

these lines are explicitly comment out on purpose.

Let's just remove these useless comments, we can add them back if we really need them in the future, for current code base, these comments are really confusing.

danny0405 · 2019-05-15T05:33:25Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+	 * higher). */
+	private interface OffsetDateTimeHandler {
+		boolean isOffsetDateTime(Object o);
+		String stringValue(Object o);


Do we really need this interface stringValue ? The two implementations: one is static and one throws an exception, i didn't see why we should keep this.

this is copied from avactica 1.14.0 release

danny0405 · 2019-05-15T05:35:45Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+	/** Returns the value of a {@code OffsetDateTime} as a string. */
+	public static String offsetDateTimeValue(Object o) {
+		return OFFSET_DATE_TIME_HANDLER.stringValue(o);
+	}


Does this mean if we do not have class "java.time.OffsetDateTime", we would always got an exception ?

this is copied from avactica 1.14.0 release

danny0405 · 2019-05-15T05:36:06Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/AuxiliaryConverter.java

-//        return rexBuilder.makeCall(
-//            SqlStdOperatorTable.PLUS, e,
-//            ((RexCall) groupCall).operands.get(2));
-//      default:


Let's just keep these comments.

good point. I should kept the comments. thanks for reminder.

danny0405 · 2019-05-15T05:37:55Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+	private static int getIso8601WeekNumber(int julian, int year, int month, int day) {
+		long fmofw = firstMondayOfFirstWeek(year);
+		if (month == 12 && day > 28) {
+			if (31 - day + 4 > 7 - ((int) floorMod(julian, 7) + 1)


Although the Calcite bug may be fixed in future, lets's add UT for these transformation functions for our current code base.

these lines are copied from avatica 1.14.0 release

walterddr

thanks for the review @danny0405 . please see my comments inline. I will update the change accordingly.

walterddr · 2019-05-15T16:25:44Z

flink-table/flink-table-planner/pom.xml


-				[INFO] +- org.apache.calcite:calcite-core:jar:1.18.0:compile
+				[INFO] +- org.apache.calcite:calcite-core:jar:1.19.0:compile
 				[INFO] |  +- org.apache.calcite.avatica:avatica-core:jar:1.13.0:compile


I think we will use whatever version calcite-core is using. thus avatica 1.15.0 will be upgraded when upgrade to calcite 1.20.0

walterddr · 2019-05-15T16:27:32Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

@@ -26,7 +26,8 @@
 import java.util.TimeZone;

 /*
- * THIS FILE HAS BEEN COPIED FROM THE APACHE CALCITE PROJECT UNTIL CALCITE-1884 IS FIXED.
+ * THIS FILE HAS BEEN COPIED FROM THE APACHE CALCITE PROJECT UNTIL CALCITE-2989 IS FIXED.


@danny0405, per the Flink table contribution guideline. when we alter Calcite classes, we need to have a [CALCITE-xxxx] ticket explained the future consolidation plan. as well as explicitly comment out the changes made on top of the copied Calcite classes.

This class is copied from avatica 1.14.0 release and made only the following lines of changes.
please refer to CALCITE-2989 / FLINK-11935 for more info

walterddr · 2019-05-15T16:28:32Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+//		int d = da - (m + 4) * 153 / 5 + 122;
+//		int year = y - 4800 + (m + 2) / 12;
+//		int month = (m + 2) % 12 + 1;
+//		int day = d + 1;


this is explicitly commented out on purpose.

walterddr · 2019-05-15T16:29:09Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+		int m = (da * 5 + 308) / 153 - 2;
+		// number of days elapsed since day 1 of the month
+		int d = da - (m + 4) * 153 / 5 + 122;
+		int year = y - 4800 + (m + 2) / 12;


this is copied from avatica 1.14.0 release

walterddr · 2019-05-15T16:29:26Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+	private static int getIso8601WeekNumber(int julian, int year, int month, int day) {
+		long fmofw = firstMondayOfFirstWeek(year);
+		if (month == 12 && day > 28) {
+			if (31 - day + 4 > 7 - ((int) floorMod(julian, 7) + 1)


these lines are copied from avatica 1.14.0 release

walterddr · 2019-05-15T16:30:33Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+//			+ 365 * y
+//			+ y / 4
+//			- y / 100
+//			+ y / 400


these lines are explicitly comment out on purpose.

walterddr · 2019-05-15T16:30:50Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+	/** Returns the value of a {@code OffsetDateTime} as a string. */
+	public static String offsetDateTimeValue(Object o) {
+		return OFFSET_DATE_TIME_HANDLER.stringValue(o);
+	}


this is copied from avactica 1.14.0 release

walterddr · 2019-05-15T16:30:54Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/avatica/util/DateTimeUtils.java

+	 * higher). */
+	private interface OffsetDateTimeHandler {
+		boolean isOffsetDateTime(Object o);
+		String stringValue(Object o);


this is copied from avactica 1.14.0 release

walterddr · 2019-05-15T16:31:20Z

...k-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/AuxiliaryConverter.java

-//        return rexBuilder.makeCall(
-//            SqlStdOperatorTable.PLUS, e,
-//            ((RexCall) groupCall).operands.get(2));
-//      default:


good point. I should kept the comments. thanks for reminder.

walterddr · 2019-05-16T15:29:40Z

thanks for the pointer @danny0405 actually i agree with the guideline @twalthr setup previously to left the code commented out not removed, and kept the java class in sync with the one used in calcite except for the lines we changed. because

commented it out makes it easier for future contributor to maintain the code - I had to dig deep to understand which part of the code is needed to comment out by running multiple tests because that previous removal was not traceable.
ideally speaking we shouldn't copy and alter calcite/avatica code, this is only to unblock the release until a specific CALCITE JIRA has been fix - the previous version of the DateTimeUtils has not been updated since 3,4 versions ago. I think it make sense to upgrade it to the one that currently calcite-core depends on.

What do you guys think? I am open up for any changes to the policy as long as we all agree on the process.

1. adding in CALCITE pull in file comments and update modification notes 2. also fix CALCITE-2454, PR apache#1002 and fix scalastyle

walterddr · 2019-05-24T16:39:26Z

@fhueske @twalthr could you take a look?

danny0405 · 2019-05-25T05:07:38Z

CALCITE-3055 is a vital regression for VolcanoPlanner, it would be fixed in 1.20.0, i think we should skip 1.19.0 version Calcite, and use 1.20.0 directly.

walterddr · 2019-05-28T19:53:07Z

@danny0405 hmm. I think that's probably a good idea

Considering the light-weighted changes I did for this PR. I suggest we commit this PR's change first and then continue to move on releasing 1.9 with Calcite 1.20.0 if possible.

Or I can also commit on top of this PR for 1.20.0, do you know how far away CALCITE-3055 is? Looks like the PR is small and should be merged soon?

danny0405 · 2019-05-29T08:48:37Z

@danny0405 hmm. I think that's probably a good idea

Considering the light-weighted changes I did for this PR. I suggest we commit this PR's change first and then continue to move on releasing 1.9 with Calcite 1.20.0 if possible.

Or I can also commit on top of this PR for 1.20.0, do you know how far away CALCITE-3055 is? Looks like the PR is small and should be merged soon?

@walterddr CALCITE-3055 has been merged, and Calcite version 1.20 may be released around Mid June, let's just skip 1.19 and use 1.20 directly.

walterddr · 2019-06-07T17:17:36Z

close this pull request and recreate new one once calcite 1.20 releases

danny0405 · 2019-06-30T05:41:19Z

@walterddr The Apache Calcite 1.20.0 has been released, can you do the upgrade now ?

walterddr · 2019-06-30T15:39:34Z

thanks for the reminder @danny0405. will try to take a look and create another PR asap.

rmetzger added review=description? component=TableSQL/Planner labels Apr 30, 2019

rmetzger requested a review from twalthr May 2, 2019 04:40

rmetzger requested a review from wuchong May 11, 2019 11:41

danny0405 reviewed May 15, 2019

View reviewed changes

walterddr commented May 15, 2019

View reviewed changes

Rong Rong added 2 commits May 21, 2019 08:44

initial commit to upgrade to calcite 1.19, additionally:

2bc4ce6

1. adding in CALCITE pull in file comments and update modification notes 2. also fix CALCITE-2454, PR apache#1002 and fix scalastyle

address diff comments

a835ee0

walterddr force-pushed the FLINK-11921 branch from 271550b to a835ee0 Compare May 21, 2019 15:46

walterddr closed this Jun 7, 2019

walterddr mentioned this pull request Jun 17, 2019

[hotfix][FLINK-11120][table]fix the bug of timestampadd handles time #8746

Closed

walterddr deleted the FLINK-11921 branch July 10, 2019 15:31

[FLINK-11921][table] Upgrade to calcite 1.19 #8324

[FLINK-11921][table] Upgrade to calcite 1.19 #8324

Conversation

walterddr commented Apr 30, 2019 • edited

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

flinkbot commented Apr 30, 2019 • edited

Review Progress

walterddr commented May 2, 2019

walterddr commented May 9, 2019

walterddr commented May 9, 2019

walterddr commented May 14, 2019

KurtYoung commented May 15, 2019

danny0405 commented May 15, 2019

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

danny0405 May 16, 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

danny0405 May 15, 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

walterddr 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

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

walterddr commented May 16, 2019 • edited

walterddr commented May 24, 2019

danny0405 commented May 25, 2019

walterddr commented May 28, 2019 • edited

danny0405 commented May 29, 2019

walterddr commented Jun 7, 2019

danny0405 commented Jun 30, 2019

walterddr commented Jun 30, 2019

walterddr commented Apr 30, 2019 •

edited

flinkbot commented Apr 30, 2019 •

edited

danny0405 May 16, 2019 •

edited

danny0405 May 15, 2019 •

edited

walterddr commented May 16, 2019 •

edited

walterddr commented May 28, 2019 •

edited