[FLINK-15381] [table-planner-blink] correct collation derive logic on RelSubset in RelMdCollation #10694

godfreyhe · 2019-12-26T07:13:46Z

What is the purpose of the change

sql: select cast(a as int), cast(b as varchar) from (values (3, 'c')) T(a,b) will fail,
the reason is: the original LogicalProject has collation trait (see the picture in FLINK-15381: [1] which means the second field is ordered and its direction is ascending), but when LogicalProject converts to LogicalCalc in ProjectToCalcRule, the collation info of new Calc is empty. The root cause is the collation derive logic on RelSubset in RelMdCollation in not collect.
This PR aim to fix the bug

Brief change log

Implement Collation handler in Flink (most logic except the part about RelSubset is same with RelMdCollation)

Verifying this change

This change added tests and can be verified as follows:

Added FlinkRelMdCollationTest to verify the collation derive logic on each RelNode
Added test that validates the plan of select cast(a as int), cast(b as varchar) from (values (3, 'c')) T(a,b)

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

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

godfreyhe · 2019-12-26T07:16:03Z

...er-blink/src/main/java/org/apache/flink/table/planner/plan/metadata/FlinkRelMdCollation.java

+ * {@link org.apache.calcite.rel.metadata.RelMetadataQuery#collations}
+ * for the standard logical algebra.
+ */
+public class FlinkRelMdCollation implements MetadataHandler<BuiltInMetadata.Collation> {


Most logic (except the part about RelSebset) is same with RelMdCollation.

Maybe we should add the comment in The code. It's really a long class.

flinkbot · 2019-12-26T07:17:27Z

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 e9b4471 (Thu Dec 26 07:17:26 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.

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

flinkbot · 2019-12-26T07:44:17Z

CI report:

e9b4471 Travis: SUCCESS Azure: SUCCESS
a37a0f8 Travis: SUCCESS Azure: SUCCESS
7a59e48 Travis: SUCCESS Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

godfreyhe · 2019-12-26T13:22:45Z

@flinkbot run travis

beyond1920

@godfreyhe ,the fix makes sense.
However, it's better to fix the bug in Calcite.
Or Mark "The class should be removed after CALCITE-{JIARANUMBER} is fixed." in the header comment of the class.
What do you think?

beyond1920 · 2019-12-31T07:29:06Z

...er-blink/src/main/java/org/apache/flink/table/planner/plan/metadata/FlinkRelMdCollation.java

+ return mq.collations(rel.getOriginal());
+ }
+ }
+


The fix makes sense.

beyond1920 · 2019-12-31T07:29:59Z

...er-blink/src/main/java/org/apache/flink/table/planner/plan/metadata/FlinkRelMdCollation.java

+ * {@link org.apache.calcite.rel.metadata.RelMetadataQuery#collations}
+ * for the standard logical algebra.
+ */
+public class FlinkRelMdCollation implements MetadataHandler<BuiltInMetadata.Collation> {


Maybe we should add the comment in The code. It's really a long class.

godfreyhe · 2020-01-02T03:20:45Z

@godfreyhe ,the fix makes sense.
However, it's better to fix the bug in Calcite.
Or Mark "The class should be removed after CALCITE-{JIARANUMBER} is fixed." in the header comment of the class.
What do you think?

thanks for the suggestion @beyond1920 . Flink had rewritten most metadata handlers defined in Calcite. I think we should keep this class in order to support more RelNodes (many RelNodes can derive collation, like Rank). So I would like to add some comments in public ImmutableList<RelCollation> collations(RelSubset subset, RelMetadataQuery mq) method instead of in the header of the class.

beyond1920

I'm agreed with you.
However it's unsafe to throw exception in the else branch.

beyond1920 · 2020-01-02T03:42:32Z

...er-blink/src/main/java/org/apache/flink/table/planner/plan/metadata/FlinkRelMdCollation.java

+ RelNode rel = Util.first(subset.getBest(), subset.getOriginal());
+ return mq.collations(rel);
+ } else {
+ throw new RuntimeException("CALCITE_1048 is fixed, so check this method again!");


It's unsafe to throw exception here.

godfreyhe · 2020-01-02T05:59:55Z

I'm agreed with you.
However it's unsafe to throw exception in the else branch.

FlinkRelMdUniqueKeys and FlinkRelMdUniqueGroups also throw exception for this case and there are many test cases will fail once CALCITE_1048 is fixed

beyond1920 · 2020-01-02T06:45:29Z

LGTM

wuchong · 2020-01-02T08:27:45Z

@godfreyhe could you remove the cast string on this line?

flink/flink-connectors/flink-connector-hive/src/test/java/org/apache/flink/connectors/hive/TableEnvHiveConnectorTest.java

Line 229 in 2e9a7f3

 tableEnv.sqlUpdate("insert overwrite db1.dest values (3,cast('c' as varchar))"); 

It should work now if the bug is fixed.

… RelSubset in RelMdCollation

…sertOverwrite

wuchong

LGTM. Thanks for the effort @godfreyhe .

…Subset in RelMdCollation (#10694)

godfreyhe commented Dec 26, 2019

View reviewed changes

rmetzger added the review=description? label Dec 26, 2019

rmetzger added the component=TableSQL/Planner label Dec 26, 2019

beyond1920 reviewed Dec 31, 2019

View reviewed changes

beyond1920 reviewed Jan 2, 2020

View reviewed changes

godfreyhe added 3 commits January 2, 2020 16:33

[FLINK-15381] [table-planner-blink] correct collation derive logic on…

722764d

… RelSubset in RelMdCollation

address comments

1f464e1

rebase master and remove the cast in TableEnvHiveConnectorTest#testIn…

7a59e48

…sertOverwrite

godfreyhe force-pushed the FLINK-15381 branch from a37a0f8 to 7a59e48 Compare January 2, 2020 08:37

wuchong approved these changes Jan 2, 2020

View reviewed changes

wuchong merged commit 6b6dfd0 into apache:master Jan 2, 2020

wuchong pushed a commit that referenced this pull request Jan 2, 2020

[FLINK-15381] [table-planner-blink] Fix collation derive logic on Rel…

49ddaa1

…Subset in RelMdCollation (#10694)

godfreyhe deleted the FLINK-15381 branch January 3, 2020 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-15381] [table-planner-blink] correct collation derive logic on RelSubset in RelMdCollation #10694

[FLINK-15381] [table-planner-blink] correct collation derive logic on RelSubset in RelMdCollation #10694

godfreyhe commented Dec 26, 2019 •

edited

Loading

godfreyhe Dec 26, 2019

beyond1920 Dec 31, 2019

flinkbot commented Dec 26, 2019

flinkbot commented Dec 26, 2019 •

edited

Loading

godfreyhe commented Dec 26, 2019

beyond1920 left a comment

beyond1920 Dec 31, 2019

beyond1920 Dec 31, 2019

godfreyhe commented Jan 2, 2020

beyond1920 left a comment

beyond1920 Jan 2, 2020

godfreyhe commented Jan 2, 2020 •

edited

Loading

beyond1920 commented Jan 2, 2020

wuchong commented Jan 2, 2020

wuchong left a comment

[FLINK-15381] [table-planner-blink] correct collation derive logic on RelSubset in RelMdCollation #10694

[FLINK-15381] [table-planner-blink] correct collation derive logic on RelSubset in RelMdCollation #10694

Conversation

godfreyhe commented Dec 26, 2019 • edited Loading

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

godfreyhe Dec 26, 2019

Choose a reason for hiding this comment

beyond1920 Dec 31, 2019

Choose a reason for hiding this comment

flinkbot commented Dec 26, 2019

Automated Checks

Review Progress

flinkbot commented Dec 26, 2019 • edited Loading

CI report:

godfreyhe commented Dec 26, 2019

beyond1920 left a comment

Choose a reason for hiding this comment

beyond1920 Dec 31, 2019

Choose a reason for hiding this comment

beyond1920 Dec 31, 2019

Choose a reason for hiding this comment

godfreyhe commented Jan 2, 2020

beyond1920 left a comment

Choose a reason for hiding this comment

beyond1920 Jan 2, 2020

Choose a reason for hiding this comment

godfreyhe commented Jan 2, 2020 • edited Loading

beyond1920 commented Jan 2, 2020

wuchong commented Jan 2, 2020

wuchong left a comment

Choose a reason for hiding this comment

godfreyhe commented Dec 26, 2019 •

edited

Loading

flinkbot commented Dec 26, 2019 •

edited

Loading

godfreyhe commented Jan 2, 2020 •

edited

Loading