[FLINK-13107][table-planner-blink] Copy TableApi IT and UT to Blink planner #9006

beyond1920 · 2019-07-06T12:23:58Z

What is the purpose of the change

The issue aims to copy the testcases in the following packages from flink-planner and original blink to Blink-planner:

org.apache.flink.table.api.batch.table
org.apache.flink.table.api.stream.table
org.apache.flink.table.runtime.batch.table
org.apache.flink.table.runtime.stream.table

Brief change log

Copy UT and ITCase of TableApi
Fix some Bugs, commit message contains detail information about those Bugs

Verifying this change

UT, ITCase

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-07-06T12:25:24Z

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.

Automated Checks

Last check on commit 9078425 (Tue Aug 06 15:39:38 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

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

KurtYoung

Thanks for the work!

...k-table-planner-blink/src/main/java/org/apache/flink/table/expressions/RexNodeConverter.java

KurtYoung · 2019-07-06T13:15:18Z

...lanner-blink/src/main/scala/org/apache/flink/table/plan/schema/TypedFlinkTableFunction.scala

-    // we have determined the row type before, just convert it to RelDataType
-    typeFactory.asInstanceOf[FlinkTypeFactory].createFieldTypeFromLogicalType(
-      fromDataTypeToLogicalType(externalResultType))
+    val fieldTypes = TableEnvironment.getFieldTypes(externalResultType)


I would suggest to not use TableEnvironment.getFieldTypes, it will be deleted soon

OK, I would use FieldInfoUtils in api-module instead.

flinkbot · 2019-07-07T06:56:23Z

CI report for commit 7da887d: SUCCESS Build

flinkbot · 2019-07-07T06:56:27Z

CI report for commit b756b1f: SUCCESS Build

JingsongLi · 2019-07-08T01:14:52Z

We need copy all org.apache.flink.table.api.*? I think we have these duplicate plan cases in org.apache.flink.table.table.*? Why not put cases to org.apache.flink.table.table.* to be consistent with previous SQL plan tests?

JingsongLi · 2019-07-08T01:17:20Z

...k-table-runtime-blink/src/main/java/org/apache/flink/table/types/ClassDataTypeConverter.java

+
+		// NOTE: this list explicitly excludes data types that need further parameters
+		// exclusions: DECIMAL, INTERVAL YEAR TO MONTH, MAP, MULTISET, ROW, NULL, ANY
+		addDefaultDataType(boolean.class, DataTypes.BOOLEAN());


-1 about this change, why we need convert a int to DataType(Integer)?

The code is similar with ClassDataTypeConverter in table-common module. When UDF has a eval method explicitly return int type, we need convert a int to DataType.
I add code here to fix the Inconsistence between ClassDataTypeConverter with LegacyTypeInfoDataTypeConverter.
ClassDataTypeConverter map Class to DataTypes, However, for primitive class, it explicit do the following logical to binds convention class:
private static void addDefaultDataType(Class<?> clazz, DataType rootType) { final DataType dataType; if (clazz.isPrimitive()) { dataType = rootType.notNull(); } else { dataType = rootType.nullable(); } defaultDataTypes.put(clazz.getName(), dataType.bridgedTo(clazz)); }
So for the primitive class, for example, int.class, it's DataType binding to int.class, instead of Integer.class.
The logical is not consistent with LegacyTypeInfoDataTypeConverter.
the mapping in LegacyTypeInfoDataTypeConverter only contains DataTypes.INT().bridgedTo(Integer.class), not including DataTypes.INT().bridgedTo(int.class).

So why not change TypeInfoDataTypeConverter? This is the conversion util in blink.

Good suggestion.

JingsongLi · 2019-07-08T01:23:23Z

...ink-table-planner-blink/src/main/scala/org/apache/flink/table/expressions/aggregations.scala

  override def toString = s"sum($child)"

-  override private[flink] def resultType = child.resultType
+  override private[flink] def resultType = {


[FLINK-13107][table-planner-blink] Fix bug when infer resultType of some PlannerExpression.

[FLINK-13107][table-planner-blink] Derive sum, avg, div return type in planner expressions using behavior of blink ?

Run a sql query using blink, the behavior already using behavior of blink. It would be strange if the behavior is different when run a tableApi query.
So I think we should use behavior of blink.
Do you mean change the commit message?

Yeah, The change is very good, I think the commit message should be more clear.

...k-table-planner-blink/src/main/scala/org/apache/flink/table/plan/util/SetOpRewriteUtil.scala

flinkbot · 2019-07-08T21:46:42Z

CI report for commit ce897c5: FAILURE Build

twalthr

What are the implications on the test times when we copy so many tests?

…TableFunction to using fieldNames which is specified.

…h has primitiveClass as convensionClass to TypeInformation.

…n planner expressions using behavior of blink

…is RowMode or RangeMode.

…lanner.

flinkbot · 2019-07-11T09:38:57Z

CI report:

9078425 : SUCCESS Build

KurtYoung · 2019-07-11T11:15:30Z

What are the implications on the test times when we copy so many tests?

According to this: https://travis-ci.org/beyond1920/flink/builds/557233740, the test time increases less than 2 mins.

KurtYoung · 2019-07-11T11:18:10Z

travis passed here: https://travis-ci.org/beyond1920/flink/builds/557233740
I'm merging this

rmetzger added the review=description? label Jul 6, 2019

KurtYoung requested changes Jul 6, 2019

View reviewed changes

rmetzger added the component=TableSQL/Planner label Jul 6, 2019

JingsongLi reviewed Jul 8, 2019

View reviewed changes

...k-table-planner-blink/src/main/scala/org/apache/flink/table/plan/util/SetOpRewriteUtil.scala Show resolved Hide resolved

beyond1920 force-pushed the TableApiTest branch 4 times, most recently from 4cb0ae7 to ce897c5 Compare July 8, 2019 07:42

twalthr reviewed Jul 9, 2019

View reviewed changes

beyond1920 added 5 commits July 11, 2019 17:00

[FLINK-13107][table-planner-blink] Introduce fieldNames to TypedFlink…

7f9c676

…TableFunction to using fieldNames which is specified.

[FLINK-13107][table-planner-blink] Fix Bug when convert DataType whic…

ad3013e

…h has primitiveClass as convensionClass to TypeInformation.

[FLINK-13107][table-planner-blink] Derive sum, avg, div return type i…

3f13623

…n planner expressions using behavior of blink

[FLINK-13107][table-planner-blink] Fix Bug to check whether OverCall …

3ad3a7f

…is RowMode or RangeMode.

[FLINK-13107][table-planner-blink] Copy TableApi IT and UT to Blink p…

9078425

…lanner.

beyond1920 force-pushed the TableApiTest branch from ce897c5 to 9078425 Compare July 11, 2019 09:29

KurtYoung closed this in 116c10b Jul 11, 2019

[FLINK-13107][table-planner-blink] Copy TableApi IT and UT to Blink planner #9006

[FLINK-13107][table-planner-blink] Copy TableApi IT and UT to Blink planner #9006

Uh oh!

Conversation

beyond1920 commented Jul 6, 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 Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

KurtYoung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KurtYoung Jul 6, 2019

Choose a reason for hiding this comment

Uh oh!

beyond1920 Jul 6, 2019

Choose a reason for hiding this comment

Uh oh!

flinkbot commented Jul 7, 2019

Uh oh!

flinkbot commented Jul 7, 2019

Uh oh!

JingsongLi commented Jul 8, 2019

Uh oh!

JingsongLi Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

beyond1920 Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

JingsongLi Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

beyond1920 Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

JingsongLi Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beyond1920 Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JingsongLi Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flinkbot commented Jul 8, 2019

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

flinkbot commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

KurtYoung commented Jul 11, 2019

Uh oh!

KurtYoung commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flinkbot commented Jul 6, 2019 •

edited

Loading

JingsongLi Jul 8, 2019 •

edited

Loading

beyond1920 Jul 8, 2019 •

edited

Loading

flinkbot commented Jul 11, 2019 •

edited

Loading