Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6047 Add support for Retraction in Table API / SQL
  3. FLINK-6089

Implement decoration phase for rewriting predicated logical plan after volcano optimization phase

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Implemented
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: Table API & SQL
    • Labels:
      None

      Description

      At present, there is no chance to modify the DataStreamRel tree after the volcano optimization. We consider to add a decoration phase after volcano optimization phase. Decoration phase is dedicated for rewriting predicated logical plan and is independent of cost module. After decoration phase is added, we get the chance to apply retraction rules at this phase.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user hequn8128 opened a pull request:

          https://github.com/apache/flink/pull/3564

          FLINK-6089 [table] Implement decoration phase for rewriting predicated logical plan after volcano optimization phase

          At present, there is no chance to modify the DataStreamRel tree after the volcano optimization. This pr consider to add a decoration phase after volcano optimization phase. Decoration phase is dedicated for rewriting predicated logical plan and is independent of cost module. After decoration phase is added, we get the chance to apply retraction rules at this phase (The next jiraFLINK-6090 will add retraction rules at decorate phase).

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/hequn8128/flink FLINK-6089

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3564.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3564


          commit 0fed825bc3655af9b4fce5e67e66018e194ba6b3
          Author: hequn.chq <hequn.chq@alibaba-inc.com>
          Date: 2017-03-16T03:11:17Z

          Implement decoration phase for predicated logical plan rewriting after volcano optimization phase


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user hequn8128 opened a pull request: https://github.com/apache/flink/pull/3564 FLINK-6089 [table] Implement decoration phase for rewriting predicated logical plan after volcano optimization phase At present, there is no chance to modify the DataStreamRel tree after the volcano optimization. This pr consider to add a decoration phase after volcano optimization phase. Decoration phase is dedicated for rewriting predicated logical plan and is independent of cost module. After decoration phase is added, we get the chance to apply retraction rules at this phase (The next jira FLINK-6090 will add retraction rules at decorate phase). You can merge this pull request into a Git repository by running: $ git pull https://github.com/hequn8128/flink FLINK-6089 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3564.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3564 commit 0fed825bc3655af9b4fce5e67e66018e194ba6b3 Author: hequn.chq <hequn.chq@alibaba-inc.com> Date: 2017-03-16T03:11:17Z Implement decoration phase for predicated logical plan rewriting after volcano optimization phase
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3564#discussion_r107105616

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/CalciteConfig.scala —
          @@ -113,35 +135,39 @@ class CalciteConfigBuilder {
          val replacesNormRuleSet: Boolean,
          val getOptRuleSet: Option[RuleSet],
          val replacesOptRuleSet: Boolean,
          + val getDecoRuleSet: Option[RuleSet],
          + val replacesDecoRuleSet: Boolean,
          val getSqlOperatorTable: Option[SqlOperatorTable],
          val replacesSqlOperatorTable: Boolean,
          val getSqlParserConfig: Option[SqlParser.Config])
          extends CalciteConfig

          +
          /**

          • * Builds a new [[CalciteConfig]].
            + * Convert the [[RuleSet]] List to [[Option]] type
            */
          • def build(): CalciteConfig = new CalciteConfigImpl(
          • normRuleSets match {
            + def getRuleSet(inputRuleSet: List[RuleSet]): Option[RuleSet] = {
              • End diff –

          I think this method can be private.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on a diff in the pull request: https://github.com/apache/flink/pull/3564#discussion_r107105616 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/CalciteConfig.scala — @@ -113,35 +135,39 @@ class CalciteConfigBuilder { val replacesNormRuleSet: Boolean, val getOptRuleSet: Option [RuleSet] , val replacesOptRuleSet: Boolean, + val getDecoRuleSet: Option [RuleSet] , + val replacesDecoRuleSet: Boolean, val getSqlOperatorTable: Option [SqlOperatorTable] , val replacesSqlOperatorTable: Boolean, val getSqlParserConfig: Option [SqlParser.Config] ) extends CalciteConfig + /** * Builds a new [ [CalciteConfig] ]. + * Convert the [ [RuleSet] ] List to [ [Option] ] type */ def build(): CalciteConfig = new CalciteConfigImpl( normRuleSets match { + def getRuleSet(inputRuleSet: List [RuleSet] ): Option [RuleSet] = { End diff – I think this method can be private.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3564#discussion_r107106790

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/CalciteConfig.scala —
          @@ -81,6 +84,25 @@ class CalciteConfigBuilder {
          }

          /**
          + * Replaces the built-in decoration rule set with the given rule set.
          — End diff –

          Can you add a more detailed description what decoration rules, norm rules etc. are. You can reuse your PR text: "after the volcano optimization, Decoration phase is dedicated for rewriting predicated logical plan and is independent of cost module"

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on a diff in the pull request: https://github.com/apache/flink/pull/3564#discussion_r107106790 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/CalciteConfig.scala — @@ -81,6 +84,25 @@ class CalciteConfigBuilder { } /** + * Replaces the built-in decoration rule set with the given rule set. — End diff – Can you add a more detailed description what decoration rules, norm rules etc. are. You can reuse your PR text: "after the volcano optimization, Decoration phase is dedicated for rewriting predicated logical plan and is independent of cost module"
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3564

          *Please note*: This is the first PR for adding retraction support. It should be merged to the `table-retraction` feature branch, not the `master` branch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3564 * Please note *: This is the first PR for adding retraction support. It should be merged to the `table-retraction` feature branch, not the `master` branch.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hequn8128 commented on the issue:

          https://github.com/apache/flink/pull/3564

          @twalthr Thanks for the review. I have addressed all your comments and updated the PR

          Show
          githubbot ASF GitHub Bot added a comment - Github user hequn8128 commented on the issue: https://github.com/apache/flink/pull/3564 @twalthr Thanks for the review. I have addressed all your comments and updated the PR
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shaoxuan-wang commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3564#discussion_r107594707

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/TableEnvironment.scala —
          @@ -162,6 +162,27 @@ abstract class TableEnvironment(val config: TableConfig) {
          }

          /**
          + * Returns the decoration rule set for this environment
          + * including a custom RuleSet configuration.
          — End diff –

          Do not unnecessarily break lines.

          Show
          githubbot ASF GitHub Bot added a comment - Github user shaoxuan-wang commented on a diff in the pull request: https://github.com/apache/flink/pull/3564#discussion_r107594707 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/TableEnvironment.scala — @@ -162,6 +162,27 @@ abstract class TableEnvironment(val config: TableConfig) { } /** + * Returns the decoration rule set for this environment + * including a custom RuleSet configuration. — End diff – Do not unnecessarily break lines.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hequn8128 commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3564#discussion_r107603328

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/TableEnvironment.scala —
          @@ -162,6 +162,27 @@ abstract class TableEnvironment(val config: TableConfig) {
          }

          /**
          + * Returns the decoration rule set for this environment
          + * including a custom RuleSet configuration.
          — End diff –

          done, pr has been updated~

          Show
          githubbot ASF GitHub Bot added a comment - Github user hequn8128 commented on a diff in the pull request: https://github.com/apache/flink/pull/3564#discussion_r107603328 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/TableEnvironment.scala — @@ -162,6 +162,27 @@ abstract class TableEnvironment(val config: TableConfig) { } /** + * Returns the decoration rule set for this environment + * including a custom RuleSet configuration. — End diff – done, pr has been updated~
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3564

          Hi @hequn8128, thanks for the PR. I think it looks good, but I have a question.
          This PR is prepares for adding retractions. Why do we need a decoration phase for batch queries if this is about retractions which only apply for streams?

          Thanks, Fabian

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3564 Hi @hequn8128, thanks for the PR. I think it looks good, but I have a question. This PR is prepares for adding retractions. Why do we need a decoration phase for batch queries if this is about retractions which only apply for streams? Thanks, Fabian
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hequn8128 commented on the issue:

          https://github.com/apache/flink/pull/3564

          hi @fhueske , thanks for the review. I was expecting some other batch use-cases where we may need to rewrite the logical plan after volcano. But you are right, we do not need to add it into BatchTableEnvironment for now. I have updated PR to remove it from BatchTableEnvironment. Please take a look.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hequn8128 commented on the issue: https://github.com/apache/flink/pull/3564 hi @fhueske , thanks for the review. I was expecting some other batch use-cases where we may need to rewrite the logical plan after volcano. But you are right, we do not need to add it into BatchTableEnvironment for now. I have updated PR to remove it from BatchTableEnvironment. Please take a look.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3564

          Thanks for the update @hequn8128.

          I think this PR is good to merge once you addressed @twalthr's comment about extending the method docs.

          Thank you, Fabian

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3564 Thanks for the update @hequn8128. I think this PR is good to merge once you addressed @twalthr's comment about extending the method docs. Thank you, Fabian
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3564

          merging

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3564 merging
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3564

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3564
          Hide
          fhueske Fabian Hueske added a comment -

          Implemented with 6949c8c79c41344023df08dde2936f06daa00e0d

          Show
          fhueske Fabian Hueske added a comment - Implemented with 6949c8c79c41344023df08dde2936f06daa00e0d
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3564

          I accidentally merged this to `master` not `table-retraction`.
          Since the changes do not break anything, this is not a problem.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3564 I accidentally merged this to `master` not `table-retraction`. Since the changes do not break anything, this is not a problem.

            People

            • Assignee:
              hequn8128 Hequn Cheng
              Reporter:
              ShaoxuanWang Shaoxuan Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development