[FLINK-9134] [table] upgrade Calcite dependency to 1.17 #6484

suez1224 · 2018-08-03T07:00:20Z

What is the purpose of the change

upgrade Calcite dependency to 1.17

Brief change log

incoporate calcite change in DATETIME_PLUS operator.
fix unittests.
fix SqlToRelConverter in Flink repo.

Verifying this change

This change is already covered by existing tests

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)

twalthr

Thank you @suez1224. I added some comments.

twalthr · 2018-08-03T09:55:09Z

...raries/flink-table/src/main/scala/org/apache/flink/table/catalog/ExternalCatalogSchema.scala

@@ -20,7 +20,10 @@ package org.apache.flink.table.catalog

 import java.util.{Collection => JCollection, Collections => JCollections, LinkedHashSet => JLinkedHashSet, Set => JSet}

+import com.google.common.collect.ImmutableSet
+import jdk.nashorn.internal.ir.annotations.Immutable


Remove unused import.

twalthr · 2018-08-03T09:55:58Z

...raries/flink-table/src/main/scala/org/apache/flink/table/catalog/ExternalCatalogSchema.scala

+
+  override def getType(s: String): RelProtoDataType = null
+
+  override def getTypeNames: JSet[String] = ImmutableSet.of()


We should try to avoid guava. Use java.util.Collections#unmodifiableSet instead.

twalthr · 2018-08-03T09:58:35Z

flink-libraries/flink-table/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java

