Skip to content

Conversation

@ChengXiangLi
Copy link

JavaBatchTranslator does not support no aggregation select after groupBy previously, add the logic in this PR.

@aljoscha
Copy link
Contributor

What it does now is to only pick one element in the group that has the compound key of the groupBy expression. The reason why I did not want to support this initially was that the element that results from this has arbitrary values in the fields that are not key fields since only on arbitrary element of the group of elements is picked.

Should we maybe limit this to only allow selecting fields that are also key fields?

@ChengXiangLi
Copy link
Author

@aljoscha , it's a very interesting topic, should we add a limitation that only allow groupBy key fields to be no aggregated function field in select clause?
Oracle has this limitation while Mysql does not. Most of the time, select non-aggregation field after groupby has no real meaning, while sometimes, if user is aware that groupby some fielde would lead to other fields get grouped as well, this feature would help to improve the performance and user convenience, more described here: http://dev.mysql.com/doc/refman/5.0/en/group-by-handling.html . Currently we keep consistent with Mysql on this feature, allow no aggregation field after groupBy, with no guarantee of its return value.
Actually, i prefer to the Mysql way, more flexible to user, what do you think?

@aljoscha
Copy link
Contributor

OK, I see your point. We can go with the MySQL way then. Thanks for explaining it. 😄

Could you please update the documentation with a small paragraph that explains the behavior?

@ChengXiangLi
Copy link
Author

Ok, i think it could be added in the Select operator description in Table API page, it would depends on FLINK-2955. Seems it takes quite long time delay to sync merged commit to github, waiting for FLINK-2955 get synced.

@tillrohrmann
Copy link
Contributor

@Li Chengxiang, you can always pull changes directly from the ASF git
repository: https://git-wip-us.apache.org/repos/asf/flink.git. Then you
don't have to wait for syncing with the github mirror.

On Thu, Nov 19, 2015 at 10:51 AM, Li Chengxiang notifications@github.com
wrote:

Ok, i think it could be added in the Select operator description in Table
API page, it would depends on FLINK-2955. Seems it takes quite long time
delay to sync merged commit to github, waiting for FLINK-2955 get synced.


Reply to this email directly or view it on GitHub
#1377 (comment).

@ChengXiangLi ChengXiangLi force-pushed the flink-2115 branch 2 times, most recently from 55d5df6 to bfd1b10 Compare November 20, 2015 06:31
@ChengXiangLi
Copy link
Author

Hi, @aljoscha , i guess you may forget this PR 😄 , could you help to move this forward? it should be ready yet.

@aljoscha
Copy link
Contributor

aljoscha commented Dec 2, 2015

Hi, you are are absolutely right, I'm sorry. Please don't hesitate to ping me in the future if I'm taking to long but I'll also try to be more responsive on these.

I'm merging it now. 😄

@aljoscha
Copy link
Contributor

aljoscha commented Dec 2, 2015

Thanks for your efforts. 😄 Could you please close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants