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/09/28 02:33:40 UTC
[GitHub] flink pull request #2560: [FLINK-4068] [table] Move constant computations ou...
GitHub user wuchong opened a pull request:
https://github.com/apache/flink/pull/2560
[FLINK-4068] [table] Move constant computations out of code-generated
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [x ] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [x ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [x ] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
The origin PR is #2102. This PR makes `ReduceExpressionsRules` take effect. This rule can reduce constant expressions and replacing them with the corresponding constant.
And add some tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wuchong/flink reduce-expression-FLINK-4068
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2560.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 #2560
----
commit 22a0f2e949e524e20151c11cc83bd6e1e4f2a1b2
Author: Jark Wu <wu...@alibaba-inc.com>
Date: 2016-09-28T02:31:59Z
[FLINK-4068] [table] Move constant computations out of code-generated
----
---
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 #2560: [FLINK-4068] [table] Move constant computations out of co...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2560
Thanks for updating the PR @wuchong. I found a little bug, but I will fix it myself and merge this.
---
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 #2560: [FLINK-4068] [table] Move constant computations out of co...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2560
@wuchong it seems that the constant expression reduction does not work in the Table API . Could you convert your SQL tests also to additional Table tests and have a look at it? The SQL part seems to work fine.
---
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 #2560: [FLINK-4068] [table] Move constant computations ou...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2560#discussion_r80921875
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/FlinkRelBuilder.scala ---
@@ -38,7 +38,12 @@ class FlinkRelBuilder(
cluster,
relOptSchema) {
- def getPlanner: RelOptPlanner = cluster.getPlanner
+ def getPlanner: RelOptPlanner = {
+ val planner = cluster.getPlanner
+ // set the executor to evaluate constant expressions
+ planner.setExecutor(new RexExecutorImpl(Schemas.createDataContext(null)))
--- End diff --
Why don't you use `FrameworkConfig.executor()` in `TableEnvironment` to set the executor at the very beginning?
---
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 #2560: [FLINK-4068] [table] Move constant computations ou...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2560#discussion_r80919671
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/BatchTableEnvironment.scala ---
@@ -228,19 +229,13 @@ abstract class BatchTableEnvironment(
}
/**
- * Translates a [[Table]] into a [[DataSet]].
- *
- * The transformation involves optimizing the relational expression tree as defined by
+ * Generates the optimized [[RelNode]] tree from the relational expression tree as defined by
* Table API calls and / or SQL queries and generating corresponding [[DataSet]] operators.
*
* @param table The root node of the relational expression tree.
- * @param tpe The [[TypeInformation]] of the resulting [[DataSet]].
- * @tparam A The type of the resulting [[DataSet]].
- * @return The [[DataSet]] that corresponds to the translated [[Table]].
+ * @return The optimized [[RelNode]] tree
*/
- protected def translate[A](table: Table)(implicit tpe: TypeInformation[A]): DataSet[A] = {
-
- validateType(tpe)
+ private[flink] def optimize(table: Table): RelNode = {
--- End diff --
`optimize` from `Table` to `RelNode` looks strange. I think `optimize(relNode: RelNode): RelNode` would be better.
---
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 #2560: [FLINK-4068] [table] Move constant computations out of co...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/2560
Hi @twalthr , I have updated this PR , could you have a look again ?
---
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 #2560: [FLINK-4068] [table] Move constant computations out of co...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2560
I will shepherd 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 #2560: [FLINK-4068] [table] Move constant computations ou...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2560#discussion_r81163353
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/FlinkPlannerImpl.scala ---
@@ -60,6 +60,7 @@ class FlinkPlannerImpl(
var root: RelRoot = _
private def ready() {
+ planner.setExecutor(config.getExecutor)
--- End diff --
I would not set the planner here as this class is only used by the SQL API. `FlinkRelBuilder.create()` is a better place.
---
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 #2560: [FLINK-4068] [table] Move constant computations ou...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2560#discussion_r80927113
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/BatchTableEnvironment.scala ---
@@ -228,19 +229,13 @@ abstract class BatchTableEnvironment(
}
/**
- * Translates a [[Table]] into a [[DataSet]].
- *
- * The transformation involves optimizing the relational expression tree as defined by
+ * Generates the optimized [[RelNode]] tree from the relational expression tree as defined by
* Table API calls and / or SQL queries and generating corresponding [[DataSet]] operators.
--- End diff --
The method does not generate a `DataSet` operators. Can you also add that it converts to DataSet/DataStream convertion?
---
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 #2560: [FLINK-4068] [table] Move constant computations out of co...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/2560
@twalthr I add some Table API tests but skip some tests doesn't supported in Table API, such as `NULLIF`, `IN`, `EXTRACT`
---
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 #2560: [FLINK-4068] [table] Move constant computations out of co...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2560
@wuchong I heavily tested this PR again as it could introduce difficult bugs. And I also found one issue that needs to be discussed. If you have an expression like `CAST(TRUE AS VARCHAR) || 'Test'` the result will be `TTest` not `trueTest`. So the expression reduction has a different string representation of boolean literals. The question is, do we wanna adapt our code, can this be configured, or is Calcite doing something wrong? What do you think @fhueske?
---
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 #2560: [FLINK-4068] [table] Move constant computations ou...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/2560#discussion_r81270921
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/api/table/FlinkPlannerImpl.scala ---
@@ -60,6 +60,7 @@ class FlinkPlannerImpl(
var root: RelRoot = _
private def ready() {
+ planner.setExecutor(config.getExecutor)
--- End diff --
Yes you are right ! This is the reason why Table API not work.
---
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 #2560: [FLINK-4068] [table] Move constant computations ou...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2560
---
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 #2560: [FLINK-4068] [table] Move constant computations ou...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2560#discussion_r80925547
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/api/table/BatchTableEnvironmentTest.scala ---
@@ -0,0 +1,46 @@
+/*
+ * 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.api.table
+
+import org.apache.flink.api.scala.ExecutionEnvironment
+import org.apache.flink.api.scala.table._
+import org.apache.flink.api.scala.util.CollectionDataSets
+import org.junit.Assert._
+import org.junit.Test
+
+
+class BatchTableEnvironmentTest {
--- End diff --
Could you add some additional expressions for better testing? Boolean expressions, something with `NULL`, `CASE`, strings and date/time types.
---
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.
---