Skip to content

Conversation

@cambyzju
Copy link
Contributor

@cambyzju cambyzju commented Dec 16, 2022

Proposed changes

Issue Number: close #15142

Problem summary

Describe your changes.

Checklist(Required)

  1. Does it affect the original behavior:
    • Yes
    • No
    • I don't know
  2. Has unit tests been added:
    • Yes
    • No
    • No Need
  3. Has document been added or modified:
    • Yes
    • No
    • No Need
  4. Does it need to update dependencies:
    • Yes
    • No
  5. Are there any changes that cannot be rolled back:
    • Yes (If Yes, please explain WHY)
    • No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@github-actions github-actions bot added the area/planner Issues or PRs related to the query planner label Dec 16, 2022
@hello-stephen
Copy link
Contributor

hello-stephen commented Dec 16, 2022

TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 34.92 seconds
load time: 634 seconds
storage size: 17123043839 Bytes
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20221221131027_clickbench_pr_66362.html

@morningman
Copy link
Contributor

please add regression test

@cambyzju cambyzju force-pushed the fix-1216 branch 4 times, most recently from 4041398 to 9715015 Compare December 21, 2022 06:21
@cambyzju
Copy link
Contributor Author

please add regression test

done

Comment on lines +995 to +996
// case1: having clause use column name table.v1, because v1 inside group by clause
// select id, sum(v1) v1 from table group by id,v1 having(v1>1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL's behavior is very strange. according to sql standard, having should filter the data after Aggregate. So the slot in having should always bind on the output of Aggregate, in other words, the alias name.
Since doris has many different from the behavior of MySQL. Here we should follow the SQL standard instead of MySQL for a more understandable syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MySQL's behavior is very strange. according to sql standard, having should filter the data after Aggregate. So the slot in having should always bind on the output of Aggregate, in other words, the alias name. Since doris has many different from the behavior of MySQL. Here we should follow the SQL standard instead of MySQL for a more understandable syntax.

  1. accord to SQL standard, having clause do not support alias name;
  2. Doris always say "compatible with MySQL protocol", lots of SQL keeps Mysql way;
  3. Before this pr, we already try to following MySQL way. But the old implement has two problem:
  • one is, only consider has GROUP BY or NOT, not consider slots inside GROUP BY or not;
  • the other is, try whole HAVING expr, do consider part slots have alias, part slots do not;

So if we want to follow SQL standard, we should better forbid alias name in HAVING clause.
But old users will confused, because the behavior changes too large.

Copy link
Member

@yangzhg yangzhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Jan 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

PR approved by anyone and no changes requested.

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

Labels

approved Indicates a PR has been approved by one committer. area/planner Issues or PRs related to the query planner dev/1.2.2-merged kind/test reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] having clause with same alias name do not work

5 participants