expression: Push down group concat to TiFlash by fzhedu · Pull Request #24778 · pingcap/tidb

fzhedu · 2021-05-20T04:10:01Z

What problem does this PR solve?

Issue Number: close pingcap/tiflash#1908

Problem Summary: push down group_concat() to TiFlash (it is not pushed down to TiKV also).

What is changed and how it works?

Proposal: implemention doc

What's Changed:

How it Works:

Related changes

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

Check List

Tests

Integration test

Release note

push down group_concat to TiFlash

ti-chi-bot · 2021-05-20T04:10:02Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hanfei1991
rebelice

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

sre-bot · 2021-05-20T04:10:32Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

*: what's changed

lzmhhh123

@fzhedu Please fix the ci and rebase the branch.

windtalker · 2021-08-04T03:14:47Z

expression/aggregation/agg_to_pb.go

I think since we already add orderBy field in pb expression, agg_to_pb can support OrderByItems for any aggregation functions. If an aggregation function with OrderByItems is not functionally supported by TiKV or TiFlash, it should be filtered out by function CheckAggPushDown

so we can not check it here?

Agreed with windtalker. This check is unnesessary.

windtalker · 2021-08-04T03:18:32Z

expression/aggregation/aggregation.go

maybe add a new function CheckAggPushTiKV?

yes, we can implement CheckAggPushTiKV after more cases, but now this case exists only for groupConcat, so I do not implement it.

maybe adding a comment here would reduce confusion

windtalker · 2021-08-04T03:19:58Z

planner/cascades/transformation_rules.go

This is a indepandent bug fix?

this is introduced due to the special case for groupConcat code

This is a indepandent bug fix?

planner/core/exhaust_physical_plans.go

planner/core/testdata/integration_serial_suite_in.json

fzhedu · 2021-08-11T02:41:17Z

/run-check_dev_2

fzhedu · 2021-08-12T07:23:04Z

/run-all-tests

… from outer join (pingcap#27067)

ichn-hu

LGTM

ti-chi-bot · 2021-08-30T06:28:26Z

@ichn-hu: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

In response to this:

LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

fzhedu · 2021-08-30T06:30:43Z

/merge

ti-chi-bot · 2021-08-30T06:30:45Z

@fzhedu: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

Details

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

rebelice · 2021-08-30T06:33:39Z

/merge

ti-chi-bot · 2021-08-30T06:33:41Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 828dc24

fzhedu requested a review from lzmhhh123 May 20, 2021 04:10

fzhedu requested review from a team as code owners May 20, 2021 04:10

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 20, 2021

fzhedu requested review from hanfei1991 and windtalker May 20, 2021 04:10

fzhedu changed the title ~~Push down group concat to TiFlash~~ expression: Push down group concat to TiFlash May 20, 2021

fzhedu mentioned this pull request May 20, 2021

AddOrderByToExpr pingcap/tipb#222

Merged

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2021

wshwsh12 removed the request for review from a team June 18, 2021 08:33

lzmhhh123 reviewed Jun 21, 2021

View reviewed changes

fzhedu force-pushed the PushDownGroupConcat branch from 0558148 to 1bead62 Compare August 4, 2021 02:45

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2021

windtalker reviewed Aug 4, 2021

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2021

fzhedu force-pushed the PushDownGroupConcat branch from 273bf0f to d6a71d9 Compare August 9, 2021 09:29

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2021

fzhedu commented Aug 10, 2021

View reviewed changes

planner/core/testdata/integration_serial_suite_in.json Outdated Show resolved Hide resolved

ti-chi-bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 10, 2021

fzhedu requested review from lzmhhh123 and windtalker August 10, 2021 13:00

fzhedu force-pushed the PushDownGroupConcat branch from f06cef6 to 789ea66 Compare August 11, 2021 01:54

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 22, 2021

fzhedu and others added 10 commits August 30, 2021 12:31

update tests

9d9d94c

expression: do not derive filters containing null sensitive functions…

74a12ce

… from outer join (pingcap#27067)

rewrite plan write and add more tests

2eb6903

update scalar agg

947e5a3

add more tests

602ab40

address comments

5587c7e

address comments

a68e771

address comments

f1c781b

address comments

e3a9103

format code

828dc24

fzhedu force-pushed the PushDownGroupConcat branch from 103f918 to 828dc24 Compare August 30, 2021 04:31

hanfei1991 approved these changes Aug 30, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 30, 2021

ichn-hu approved these changes Aug 30, 2021

View reviewed changes

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 30, 2021

rebelice approved these changes Aug 30, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 30, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 30, 2021

ti-chi-bot added 4 commits August 30, 2021 14:33

Merge branch 'master' into PushDownGroupConcat

5154d54

Merge branch 'master' into PushDownGroupConcat

ebd1e2c

Merge branch 'master' into PushDownGroupConcat

59c640a

Merge branch 'master' into PushDownGroupConcat

f99825a

ti-chi-bot merged commit f50dd1f into pingcap:master Aug 30, 2021

Conversation

fzhedu commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented May 20, 2021

Uh oh!

lzmhhh123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fzhedu commented Aug 11, 2021

Uh oh!

fzhedu commented Aug 12, 2021

Uh oh!

ichn-hu left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Aug 30, 2021

Uh oh!

fzhedu commented Aug 30, 2021

Uh oh!

ti-chi-bot commented Aug 30, 2021

Uh oh!

rebelice commented Aug 30, 2021

Uh oh!

ti-chi-bot commented Aug 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

fzhedu commented May 20, 2021 •

edited

Loading

ti-chi-bot commented May 20, 2021 •

edited

Loading