You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by wuchong <gi...@git.apache.org> on 2016/06/15 09:31:15 UTC
[GitHub] flink pull request #2102: [FLINK-4068] [tableAPI] Move constant computations...
GitHub user wuchong opened a pull request:
https://github.com/apache/flink/pull/2102
[FLINK-4068] [tableAPI] Move constant computations out of code-generated
The `ReduceExpressionsRule` rule can reduce constant expressions and replacing them with the corresponding constant. We have `ReduceExpressionsRule.CALC_INSTANCE` in both `DATASET_OPT_RULES` and `DATASET_OPT_RULES`, but it dose not take effect. Because it require the planner have an executor to evaluate the constant expressions. This PR does this, and resolve FLINK-4068.
And some tests added.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wuchong/flink FLINK-4068
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2102.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 #2102
----
commit 786653b7be085c1e80481137fed3d47c5da2357a
Author: Jark Wu <wu...@alibaba-inc.com>
Date: 2016-06-15T09:19:35Z
[FLINK-4068] [tableAPI] Move constant computations out of code-generated functions.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/2102
After introducing `RexExecutor` which make `ReduceExpressionRules` taking effect , many errors occurred.
1. The `cannot translate call AS($t0, $t1)` is a Calcite bug I think, and I created a related issue : [CALCITE-1295](https://issues.apache.org/jira/browse/CALCITE-1295).
2. We should replace [L69&L73](https://github.com/apache/flink/blob/master/flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/expressions/arithmetic.scala#L69-L73) to `relBuilder.call(SqlStdOperatorTable.CONCAT, l, cast)` otherwise will throw the following exception. Because calcite have no plus(String, String) method.
```
java.lang.RuntimeException: while resolving method 'plus[class java.lang.String, class java.lang.String]' in class class org.apache.calcite.runtime.SqlFunctions
at org.apache.calcite.linq4j.tree.Types.lookupMethod(Types.java:345)
at org.apache.calcite.linq4j.tree.Expressions.call(Expressions.java:442)
at org.apache.calcite.adapter.enumerable.RexImpTable$BinaryImplementor.implement(RexImpTable.java:1640)
at org.apache.calcite.adapter.enumerable.RexImpTable.implementCall(RexImpTable.java:854)
at org.apache.calcite.adapter.enumerable.RexImpTable.implementNullSemantics(RexImpTable.java:843)
at org.apache.calcite.adapter.enumerable.RexImpTable.implementNullSemantics0(RexImpTable.java:756)
at org.apache.calcite.adapter.enumerable.RexImpTable.access$900(RexImpTable.java:181)
at org.apache.calcite.adapter.enumerable.RexImpTable$3.implement(RexImpTable.java:411)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translateCall(RexToLixTranslator.java:535)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate0(RexToLixTranslator.java:507)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate(RexToLixTranslator.java:222)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate0(RexToLixTranslator.java:472)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate(RexToLixTranslator.java:222)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate(RexToLixTranslator.java:217)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translateList(RexToLixTranslator.java:700)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translateProjects(RexToLixTranslator.java:192)
at org.apache.calcite.rex.RexExecutorImpl.compile(RexExecutorImpl.java:80)
at org.apache.calcite.rex.RexExecutorImpl.compile(RexExecutorImpl.java:59)
at org.apache.calcite.rex.RexExecutorImpl.reduce(RexExecutorImpl.java:118)
at org.apache.calcite.rel.rules.ReduceExpressionsRule.reduceExpressionsInternal(ReduceExpressionsRule.java:544)
at org.apache.calcite.rel.rules.ReduceExpressionsRule.reduceExpressions(ReduceExpressionsRule.java:455)
at org.apache.calcite.rel.rules.ReduceExpressionsRule.reduceExpressions(ReduceExpressionsRule.java:438)
at org.apache.calcite.rel.rules.ReduceExpressionsRule$CalcReduceExpressionsRule.onMatch(ReduceExpressionsRule.java:350)
at org.apache.calcite.plan.volcano.VolcanoRuleCall.onMatch(VolcanoRuleCall.java:213)
at org.apache.calcite.plan.volcano.VolcanoPlanner.findBestExp(VolcanoPlanner.java:838)
at org.apache.calcite.tools.Programs$RuleSetProgram.run(Programs.java:334)
at org.apache.flink.api.table.BatchTableEnvironment.translate(BatchTableEnvironment.scala:250)
at org.apache.flink.api.java.table.BatchTableEnvironment.toDataSet(BatchTableEnvironment.scala:146)
at org.apache.flink.api.java.batch.table.ExpressionsITCase.testCom
````
3. The following error is when we convert `Trim` to `RexNode`, we use a Integer to represent "LEADING", "TRAILING", "BOTH". Instead we should use `SqlTrimFunction.Flag`. But I haven't found how to write SqlTrimFunction.Flag into a `RexNode`.
```
java.lang.ClassCastException: java.lang.Integer cannot be cast to org.apache.calcite.sql.fun.SqlTrimFunction$Flag
at org.apache.calcite.adapter.enumerable.RexImpTable$TrimImplementor.implement(RexImpTable.java:1448)
at org.apache.calcite.adapter.enumerable.RexImpTable.implementCall(RexImpTable.java:854)
at org.apache.calcite.adapter.enumerable.RexImpTable.implementNullSemantics(RexImpTable.java:843)
at org.apache.calcite.adapter.enumerable.RexImpTable.implementNullSemantics0(RexImpTable.java:756)
at org.apache.calcite.adapter.enumerable.RexImpTable.access$900(RexImpTable.java:181)
at org.apache.calcite.adapter.enumerable.RexImpTable$3.implement(RexImpTable.java:411)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translateCall(RexToLixTranslator.java:535)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate0(RexToLixTranslator.java:507)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate(RexToLixTranslator.java:222)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate0(RexToLixTranslator.java:472)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate(RexToLixTranslator.java:222)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translate(RexToLixTranslator.java:217)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translateList(RexToLixTranslator.java:700)
at org.apache.calcite.adapter.enumerable.RexToLixTranslator.translateProjects(RexToLixTranslator.java:192)
at org.apache.calcite.rex.RexExecutorImpl.compile(RexExecutorImpl.java:80)
at org.apache.calcite.rex.RexExecutorImpl.compile(RexExecutorImpl.java:59)
at org.apache.calcite.rex.RexExecutorImpl.reduce(RexExecutorImpl.java:118)
at org.apache.calcite.rel.rules.ReduceExpressionsRule.reduceExpressionsInternal(ReduceExpressionsRule.java:544)
at org.apache.calcite.rel.rules.ReduceExpressionsRule.reduceExpressions(ReduceExpressionsRule.java:455)
at org.apache.calcite.rel.rules.ReduceExpressionsRule.reduceExpressions(ReduceExpressionsRule.java:438)
at org.apache.calcite.rel.rules.ReduceExpressionsRule$CalcReduceExpressionsRule.onMatch(ReduceExpressionsRule.java:350)
at org.apache.calcite.plan.volcano.VolcanoRuleCall.onMatch(VolcanoRuleCall.java:213)
at org.apache.calcite.plan.volcano.VolcanoPlanner.findBestExp(VolcanoPlanner.java:838)
at org.apache.calcite.tools.Programs$RuleSetProgram.run(Programs.java:334)
at org.apache.flink.api.table.BatchTableEnvironment.translate(BatchTableEnvironment.scala:250)
at org.apache.flink.api.java.table.BatchTableEnvironment.toDataSet(BatchTableEnvironment.scala:146)
at org.apache.flink.api.java.batch.table.ExpressionsITCase.testComplexExpression(ExpressionsITCase.java:197)
```
4. And some other errors I didn't figure out , looks like calcite bugs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2102
@wuchong I have added symbols recently. So functions like `TRIM` are now called correctly. If you like you could update this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2102: [FLINK-4068] [tableAPI] Move constant computations...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/2102#discussion_r67276795
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/api/scala/batch/sql/SelectITCase.scala ---
@@ -146,4 +148,21 @@ class SelectITCase(
tEnv.sql(sqlQuery)
}
+ @Test
+ def testConstantReduce(): Unit = {
--- End diff --
\U0001f44d It's a good idea. I will try it later. And the CI throws `cannot translate call AS...` error, I will figure it out today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2102
This @wuchong, your approach looks good. I also found that the `ReduceExpressionRules` had no effect due to the missing `RexExecutor`.
However, it seems that several tests of `ExpressionITCase` are failing with this change. You can verify that the PR does not break the build by locally running `mvn clean install`.
In addition, the added test should be changed as sketched in the comment. Please let me know if you have questions.
Thanks, Fabian
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/2102
@twalthr That sounds great ! Thank you .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2102: [FLINK-4068] [tableAPI] Move constant computations...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2102#discussion_r67233943
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/api/scala/batch/sql/SelectITCase.scala ---
@@ -146,4 +148,21 @@ class SelectITCase(
tEnv.sql(sqlQuery)
}
+ @Test
+ def testConstantReduce(): Unit = {
--- End diff --
I think the assertion of this this test should not check the name of the generate Flink operator.
Instead I propose the following:
- split the `translate()` method into an `optimize()` method that generates the optimized `RelNode` tree and a `translate()` method that translates into a `DataSet` / `DataStream` program.
- make the `optimize()` method `private[flink]` and therefore accessible from a unit test
- add a `BatchTableEnvironmentTest` and a `StreamTableEnvironmentTest` which check that the optimized `RelNode` tree contains reduced expressions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2102: [FLINK-4068] [tableAPI] Move constant computations...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong closed the pull request at:
https://github.com/apache/flink/pull/2102
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2102
Thanks for the PR @wuchong! I will have a look at it soon :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/2102
Thanks @twalthr . This issue is more complex than I expected before. I will try to update this PR in next days.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2102
Regarding 3.: I already have some code in a branch that introduces symbols instead of the "Integer hack" we are currently using, but I haven't finished it so far. I can solve this issue for you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2102: [FLINK-4068] [tableAPI] Move constant computations out of...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/2102
As I mentioned above, the `cannot translate call AS($t0, $t1)` error because of a Calcite bug. The [CALCITE-1297](https://issues.apache.org/jira/browse/CALCITE-1297) fixed it , but not yet released. I will wait for Calcite 1.9 released and start working on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---