Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

executor: support GROUP_CONCAT(ORDER BY) #16591

Merged
merged 17 commits into from
May 6, 2020

Conversation

SunRunAway
Copy link
Contributor

@SunRunAway SunRunAway commented Apr 20, 2020

What problem does this PR solve?

Issue Number: close #6838

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

Executor part:

  1. Add a heap in GROUP_CONCAT function implementation.
  2. Build hash agg in unparalleled if group_concat has ORDER BY clause.

Planner part:

  1. Build and resolve arguments of ORDER BY clause.
  2. Add explain info with ORDER BY clause.
  3. Handle arguments of ORDER BY clause in ResolveIndices, PruneColumns and InjectProjBelowAgg
  4. Do not push down to coprocessor if group_concat has ORDER BY clause.
  5. Do not push down to Union and Join if group_concat has ORDER BY clause.

How it Works:

mysql> desc select 1, 2, 3, 4, 5 , group_concat(name, id ORDER BY 2 desc, name SEPARATOR '++') from test;
+------------------------------+---------+-----------+---------------+------------------------------------------------------------------------------------------------------------------------------------------------------+
| id                           | estRows | task      | access object | operator info                                                                                                                                        |
+------------------------------+---------+-----------+---------------+------------------------------------------------------------------------------------------------------------------------------------------------------+
| Projection_4                 | 1.00    | root      |               | 1->Column#5, 2->Column#6, 3->Column#7, 4->Column#8, 5->Column#9, Column#4                                                                            |
| └─StreamAgg_8                | 1.00    | root      |               | funcs:group_concat(Column#15, Column#16 order by Column#17 desc, Column#18 asc separator "++")->Column#4                                             |
|   └─Projection_18            | 6.00    | root      |               | cast(test.test.name, var_string(20))->Column#15, cast(test.test.id, var_string(20))->Column#16, test.test.name->Column#17, test.test.name->Column#18 |
|     └─TableReader_15         | 6.00    | root      |               | data:TableFullScan_14                                                                                                                                |
|       └─TableFullScan_14     | 6.00    | cop[tikv] | table:test    | keep order:false, stats:pseudo                                                                                                                       |
+------------------------------+---------+-----------+---------------+------------------------------------------------------------------------------------------------------------------------------------------------------+
5 rows in set (0.00 sec)

mysql> select 1, 2, 3, 4, 5 , group_concat(name, id ORDER BY 2 desc, name SEPARATOR '++') from test;
+---+---+---+---+---+-------------------------------------------------------------+
| 1 | 2 | 3 | 4 | 5 | group_concat(name, id ORDER BY 2 desc, name SEPARATOR '++') |
+---+---+---+---+---+-------------------------------------------------------------+
| 1 | 2 | 3 | 4 | 5 | 5003++2003++301++201++202++101                              |
+---+---+---+---+---+-------------------------------------------------------------+
1 row in set (0.00 sec)

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more MEM

Release note

  • support ORDER BY clause in GROUP_CONCAT

@SunRunAway SunRunAway requested review from a team as code owners April 20, 2020 04:01
@ghost ghost requested review from qw4990, francis0407 and a team and removed request for a team April 20, 2020 04:01
@SunRunAway SunRunAway changed the title expression: support GROUP_CONCAT(ORDER BY) executor: support GROUP_CONCAT(ORDER BY) Apr 20, 2020
@SunRunAway SunRunAway added the sig/execution SIG execution label Apr 20, 2020
@SunRunAway SunRunAway added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 20, 2020
@SunRunAway SunRunAway marked this pull request as draft April 20, 2020 04:44
@5kbpers
Copy link
Contributor

5kbpers commented Apr 20, 2020

/build-wasm

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@SunRunAway
Copy link
Contributor Author

/run-integration-copr-test
/run-unit-test

@SunRunAway SunRunAway marked this pull request as ready for review April 20, 2020 06:02
@SunRunAway
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #16591 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #16591   +/-   ##
===========================================
  Coverage   80.5162%   80.5162%           
===========================================
  Files           510        510           
  Lines        142216     142216           
===========================================
  Hits         114507     114507           
  Misses        18760      18760           
  Partials       8949       8949           

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@SunRunAway
Copy link
Contributor Author

/run-integration-copr-test

@SunRunAway SunRunAway force-pushed the issue6838GROUP_CONCAT_ORDER_BY branch from f15e615 to a29b92a Compare April 21, 2020 13:07
@SunRunAway SunRunAway force-pushed the issue6838GROUP_CONCAT_ORDER_BY branch from 5d00063 to 26fa2e5 Compare May 6, 2020 05:10
@SunRunAway
Copy link
Contributor Author

@zz-jason @winoros PTAL thanks.

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@SunRunAway SunRunAway added the status/can-merge Indicates a PR has been approved by a committer. label May 6, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

@SunRunAway merge failed.

@zz-jason zz-jason merged commit 7ebcc20 into pingcap:master May 6, 2020
@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/PTAL labels May 6, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 6, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-3.0 in PR #16988

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 6, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-3.1 in PR #16989

@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-4.0 in PR #16990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/execution SIG execution sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support GROUP_CONCAT(ORDER BY)
6 participants