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

Remove useless param rowRelDataType of DataStreamGroupAggregate.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0, 1.4.0
    • Component/s: Table API & SQL
    • Labels:
      None

      Description

      Changes as follows:
      1. Add !agg.getAggCallList.isEmpty condition for DataStreamGroupWindowAggregateRule and DataStreamGroupAggregateRule.
      2. Remove useless param rowRelDataType of DataStreamGroupAggregate.

        Issue Links

          Activity

          Hide
          fhueske Fabian Hueske added a comment -

          Fixed for 1.3.0 with 056d9553d713834d18c1b3490a6e3f106129a9ef
          Fixed for 1.4.0 with 64d3ce8dd15917b44b0a2329e6f82343edee3c92

          Show
          fhueske Fabian Hueske added a comment - Fixed for 1.3.0 with 056d9553d713834d18c1b3490a6e3f106129a9ef Fixed for 1.4.0 with 64d3ce8dd15917b44b0a2329e6f82343edee3c92
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user fhueske commented on the issue:

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

          +1 to merge

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3922 +1 to merge
          Hide
          sunjincheng121 sunjincheng added a comment -

          Hi, Fabian Hueske you are right. I had closed the FLINK-6428 and updated this PR. (for Remove useless param `rowRelDataType` of `DataStreamGroupAggregate`).

          Thank you,
          SunJincheng

          Show
          sunjincheng121 sunjincheng added a comment - Hi, Fabian Hueske you are right. I had closed the FLINK-6428 and updated this PR. (for Remove useless param `rowRelDataType` of `DataStreamGroupAggregate`). Thank you, SunJincheng
          Hide
          fhueske Fabian Hueske added a comment -

          I just checked and FLINK-6428 is already supported.
          During logical optimization it is translated into a GROUP BY without aggregation function.

          So by not changing the rules, we automatically support SELECT DISTINCT.

          Show
          fhueske Fabian Hueske added a comment - I just checked and FLINK-6428 is already supported. During logical optimization it is translated into a GROUP BY without aggregation function. So by not changing the rules, we automatically support SELECT DISTINCT .
          Hide
          sunjincheng121 sunjincheng added a comment -

          Do we need support in `DISTINCT` in `SELECT Clause`, e.g.: `SELECT DISTINCT name FROM table` ?

          Show
          sunjincheng121 sunjincheng added a comment - Do we need support in `DISTINCT` in `SELECT Clause`, e.g.: `SELECT DISTINCT name FROM table` ?
          Hide
          sunjincheng121 sunjincheng added a comment - - edited

          Yes, DISTINCT works by `group by ` + `join`. in DataSet. In this PR. we only do one thing:

          • Remove useless param rowRelDataType of DataStreamGroupAggregate.

          BTW:What do you think about FLINK-6428?
          Best,
          SunJincheng

          Show
          sunjincheng121 sunjincheng added a comment - - edited Yes, DISTINCT works by `group by ` + `join`. in DataSet. In this PR. we only do one thing: Remove useless param rowRelDataType of DataStreamGroupAggregate. BTW:What do you think about FLINK-6428 ? Best, SunJincheng
          Hide
          fhueske Fabian Hueske added a comment -

          I think this is valid SQL syntax that can be easily supported and don't see a reason why we should not allow this.
          It works the same way in any relational database, so I would not expect users to be surprised or confused.
          In fact, Calcite translates certain DISTINCT clauses into GROUP BY without aggregations (this is how it works in batch / DataSet).

          Show
          fhueske Fabian Hueske added a comment - I think this is valid SQL syntax that can be easily supported and don't see a reason why we should not allow this. It works the same way in any relational database, so I would not expect users to be surprised or confused. In fact, Calcite translates certain DISTINCT clauses into GROUP BY without aggregations (this is how it works in batch / DataSet).
          Hide
          sunjincheng121 sunjincheng added a comment -

          Hi, Fabian Hueske, I think:
          1. for `DataStreamGroupAggregate`, if we want get distinct keys we can using FLINK-6428(the PR. need updated tomorrow).
          2. for `DataStreamGroupWindowAggregate` we also need something like FLINK-6428 to deal with.
          I agree that the functionality can get distinct keys. but user will be a little confused. Because for SQL user, they only know `DISTINCT` keyword can get the get distinct fields(keys).
          What do you think?

          Best,
          SunJincheng

          Show
          sunjincheng121 sunjincheng added a comment - Hi, Fabian Hueske , I think: 1. for `DataStreamGroupAggregate`, if we want get distinct keys we can using FLINK-6428 (the PR. need updated tomorrow). 2. for `DataStreamGroupWindowAggregate` we also need something like FLINK-6428 to deal with. I agree that the functionality can get distinct keys. but user will be a little confused. Because for SQL user, they only know `DISTINCT` keyword can get the get distinct fields(keys). What do you think? Best, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user sunjincheng121 opened a pull request:

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

          FLINK-6598[table] Fix `DataStreamGroupAggregateRule` and `DataStrea…

          The changes of this PR. is modify the `matches` method of `DataStreamGroupAggregateRule` and `DataStreamGroupWindowAggregateRule`. Add the `!agg.getAggCallList.isEmpty` condition.

          • [x] General
          • The pull request references the related JIRA issue ("FLINK-6598[table] Fix `DataStreamGroupAggregateRule` and `DataStreamGroupWindowAggregateRule` matches error.")
          • 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] 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

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

          $ git pull https://github.com/sunjincheng121/flink FLINK-6598-PR

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

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


          commit c89882112b7d1b4041733d22ec7f0d459b08af9a
          Author: sunjincheng121 <sunjincheng121@gmail.com>
          Date: 2017-05-16T11:08:11Z

          FLINK-6598[table] Fix `DataStreamGroupAggregateRule` and `DataStreamGroupWindowAggregateRule` matches error.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user sunjincheng121 opened a pull request: https://github.com/apache/flink/pull/3922 FLINK-6598 [table] Fix `DataStreamGroupAggregateRule` and `DataStrea… The changes of this PR. is modify the `matches` method of `DataStreamGroupAggregateRule` and `DataStreamGroupWindowAggregateRule`. Add the `!agg.getAggCallList.isEmpty` condition. [x] General The pull request references the related JIRA issue (" FLINK-6598 [table] Fix `DataStreamGroupAggregateRule` and `DataStreamGroupWindowAggregateRule` matches error.") 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] 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 You can merge this pull request into a Git repository by running: $ git pull https://github.com/sunjincheng121/flink FLINK-6598 -PR Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3922.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 #3922 commit c89882112b7d1b4041733d22ec7f0d459b08af9a Author: sunjincheng121 <sunjincheng121@gmail.com> Date: 2017-05-16T11:08:11Z FLINK-6598 [table] Fix `DataStreamGroupAggregateRule` and `DataStreamGroupWindowAggregateRule` matches error.
          Hide
          fhueske Fabian Hueske added a comment -

          Why should agg.getAggCallList.isEmpty not be allowed? I think it can be executed and returns distinct keys. IMO, that's a useful operation.

          Show
          fhueske Fabian Hueske added a comment - Why should agg.getAggCallList.isEmpty not be allowed? I think it can be executed and returns distinct keys. IMO, that's a useful operation.

            People

            • Assignee:
              sunjincheng121 sunjincheng
              Reporter:
              sunjincheng121 sunjincheng
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development