[FLINK-11854] [table-planner-blink] Introduce batch physical nodes #7931

godfreyhe · 2019-03-07T13:42:25Z

What is the purpose of the change

Introduce batch physical nodes

Brief change log

adds all batch physical nodes, excludes BatchExecCorrelate, BatchExecTemporalTableJoin, BatchExecXXXWindowAggregate

Verifying this change

This change is an initialization work without any test coverage.

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

flinkbot · 2019-03-07T13:42:46Z

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.

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 working on this, i left some comments

KurtYoung · 2019-03-08T01:44:50Z

...ble/flink-table-runtime-blink/src/main/java/org/apache/flink/table/dataformat/BinaryRow.java

+ }
+
+ public static boolean isMutable(InternalType type) {
+ return MUTABLE_FIELD_TYPES.contains(type) || type instanceof DecimalType;


do we also need to check Decimal's precision just like isFixedLength does?

No matter how precision is, it is mutable.
Precision is compact, Decimal is stored in 8 bytes in fix-length-part.
Precision is not compact, Decimal is stored in 16 bytes in var-length-part.
It can always be update.

method isFixedLength should be isInFixedLengthPart

why not just add the dicimal type to the MUTABLE_FIELD_TYPES, and give some explainations

Dicimal type can not be added into MUTABLE_FIELD_TYPES. Because its instance is not unique, but it has different instances for different precision or scale value.

KurtYoung · 2019-03-08T01:56:22Z

...e/flink-table-planner-blink/src/main/scala/org/apache/flink/table/plan/util/ExpandUtil.scala

+ */
+object ExpandUtil {
+
+ def projectsToString(


Can we try to move all methods like these to one util class? Various logical or physical operators will share some name formatting utilities. There will be a lot repetition if we organize it by operator.

KurtYoung · 2019-03-08T08:43:54Z

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

+/**
+ * Utility methods for RelNode.
+ */
+object RelNodeUtil {


I think this utility class should focus on operator name formatting, things like compute cost should be operator specific

BTW, this class name is also too big

godfreyhe · 2019-03-11T07:43:34Z

@KurtYoung @JingsongLi I have updated this pr based on the comments.

KurtYoung · 2019-03-11T09:32:22Z

Looks like there are some compilation errors

KurtYoung · 2019-03-11T09:58:45Z

...table-planner-blink/src/main/scala/org/apache/flink/table/plan/nodes/common/CommonCalc.scala

+ extends Calc(cluster, traitSet, input, calcProgram)
+ with FlinkRelNode {
+
+


delete extra blank line

KurtYoung · 2019-03-11T09:59:02Z

...table-planner-blink/src/main/scala/org/apache/flink/table/plan/nodes/common/CommonCalc.scala

+ .itemIf("where", conditionToString(), calcProgram.getCondition != null)
+ }
+
+ protected def conditionToString(): String = {


move this to RelExplainUtil?

this method is only used in this class

KurtYoung · 2019-03-11T09:59:11Z

...table-planner-blink/src/main/scala/org/apache/flink/table/plan/nodes/common/CommonCalc.scala

+ }
+ }
+
+ protected def projectionToString(


KurtYoung · 2019-03-11T09:59:24Z

...e-planner-blink/src/main/scala/org/apache/flink/table/plan/nodes/common/CommonExchange.scala

+ .item("distribution", distributionToString())
+ }
+
+ private def distributionToString(): String = {


KurtYoung · 2019-03-11T10:01:54Z

...blink/src/main/scala/org/apache/flink/table/plan/nodes/physical/batch/BatchPhysicalRel.scala

+ length
+ }
+
+ def computeSortMemory(mq: RelMetadataQuery, inputOfSort: RelNode): Double = {


I'm not sure we should place this method here

We can move binaryRowAverageSize and computeSortMemory methods into FlinkRelMdUtil class

KurtYoung · 2019-03-11T10:03:31Z

...ble/flink-table-runtime-blink/src/main/java/org/apache/flink/table/dataformat/BinaryRow.java

+ }
+
+ public static boolean isMutable(InternalType type) {
+ return MUTABLE_FIELD_TYPES.contains(type) || type instanceof DecimalType;


why not just add the dicimal type to the MUTABLE_FIELD_TYPES, and give some explainations

… FlinkRelMdUtil

godfreyhe

@KurtYoung I have updated this pr.

KurtYoung · 2019-03-11T12:42:56Z

LGTM, merging..

…ache#7931)

rmetzger added the review=description? label Mar 7, 2019

KurtYoung reviewed Mar 8, 2019

View reviewed changes

godfreyhe added 4 commits March 11, 2019 14:03

[FLINK-11854] [table-planner-blink] Introduce batch physical nodes

846885d

fix checkstyle error

b6871d2

refactor operator utils

66e4acf

rename RelNodeUtil to RelExplainUtil and do some refactor

b38ff77

godfreyhe force-pushed the FLINK-11854 branch from 0a3a17a to b38ff77 Compare March 11, 2019 07:41

KurtYoung reviewed Mar 11, 2019

View reviewed changes

fix compilation error and move utility methods in BatchPhysicalRel to…

950148b

… FlinkRelMdUtil

godfreyhe commented Mar 11, 2019

View reviewed changes

KurtYoung merged commit 98a4f63 into apache:master Mar 11, 2019

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

HuangZhenQiu pushed a commit to HuangZhenQiu/flink that referenced this pull request Apr 22, 2019

[FLINK-11854][table-planner-blink] Introduce batch physical nodes (ap…

3ee092e

…ache#7931)

rmetzger added component=TableSQL/Planner and removed component=API/TableSQL labels Apr 30, 2019

godfreyhe deleted the FLINK-11854 branch June 1, 2019 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-11854] [table-planner-blink] Introduce batch physical nodes #7931

[FLINK-11854] [table-planner-blink] Introduce batch physical nodes #7931

godfreyhe commented Mar 7, 2019 •

edited

Loading

flinkbot commented Mar 7, 2019

KurtYoung left a comment

KurtYoung Mar 8, 2019

JingsongLi Mar 8, 2019 •

edited

Loading

JingsongLi Mar 8, 2019

godfreyhe Mar 11, 2019

KurtYoung Mar 11, 2019

godfreyhe Mar 11, 2019

KurtYoung Mar 8, 2019

KurtYoung Mar 8, 2019

KurtYoung Mar 8, 2019

godfreyhe commented Mar 11, 2019

KurtYoung commented Mar 11, 2019

KurtYoung Mar 11, 2019

KurtYoung Mar 11, 2019

godfreyhe Mar 11, 2019

KurtYoung Mar 11, 2019

KurtYoung Mar 11, 2019

KurtYoung Mar 11, 2019

godfreyhe Mar 11, 2019

KurtYoung Mar 11, 2019

godfreyhe left a comment

KurtYoung commented Mar 11, 2019

		extends Calc(cluster, traitSet, input, calcProgram)
		with FlinkRelNode {

[FLINK-11854] [table-planner-blink] Introduce batch physical nodes #7931

[FLINK-11854] [table-planner-blink] Introduce batch physical nodes #7931

Conversation

godfreyhe commented Mar 7, 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

flinkbot commented Mar 7, 2019

Review Progress

KurtYoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JingsongLi Mar 8, 2019 • edited Loading

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

godfreyhe commented Mar 11, 2019

KurtYoung commented Mar 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godfreyhe left a comment

Choose a reason for hiding this comment

KurtYoung commented Mar 11, 2019

godfreyhe commented Mar 7, 2019 •

edited

Loading

JingsongLi Mar 8, 2019 •

edited

Loading