You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by sunjincheng121 <gi...@git.apache.org> on 2017/03/17 10:18:22 UTC
[GitHub] flink pull request #3560: [FLINK-6097][table] Guaranteed the order of the ex...
GitHub user sunjincheng121 opened a pull request:
https://github.com/apache/flink/pull/3560
[FLINK-6097][table] Guaranteed the order of the extracted field refer\u2026
\u2026ences as input order.
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-6097] Guaranteed the order of the extracted field references as input order.")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] 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 ] 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
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sunjincheng121/flink FLINK-6097-PR
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3560.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 #3560
----
commit b2717309c1f7845c996a6acdb6ee197dacc5244e
Author: \u91d1\u7af9 <ji...@alibaba-inc.com>
Date: 2017-03-17T10:11:29Z
[FLINK-6097][table] Guaranteed the order of the extracted field references as input order.
----
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the extracted...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3560
HI, @KurtYoung Thanks for your attention to this PR. Good question, Here I glad share why I notice this method\uff1a
When we try to implement OVER window TableAPI, The first version of the prototype to achieve\uff0cwe do not consider the table field will be out of order when we implement translateToPlan method\uff0cthen we set outputRow field from inputRow according to the Initial order of the table field index.
At the beginning, the projections in the select statement less than 5 columns It works well.But Unfortunately when the count of projections bigger than 4 (>=5), we got the random result. Then we debug the code, we find that ProjectionTranslator # identifyFieldReferences method uses theSet temporary save field, when the number of elements in the Set is less than 5, the Set takes the Se1, Se2, Se3, Se4 data structures. When the number of elements is greater than or equal to 5, the Set takes HashSet # HashTrieSet and which will cause the data to be out of order. So we thought 2 approach to solve this problem\uff1a
Let ProjectionTranslator # identifyFieldReferences method guaranteed the order of the extracted field references same as input order.
We add the input and output field mapping.
At last we using approach#2 solve the problem. This change is not necessary for the problem i have faced. But I feel it is better to let the output of this method in the same order as the input, it may be very helpful for other cases, though I am currently not aware of any. I am ok with not making this change, but we should add a comment instead to highlight that the potential output of the current output. Otherwise, some people may not pay attention to this and assume it is in order.
Thanks,
SunJincheng
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the extracted...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on the issue:
https://github.com/apache/flink/pull/3560
HI, @KurtYoung Thanks for your attention to this PR. Good question, the purpose of this change is just like this JIRA's description:
I want `ProjectionTranslator # identifyFieldReferences` to guaranteed the order of the extracted field references same as input order. using `Set` just want to eliminate duplicate field references, and `List` also can do this, it is no harm to keep the `ProjectionTranslator # identifyFieldReferences` method's output order consistent with input.
If the disorder results of `ProjectionTranslator # identifyFieldReferences` can work, then the orderly results must also work very well, because the order is also a situation of disorder.
What to you think?
Thanks,
SunJincheng
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the ex...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 closed the pull request at:
https://github.com/apache/flink/pull/3560
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the ex...
Posted by KurtYoung <gi...@git.apache.org>.
Github user KurtYoung commented on a diff in the pull request:
https://github.com/apache/flink/pull/3560#discussion_r106669352
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/plan/ProjectionTranslator.scala ---
@@ -227,18 +227,23 @@ object ProjectionTranslator {
* @param exprs a list of expressions to extract
* @return a list of field references extracted from the given expressions
*/
- def extractFieldReferences(exprs: Seq[Expression]): Seq[NamedExpression] = {
- exprs.foldLeft(Set[NamedExpression]()) {
+ def extractFieldReferences(exprs: Seq[Expression]): List[NamedExpression] = {
+ exprs.foldLeft(List[NamedExpression]()) {
(fieldReferences, expr) => identifyFieldReferences(expr, fieldReferences)
- }.toSeq
+ }
}
private def identifyFieldReferences(
expr: Expression,
- fieldReferences: Set[NamedExpression]): Set[NamedExpression] = expr match {
+ fieldReferences: List[NamedExpression]): List[NamedExpression] = expr match {
--- End diff --
I think using Set here gave us better intuition that we actually want to count uniq fields, any reason why you want to keep the order? Especially the order is not defined very well, it will change easily if we modify the code.
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the ex...
Posted by KurtYoung <gi...@git.apache.org>.
Github user KurtYoung commented on a diff in the pull request:
https://github.com/apache/flink/pull/3560#discussion_r106772134
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/plan/ProjectionTranslator.scala ---
@@ -227,18 +227,23 @@ object ProjectionTranslator {
* @param exprs a list of expressions to extract
* @return a list of field references extracted from the given expressions
*/
- def extractFieldReferences(exprs: Seq[Expression]): Seq[NamedExpression] = {
- exprs.foldLeft(Set[NamedExpression]()) {
+ def extractFieldReferences(exprs: Seq[Expression]): List[NamedExpression] = {
+ exprs.foldLeft(List[NamedExpression]()) {
(fieldReferences, expr) => identifyFieldReferences(expr, fieldReferences)
- }.toSeq
+ }
}
private def identifyFieldReferences(
expr: Expression,
- fieldReferences: Set[NamedExpression]): Set[NamedExpression] = expr match {
+ fieldReferences: List[NamedExpression]): List[NamedExpression] = expr match {
--- End diff --
The order is really depending on how we extract fields from all kinds of expressions. Like `BinaryExpression`, we first extract `left child`, and then `right child`. And for `Funtion Calls`, we extract the field from parameter with left to right order. More complex example will be `over`, imagine there is an aggregate on a partitioned window. Should the fields appeared in the aggregate or the field which partitioned on should be considered first?
So i think this kind of order is hard to define and hard to stay consistency, it will change easily when we modifying the codes. We should not rely anything on 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 #3560: [FLINK-6097][table] Guaranteed the order of the extracted...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3560
Hi @sunjincheng121, thanks for explaining the issue, but I did not completely understand the problem. The `ProjectionTranslator` is working on Table API expressions and `LogicalNodes` which are translated into Calcite `RelNodes` and `RexNodes`. The `RelBuilder` works with names not positions, so shouldn't field positions and layout be handled by Calcite?
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the extracted...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3560
Can you close this PR as well @sunjincheng121?
Thanks!
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the ex...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/3560#discussion_r106761581
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/plan/ProjectionTranslator.scala ---
@@ -227,18 +227,23 @@ object ProjectionTranslator {
* @param exprs a list of expressions to extract
* @return a list of field references extracted from the given expressions
*/
- def extractFieldReferences(exprs: Seq[Expression]): Seq[NamedExpression] = {
- exprs.foldLeft(Set[NamedExpression]()) {
+ def extractFieldReferences(exprs: Seq[Expression]): List[NamedExpression] = {
+ exprs.foldLeft(List[NamedExpression]()) {
(fieldReferences, expr) => identifyFieldReferences(expr, fieldReferences)
- }.toSeq
+ }
}
private def identifyFieldReferences(
expr: Expression,
- fieldReferences: Set[NamedExpression]): Set[NamedExpression] = expr match {
+ fieldReferences: List[NamedExpression]): List[NamedExpression] = expr match {
--- End diff --
Can you explain more about ` Especially the order is not defined very well, it will change easily if we modify the code.`?
---
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 #3560: [FLINK-6097][table] Guaranteed the order of the ex...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/3560#discussion_r107033984
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/plan/ProjectionTranslator.scala ---
@@ -227,18 +227,23 @@ object ProjectionTranslator {
* @param exprs a list of expressions to extract
* @return a list of field references extracted from the given expressions
*/
- def extractFieldReferences(exprs: Seq[Expression]): Seq[NamedExpression] = {
- exprs.foldLeft(Set[NamedExpression]()) {
+ def extractFieldReferences(exprs: Seq[Expression]): List[NamedExpression] = {
+ exprs.foldLeft(List[NamedExpression]()) {
(fieldReferences, expr) => identifyFieldReferences(expr, fieldReferences)
- }.toSeq
+ }
}
private def identifyFieldReferences(
expr: Expression,
- fieldReferences: Set[NamedExpression]): Set[NamedExpression] = expr match {
+ fieldReferences: List[NamedExpression]): List[NamedExpression] = expr match {
--- End diff --
We can also use a `LinkedHashSet` which preserves the order in which elements are inserted.
---
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.
---