[FLINK-12288] [table-planner-blink] Bump Calcite dependency to 1.19.0 in blink planner by wuchong · Pull Request #8234 · apache/flink

wuchong · 2019-04-22T09:59:46Z

What is the purpose of the change

Bump Calcite dependency to 1.19.0 in flink-table-planner-blink module.

Brief change log

Upgrade version in flink-table-planner-blink/pom.xml
Update FlinkRelOptTable to implement new interface method.
Update plan tests to adapt to CALCITE-2454 changes.

Verifying this change

no new tests added.

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)
If yes, how is the feature documented? (not applicable)

flinkbot · 2019-04-22T10:00:11Z

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.

Details

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

… in blink planner

wuchong · 2019-04-22T10:05:26Z

...le-planner-blink/src/test/resources/org/apache/flink/table/plan/stream/sql/join/JoinTest.xml

-      +- LogicalProject(i=[$0], k=[$1])
-         +- LogicalTableScan(table=[[MyTable4, source: [TestTableSource(i, k)]]])
+   +- LogicalProject(i=[$0], k=[$1])
+      +- LogicalTableScan(table=[[MyTable4, source: [TestTableSource(i, k)]]])


It seems that Calcite removes LogicalSort in the initial RelNode tree. And it makes sense.

godfreyhe · 2019-04-23T07:53:00Z

Didn't any plans be changed in flink-planner? I note all changed plans are in blink-planner.

wuchong · 2019-04-23T09:26:15Z

Hi @godfreyhe , I only changed Calcite version in blink planner. Flink planner is still using 1.18.0.

twalthr · 2019-04-23T10:23:39Z

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

 			<artifactId>calcite-core</artifactId>
 			<!-- When updating the Calcite version, make sure to update the dependency exclusions -->
-			<version>1.18.0</version>
+			<version>1.19.0</version>


@wuchong Has Calcite changed any dependencies in the last release? Please verify if the shading/exclusions are still correct. Thanks.

Yes, I have verified it. mvn dependency:tree outputs the same tree as before.

twalthr · 2019-04-23T11:12:02Z

@walterddr fyi: this might be useful for the Flink planner Calcite upgrade as well.

walterddr

Thanks for the contribution @wuchong . this would definitely also help on the table-planner side calcite upgrade.

Just left one comment regarding timestamp type. otherwise lgtm

walterddr · 2019-04-23T23:47:57Z

...nk-table-planner-blink/src/test/resources/org/apache/flink/table/plan/batch/sql/CalcTest.xml

    <Resource name="planAfter">
      <![CDATA[
-Calc(select=[a, CAST(1984-07-12) AS b, CAST(14:34:24) AS c, CAST(1984-07-12 14:34:24) AS d], where=[AND(=(b, 1984-07-12), =(c, 14:34:24), =(d, 1984-07-12 14:34:24))])
+Calc(select=[a, CAST(1984-07-12) AS b, CAST(14:34:24) AS c, CAST(1984-07-12 14:34:24:TIMESTAMP(3)) AS d], where=[AND(=(b, 1984-07-12), =(c, 14:34:24), =(d, 1984-07-12 14:34:24:TIMESTAMP(3)))])


I observe something weird upon comparing calcite 1.16/17/18/19 on this (not yet on 19) seems like the timestamp type representation is bouncing here and there a lot. do you think this is solely just the serialization (print string) changes or actually there's some changes to the underlying type? as this might be problematic.

Hi @walterddr , what do you mean "timestamp type representation is bouncing here and there a lot"?

@walterddr I think the reason is the modify of RexLiteral.toJavaString in apache/calcite#1002 , The Timestamp default precision is 3, I think it hasn't changed.

looks like it. what I meant is that I've been changing the test cases string literals back and forth for some of the pass calcite upgrades. Seems like not an issue then.

godfreyhe · 2019-04-24T05:15:25Z

LGTM, +1 to merge

wuchong · 2019-04-24T05:18:02Z

Thanks @godfreyhe .

Hi @walterddr @twalthr @KurtYoung , I will merge this if you have no other comments.

wuchong · 2019-04-24T08:49:56Z

Merging...

…in blink planner This closes apache#8234

rmetzger added the review=description? label Apr 22, 2019

[FLINK-12288] [table-planner-blink] Bump Calcite dependency to 1.19.0…

0fabe2b

… in blink planner

wuchong force-pushed the bump-calcite branch from 602b9af to 0fabe2b Compare April 22, 2019 10:03

wuchong commented Apr 22, 2019

View reviewed changes

rmetzger added the component=TableSQL/Planner label Apr 22, 2019

fix GroupWindowTest

3d2766f

twalthr reviewed Apr 23, 2019

View reviewed changes

walterddr reviewed Apr 23, 2019

View reviewed changes

asfgit closed this in b4a3738 Apr 24, 2019

wuchong deleted the bump-calcite branch April 24, 2019 08:59

tianchen92 pushed a commit to tianchen92/flink that referenced this pull request May 13, 2019

[FLINK-12288][table-planner-blink] Bump Calcite dependency to 1.19.0 …

f7ca3f0

…in blink planner This closes apache#8234

walterddr mentioned this pull request Jun 17, 2019

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

Closed

Conversation

wuchong commented Apr 22, 2019

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

Uh oh!

flinkbot commented Apr 22, 2019

Review Progress

Uh oh!

wuchong Apr 22, 2019

Choose a reason for hiding this comment

Uh oh!

godfreyhe commented Apr 23, 2019

Uh oh!

wuchong commented Apr 23, 2019

Uh oh!

twalthr Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

wuchong Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

twalthr commented Apr 23, 2019

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

walterddr Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

wuchong Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

JingsongLi Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

walterddr Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

godfreyhe commented Apr 24, 2019

Uh oh!

wuchong commented Apr 24, 2019

Uh oh!

wuchong commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants