Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-5406

add normalization phase for predicate logical plan rewriting between decorrelate query phase and volcano optimization phase

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: Table API & SQL
    • Labels:
      None

      Description

      Normalization phase is for predicate logical plan rewriting and is independent of cost module. The rules in normalization phase do not need to repeatedly applied to different logical plan which is different to volcano optimization phase. And the benefit of normalization phase is to reduce the running time of volcano planner.

      ReduceExpressionsRule can apply various simplifying transformations on RexNode trees. Currently, there are two transformations:
      1) Constant reduction, which evaluates constant subtrees, replacing them with a corresponding RexLiteral
      2) Removal of redundant casts, which occurs when the argument into the cast is the same as the type of the resulting cast expression

      the above transformations do not depend on the cost module, so we can move the rules in ReduceExpressionsRule from DATASET_OPT_RULES/DATASTREAM_OPT_RULES to DataSet/DataStream Normalization Rules.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user godfreyhe opened a pull request:

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

          FLINK-5406 [table] add normalization phase for predicate logical plan rewriting

          Normalization phase is for predicate logical plan rewriting and is independent of cost module. The rules in normalization phase do not need to repeatedly applied to different logical plan which is different to volcano optimization phase. And the benefit of normalization phase is to reduce the running time of volcano planner.

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

          $ git pull https://github.com/godfreyhe/flink master

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

          https://github.com/apache/flink/pull/3101.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 #3101


          commit c9f11e7c38f921a403b56a5c124974eb66bddcac
          Author: godfreyhe <godfreyhe@163.com>
          Date: 2017-01-12T10:42:49Z

          add normalization phase for predicate logical plan rewriting


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user godfreyhe opened a pull request: https://github.com/apache/flink/pull/3101 FLINK-5406 [table] add normalization phase for predicate logical plan rewriting Normalization phase is for predicate logical plan rewriting and is independent of cost module. The rules in normalization phase do not need to repeatedly applied to different logical plan which is different to volcano optimization phase. And the benefit of normalization phase is to reduce the running time of volcano planner. You can merge this pull request into a Git repository by running: $ git pull https://github.com/godfreyhe/flink master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3101.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 #3101 commit c9f11e7c38f921a403b56a5c124974eb66bddcac Author: godfreyhe <godfreyhe@163.com> Date: 2017-01-12T10:42:49Z add normalization phase for predicate logical plan rewriting
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on the issue:

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

          I will shepherd this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on the issue: https://github.com/apache/flink/pull/3101 I will shepherd this PR.
          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/3101#discussion_r97996946

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala —
          @@ -0,0 +1,102 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.flink.table
          +
          +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule
          +import org.apache.calcite.tools.RuleSets
          +import org.apache.flink.api.scala._
          +import org.apache.flink.table.api.scala._
          +import org.apache.flink.table.calcite.

          {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder}

          +import org.apache.flink.table.utils.TableTestBase
          +import org.apache.flink.table.utils.TableTestUtil._
          +import org.junit.Test
          +
          +class NormalizationRulesTests extends TableTestBase {
          +
          + @Test
          + def testAppleNormalizationRuleForForBatchSQL(): Unit = {
          + val util = batchTestUtil()
          + val rc: RuleSetConfig = new RuleSetConfigBuilder()
          + // rewrite distinct aggregate
          + .replaceNormRuleSet(RuleSets.ofList(AggregateExpandDistinctAggregatesRule.JOIN))
          + .replaceOptRuleSet(RuleSets.ofList())
          + .build
          +
          + val cc: CalciteConfig = new CalciteConfigBuilder().replaceRuleSetConfig(rc).build
          + util.tEnv.getConfig.setCalciteConfig(cc)
          +
          + util.addTable[(Int, Long, String)]("MyTable", 'a, 'b, 'c)
          +
          + val sqlQuery = "SELECT " +
          + "COUNT(DISTINCT a)" +
          + "FROM MyTable group by b"
          +
          + // expect double aggregate
          + val expected = unaryNode(
          + "LogicalProject",
          + unaryNode("LogicalAggregate",
          + unaryNode("LogicalAggregate",
          — End diff –

          We use two spaces for indention.

          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/3101#discussion_r97996946 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala — @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.flink.table + +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule +import org.apache.calcite.tools.RuleSets +import org.apache.flink.api.scala._ +import org.apache.flink.table.api.scala._ +import org.apache.flink.table.calcite. {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder} +import org.apache.flink.table.utils.TableTestBase +import org.apache.flink.table.utils.TableTestUtil._ +import org.junit.Test + +class NormalizationRulesTests extends TableTestBase { + + @Test + def testAppleNormalizationRuleForForBatchSQL(): Unit = { + val util = batchTestUtil() + val rc: RuleSetConfig = new RuleSetConfigBuilder() + // rewrite distinct aggregate + .replaceNormRuleSet(RuleSets.ofList(AggregateExpandDistinctAggregatesRule.JOIN)) + .replaceOptRuleSet(RuleSets.ofList()) + .build + + val cc: CalciteConfig = new CalciteConfigBuilder().replaceRuleSetConfig(rc).build + util.tEnv.getConfig.setCalciteConfig(cc) + + util.addTable [(Int, Long, String)] ("MyTable", 'a, 'b, 'c) + + val sqlQuery = "SELECT " + + "COUNT(DISTINCT a)" + + "FROM MyTable group by b" + + // expect double aggregate + val expected = unaryNode( + "LogicalProject", + unaryNode("LogicalAggregate", + unaryNode("LogicalAggregate", — End diff – We use two spaces for indention.
          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/3101#discussion_r97992445

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/AggregationTest.scala —
          @@ -165,14 +165,20 @@ class AggregationTest extends TableTestBase {
          val util = batchTestUtil()
          val sourceTable = util.addTable[(Int, Long, Int)]("MyTable", 'a, 'b, 'c)

          • val resultTable = sourceTable.groupBy('a)
            + // Move "where" before "groupBy" for the former query would generate
            + // nondeterministic plans with same cost. If we change FlinkRuleSets.DATASET_OPT_RULES,
            + // the importance of relNode may change, and the test may fail. This issue is mentioned
            + // in FLINK-5394, we could move "where" to the end when FLINK-5394 is fixed.
            + val resultTable = sourceTable.where('a === 1)
              • End diff –

          FLINK-5394 is fixed. So we can remove the changes here right?

          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/3101#discussion_r97992445 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/AggregationTest.scala — @@ -165,14 +165,20 @@ class AggregationTest extends TableTestBase { val util = batchTestUtil() val sourceTable = util.addTable [(Int, Long, Int)] ("MyTable", 'a, 'b, 'c) val resultTable = sourceTable.groupBy('a) + // Move "where" before "groupBy" for the former query would generate + // nondeterministic plans with same cost. If we change FlinkRuleSets.DATASET_OPT_RULES, + // the importance of relNode may change, and the test may fail. This issue is mentioned + // in FLINK-5394 , we could move "where" to the end when FLINK-5394 is fixed. + val resultTable = sourceTable.where('a === 1) End diff – FLINK-5394 is fixed. So we can remove the changes here right?
          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/3101#discussion_r97992130

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/RuleSetConfig.scala —
          @@ -0,0 +1,140 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.flink.table.calcite
          +
          +import org.apache.calcite.plan.RelOptRule
          +import org.apache.calcite.tools.

          {RuleSet, RuleSets}

          +import org.apache.flink.util.Preconditions
          +
          +import scala.collection.JavaConverters._
          +
          +/**
          + * Builder for creating a RuleSet configuration.
          + */
          +class RuleSetConfigBuilder {
          — End diff –

          I think we don't need a builder in a builder. Can we integrate `RuleSetConfigBuilder` in `CalciteConfigBuilder`?

          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/3101#discussion_r97992130 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/RuleSetConfig.scala — @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.table.calcite + +import org.apache.calcite.plan.RelOptRule +import org.apache.calcite.tools. {RuleSet, RuleSets} +import org.apache.flink.util.Preconditions + +import scala.collection.JavaConverters._ + +/** + * Builder for creating a RuleSet configuration. + */ +class RuleSetConfigBuilder { — End diff – I think we don't need a builder in a builder. Can we integrate `RuleSetConfigBuilder` in `CalciteConfigBuilder`?
          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/3101#discussion_r97993265

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala —
          @@ -0,0 +1,102 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.flink.table
          +
          +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule
          +import org.apache.calcite.tools.RuleSets
          +import org.apache.flink.api.scala._
          +import org.apache.flink.table.api.scala._
          +import org.apache.flink.table.calcite.

          {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder}

          +import org.apache.flink.table.utils.TableTestBase
          +import org.apache.flink.table.utils.TableTestUtil._
          +import org.junit.Test
          +
          +class NormalizationRulesTests extends TableTestBase {
          — End diff –

          Merge this class with `CalciteConfigBuilderTest`.

          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/3101#discussion_r97993265 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala — @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.flink.table + +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule +import org.apache.calcite.tools.RuleSets +import org.apache.flink.api.scala._ +import org.apache.flink.table.api.scala._ +import org.apache.flink.table.calcite. {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder} +import org.apache.flink.table.utils.TableTestBase +import org.apache.flink.table.utils.TableTestUtil._ +import org.junit.Test + +class NormalizationRulesTests extends TableTestBase { — End diff – Merge this class with `CalciteConfigBuilderTest`.
          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/3101#discussion_r97997077

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala —
          @@ -0,0 +1,102 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.flink.table
          +
          +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule
          +import org.apache.calcite.tools.RuleSets
          +import org.apache.flink.api.scala._
          +import org.apache.flink.table.api.scala._
          +import org.apache.flink.table.calcite.

          {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder}

          +import org.apache.flink.table.utils.TableTestBase
          +import org.apache.flink.table.utils.TableTestUtil._
          +import org.junit.Test
          +
          +class NormalizationRulesTests extends TableTestBase {
          +
          + @Test
          + def testAppleNormalizationRuleForForBatchSQL(): Unit = {
          — End diff –

          `Apply`

          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/3101#discussion_r97997077 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala — @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.flink.table + +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule +import org.apache.calcite.tools.RuleSets +import org.apache.flink.api.scala._ +import org.apache.flink.table.api.scala._ +import org.apache.flink.table.calcite. {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder} +import org.apache.flink.table.utils.TableTestBase +import org.apache.flink.table.utils.TableTestUtil._ +import org.junit.Test + +class NormalizationRulesTests extends TableTestBase { + + @Test + def testAppleNormalizationRuleForForBatchSQL(): Unit = { — End diff – `Apply`
          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/3101#discussion_r97996845

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala —
          @@ -0,0 +1,102 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.flink.table
          +
          +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule
          +import org.apache.calcite.tools.RuleSets
          +import org.apache.flink.api.scala._
          +import org.apache.flink.table.api.scala._
          +import org.apache.flink.table.calcite.

          {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder}

          +import org.apache.flink.table.utils.TableTestBase
          +import org.apache.flink.table.utils.TableTestUtil._
          +import org.junit.Test
          +
          +class NormalizationRulesTests extends TableTestBase {
          +
          + @Test
          + def testAppleNormalizationRuleForForBatchSQL(): Unit = {
          + val util = batchTestUtil()
          + val rc: RuleSetConfig = new RuleSetConfigBuilder()
          + // rewrite distinct aggregate
          + .replaceNormRuleSet(RuleSets.ofList(AggregateExpandDistinctAggregatesRule.JOIN))
          + .replaceOptRuleSet(RuleSets.ofList())
          + .build
          — End diff –

          Add parentheses to prevent strict IDE warnings.

          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/3101#discussion_r97996845 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala — @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.flink.table + +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule +import org.apache.calcite.tools.RuleSets +import org.apache.flink.api.scala._ +import org.apache.flink.table.api.scala._ +import org.apache.flink.table.calcite. {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder} +import org.apache.flink.table.utils.TableTestBase +import org.apache.flink.table.utils.TableTestUtil._ +import org.junit.Test + +class NormalizationRulesTests extends TableTestBase { + + @Test + def testAppleNormalizationRuleForForBatchSQL(): Unit = { + val util = batchTestUtil() + val rc: RuleSetConfig = new RuleSetConfigBuilder() + // rewrite distinct aggregate + .replaceNormRuleSet(RuleSets.ofList(AggregateExpandDistinctAggregatesRule.JOIN)) + .replaceOptRuleSet(RuleSets.ofList()) + .build — End diff – Add parentheses to prevent strict IDE warnings.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3101#discussion_r99522172

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/AggregationTest.scala —
          @@ -165,14 +165,20 @@ class AggregationTest extends TableTestBase {
          val util = batchTestUtil()
          val sourceTable = util.addTable[(Int, Long, Int)]("MyTable", 'a, 'b, 'c)

          • val resultTable = sourceTable.groupBy('a)
            + // Move "where" before "groupBy" for the former query would generate
            + // nondeterministic plans with same cost. If we change FlinkRuleSets.DATASET_OPT_RULES,
            + // the importance of relNode may change, and the test may fail. This issue is mentioned
            + // in FLINK-5394, we could move "where" to the end when FLINK-5394 is fixed.
            + val resultTable = sourceTable.where('a === 1)
              • End diff –

          OK

          Show
          githubbot ASF GitHub Bot added a comment - Github user godfreyhe commented on a diff in the pull request: https://github.com/apache/flink/pull/3101#discussion_r99522172 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/AggregationTest.scala — @@ -165,14 +165,20 @@ class AggregationTest extends TableTestBase { val util = batchTestUtil() val sourceTable = util.addTable [(Int, Long, Int)] ("MyTable", 'a, 'b, 'c) val resultTable = sourceTable.groupBy('a) + // Move "where" before "groupBy" for the former query would generate + // nondeterministic plans with same cost. If we change FlinkRuleSets.DATASET_OPT_RULES, + // the importance of relNode may change, and the test may fail. This issue is mentioned + // in FLINK-5394 , we could move "where" to the end when FLINK-5394 is fixed. + val resultTable = sourceTable.where('a === 1) End diff – OK
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3101#discussion_r99524112

          — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/RuleSetConfig.scala —
          @@ -0,0 +1,140 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +package org.apache.flink.table.calcite
          +
          +import org.apache.calcite.plan.RelOptRule
          +import org.apache.calcite.tools.

          {RuleSet, RuleSets}

          +import org.apache.flink.util.Preconditions
          +
          +import scala.collection.JavaConverters._
          +
          +/**
          + * Builder for creating a RuleSet configuration.
          + */
          +class RuleSetConfigBuilder {
          — End diff –

          If we integrate` RuleSetConfigBuilder` in `CalciteConfigBuilder`, It's better we integrate `RuleSetConfig` in `CalciteConfig` too ? If we want to add another kind of `RuleSet` in future, both `RuleSetConfig` and `CalciteConfigBuilder` are need to change. Previously, I want to keep `CalciteConfig` and `CalciteConfigBuilder` stable.

          Show
          githubbot ASF GitHub Bot added a comment - Github user godfreyhe commented on a diff in the pull request: https://github.com/apache/flink/pull/3101#discussion_r99524112 — Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/calcite/RuleSetConfig.scala — @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.table.calcite + +import org.apache.calcite.plan.RelOptRule +import org.apache.calcite.tools. {RuleSet, RuleSets} +import org.apache.flink.util.Preconditions + +import scala.collection.JavaConverters._ + +/** + * Builder for creating a RuleSet configuration. + */ +class RuleSetConfigBuilder { — End diff – If we integrate` RuleSetConfigBuilder` in `CalciteConfigBuilder`, It's better we integrate `RuleSetConfig` in `CalciteConfig` too ? If we want to add another kind of `RuleSet` in future, both `RuleSetConfig` and `CalciteConfigBuilder` are need to change. Previously, I want to keep `CalciteConfig` and `CalciteConfigBuilder` stable.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3101#discussion_r100488613

          — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala —
          @@ -0,0 +1,102 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.flink.table
          +
          +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule
          +import org.apache.calcite.tools.RuleSets
          +import org.apache.flink.api.scala._
          +import org.apache.flink.table.api.scala._
          +import org.apache.flink.table.calcite.

          {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder}

          +import org.apache.flink.table.utils.TableTestBase
          +import org.apache.flink.table.utils.TableTestUtil._
          +import org.junit.Test
          +
          +class NormalizationRulesTests extends TableTestBase {
          — End diff –

          CalciteConfigBuilderTest just tests the correctness of CalciteConfig, does not test logical plan changing when applied a rule. However NormalizationRulesTest tests the correctness of a logical plan when applied a normalization rule. I suggest we keep NormalizationRulesTest.

          Show
          githubbot ASF GitHub Bot added a comment - Github user godfreyhe commented on a diff in the pull request: https://github.com/apache/flink/pull/3101#discussion_r100488613 — Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/NormalizationRulesTests.scala — @@ -0,0 +1,102 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.flink.table + +import org.apache.calcite.rel.rules.AggregateExpandDistinctAggregatesRule +import org.apache.calcite.tools.RuleSets +import org.apache.flink.api.scala._ +import org.apache.flink.table.api.scala._ +import org.apache.flink.table.calcite. {CalciteConfig, CalciteConfigBuilder, RuleSetConfig, +RuleSetConfigBuilder} +import org.apache.flink.table.utils.TableTestBase +import org.apache.flink.table.utils.TableTestUtil._ +import org.junit.Test + +class NormalizationRulesTests extends TableTestBase { — End diff – CalciteConfigBuilderTest just tests the correctness of CalciteConfig, does not test logical plan changing when applied a rule. However NormalizationRulesTest tests the correctness of a logical plan when applied a normalization rule. I suggest we keep NormalizationRulesTest.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

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

          Thanks for the update @godfreyhe!
          PR looks good to me and can be merged IMO.

          @godfreyhe, can you rebase the PR to the current master?
          @twalthr, do you want to have another look as well?

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3101 Thanks for the update @godfreyhe! PR looks good to me and can be merged IMO. @godfreyhe, can you rebase the PR to the current master? @twalthr, do you want to have another look as well?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user godfreyhe commented on the issue:

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

          Done, thanks for the suggest @fhueske

          Show
          githubbot ASF GitHub Bot added a comment - Github user godfreyhe commented on the issue: https://github.com/apache/flink/pull/3101 Done, thanks for the suggest @fhueske
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twalthr commented on the issue:

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

          Thanks for the update @godfreyhe.
          I will go over the code a last time and merge it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user twalthr commented on the issue: https://github.com/apache/flink/pull/3101 Thanks for the update @godfreyhe. I will go over the code a last time and merge it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3101
          Hide
          twalthr Timo Walther added a comment -

          Fixed in 1.3.0: 8efacf588cee45eb99a24628136ced308c4fb418

          Show
          twalthr Timo Walther added a comment - Fixed in 1.3.0: 8efacf588cee45eb99a24628136ced308c4fb418

            People

            • Assignee:
              godfreyhe godfrey he
              Reporter:
              godfreyhe godfrey he
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development