+import static org.apache.calcite.sql.SqlUtil.stripAs;
+
+/**
+ * THIS FILE HAS BEEN COPIED FROM THE APACHE CALCITE PROJECT UNTIL CALCITE-2440 is fixed.


Could you mention which test would fail if we remove this class?

twalthr · 2018-08-03T10:26:56Z

...main/scala/org/apache/flink/table/plan/rules/common/WindowAggregateReduceFunctionsRule.scala

-
-    // create a LogicalCalc that computes the complex aggregates and forwards the window properties
-    relBuilder.project(exprs, oldAgg.getRowType.getFieldNames)
+  override def newCalcRel(relBuilder: RelBuilder, rowType: RelDataType, exprs: util.List[RexNode]) {


Please format the code again and add some inline comments to know what is happening here. Add :Unit as well.

twalthr · 2018-08-03T10:37:24Z

...aries/flink-table/src/test/scala/org/apache/flink/table/api/stream/sql/GroupWindowTest.scala

@@ -161,7 +161,7 @@ class GroupWindowTest extends TableTestBase {
            "rowtime('w$) AS w$rowtime",
            "proctime('w$) AS w$proctime")
        ),
-        term("select", "EXPR$0", "DATETIME_PLUS(w$end, 60000) AS EXPR$1")
+        term("select", "EXPR$0", "+(w$end, 60000) AS EXPR$1")


Do you know the reason for this change? Why is DATETIME_PLUS now equivalent to +? It is not deprecated. Can we replace all occurrences of this constant in our code base?

@twalthr code relevant is around CodeGenerator#L769 and I think + is equivalent to DATETIME_PLUS here and below.
However, I see no reason for this change. Not only DATETIME_PLUS works as expected, but also DATETIME_PLUS is more clear than bare +. How can one know (w$end, 60000) is about DATETIME without take a near look.

FYI, code around CodeGenerator#L769 like:

case PLUS | DATETIME_PLUS if isTemporal(resultType) => val left = operands.head val right = operands(1) requireTemporal(left) requireTemporal(right) generateTemporalPlusMinus(plus = true, nullCheck, left, right, config)

This is changed in https://issues.apache.org/jira/browse/CALCITE-2188. DATETIME_PLUS operator is not equal to PLUS operator, they just somehow share the same name "+" in plan after the PR. I can reach out to Calcite community to see if it makes sense to revert it back, I agree that using "DATETIME_PLUS" is more clear.

Thanks for the research. @suez1224 if you find time, you could discuss this in the Calcite community again. I find it a bit confusing to have the same same string for different SQL operators. But it is not a blocker for this PR.

- incoporate calcite change in DATETIME_PLUS operator. - fix unittests. - fix SqlToRelConverter in Flink repo.

suez1224 · 2018-08-04T07:29:00Z

@twalthr addressed the comments. Let me know what you think.

hequn8128 · 2018-08-04T07:44:07Z

@suez1224 Hi, thanks for your PR. Only one question from my side: do we have any good ideas or plans about how to remove AuxiliaryConverter since calcite-1761 is fixed.

twalthr · 2018-08-06T07:53:37Z

@hequn8128 I'm not aware of any plans. Actually, I'm wondering if the community could just remove the logic in AuxiliaryConverter and just return the plan unmodified. Every project could implement their own conversion logic.

twalthr

Thanks for the update @suez1224.
+1 from my side to merge this.

suez1224 · 2018-08-06T22:54:03Z

merged

twalthr · 2018-08-07T09:10:13Z

@suez1224 please include the issue in commit such as

[FLINK-9134] [table] Upgrade Calcite dependency to 1.17

Otherwise no one knows what happened in this commit:

commit eb3a1741032795ab8db76b40c3614820a2529166 (HEAD -> master, upstream/master)
Author: Shuyi Chen <shuyi@uber.com>
Date:   Fri Jul 27 17:27:05 2018 -0700

    - remove AggregateReduceFunctionsRule copy in Flink repo.
    - incoporate calcite change in DATETIME_PLUS operator.
    - fix unittests.
    - fix SqlToRelConverter in Flink repo.

twalthr changed the title ~~[FLINK-9134][Table API & SQL] upgrade Calcite dependency to 1.17~~ [FLINK-9134] [table] upgrade Calcite dependency to 1.17 Aug 3, 2018

twalthr requested changes Aug 3, 2018

View reviewed changes

twalthr mentioned this pull request Aug 3, 2018

[FLINK-9473] [table] Added new methods into ExternalCatalogSchema based on interface Schema changes in Calcite #6099

Closed

- remove AggregateReduceFunctionsRule copy in Flink repo.

76e12da

- incoporate calcite change in DATETIME_PLUS operator. - fix unittests. - fix SqlToRelConverter in Flink repo.

suez1224 force-pushed the calcite-1.17 branch from daa3e64 to 76e12da Compare August 4, 2018 07:28

twalthr approved these changes Aug 6, 2018

View reviewed changes

suez1224 closed this Aug 6, 2018

rmetzger added the component=API/TableSQL label Mar 18, 2019

flinkbot added component=TableSQL/API and removed component=API/TableSQL labels Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-9134] [table] upgrade Calcite dependency to 1.17 #6484

[FLINK-9134] [table] upgrade Calcite dependency to 1.17 #6484

suez1224 commented Aug 3, 2018

twalthr left a comment

twalthr Aug 3, 2018

suez1224 Aug 4, 2018

twalthr Aug 3, 2018

twalthr Aug 3, 2018

suez1224 Aug 4, 2018

twalthr Aug 3, 2018

suez1224 Aug 4, 2018

twalthr Aug 3, 2018

tisonkun Aug 3, 2018

tisonkun Aug 3, 2018

suez1224 Aug 4, 2018

twalthr Aug 6, 2018

suez1224 commented Aug 4, 2018

hequn8128 commented Aug 4, 2018

twalthr commented Aug 6, 2018 •

edited

twalthr left a comment

suez1224 commented Aug 6, 2018

twalthr commented Aug 7, 2018


		override def getType(s: String): RelProtoDataType = null

		override def getTypeNames: JSet[String] = ImmutableSet.of()

[FLINK-9134] [table] upgrade Calcite dependency to 1.17 #6484

[FLINK-9134] [table] upgrade Calcite dependency to 1.17 #6484

Conversation

suez1224 commented Aug 3, 2018

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

twalthr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suez1224 commented Aug 4, 2018

hequn8128 commented Aug 4, 2018

twalthr commented Aug 6, 2018 • edited

twalthr left a comment

Choose a reason for hiding this comment

suez1224 commented Aug 6, 2018

twalthr commented Aug 7, 2018

twalthr commented Aug 6, 2018 •

edited