planner, executor: refactor expand logic from analyzing grouping sets to level projections #7169

AilinKid · 2023-03-27T11:44:59Z

What problem does this PR solve?

Issue Number: close #6852

Problem Summary:

What is changed and how it works?

In previous PR #6545, we introduced Expand operator to analyze raw grouping-sets pb, and generating the implicit projection logic (basically expand and change the value in the block itself)

While for the better implementation of ROLL UP syntax, which means more dynamic generated columns and more complicated grouping sets definition, we choose to move the analyzing work to TiDB planner, and let the execution logic in tiflash side as clean and simple as possible.

In this PR, TiFlash should receive the level-projections pb structure from TiDB planner, and what it only need to concern is to just project it out (one level projection for outputting a individual block itself)

In the new implementation, we take expand as projection, so it will be also seen as a datasource node in query block division. Additionally, we introduced a ExpandBlockInputStream to control the block cache and current projected index position which is being done the same in ExpandTransformAction.

the new pb structure will be like below:

message ProjectExpr {
	repeated Expr exprs = 1;
}

message Expand2{
	repeated ProjectExpr proj_exprs = 1;         // for grouping sets generated projection levels, like [a, b, c, 1#gen_col1, 2#gen_col2]
	repeated string generated_output_names = 2;  // for output names for generated cols like grouping_id etc, like "gen_col1", "gen_col2" here
	optional Executor child = 3;
}

TiPB link: pingcap/tipb#300

Note: for now, since TiDB still relay on the old Expand logic (but not public to user yet, the default switch is off now), after we refactored the old logic in Multi Distinct Aggregate from expand to expand2, I will remove all the old logic completely without concern about the compatibility in the rolling update.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

planner, executor: refactor expand logic from analyzing grouping sets to level projections

ti-chi-bot · 2023-03-27T11:45:02Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

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

sre-bot · 2023-03-27T11:45:15Z

All committers have signed the CLA.

dbms/src/DataStreams/ExpandTransformAction.h

SeaRise · 2023-03-30T07:18:46Z

dbms/src/DataStreams/ExpandBlockInputStream.cpp

+Block ExpandBlockInputStream::getHeader() const
+{
+    Block res(names_and_types);
+    return res;
+}


ColumnPtr is required for the Block returned by getHeader.
ColumnPtr is not needed only for getSampleBlock.

like

tiflash/dbms/src/DataStreams/MockExchangeReceiverInputStream.h

Lines 31 to 33 in 1dc2d1a

Block getHeader() const override

{

return Block(columns_vector[0]).cloneEmpty();

addressed, 2f2f325

dbms/src/DataStreams/ExpandTransformAction.cpp

dbms/src/Interpreters/ExpressionActions.cpp

xzhangxian1008 · 2023-03-30T07:11:36Z

dbms/src/Interpreters/ExpressionActions.cpp

@@ -149,6 +149,14 @@ ExpressionAction ExpressionAction::expandSource(GroupingSets grouping_sets_)
    return a;
 }

+ExpressionAction ExpressionAction::addNullable(const std::string & col_name)


May be rename this as buildConvertToNullableAction

static ExpressionAction addColumn(const ColumnWithTypeAndName & added_column_); static ExpressionAction removeColumn(const std::string & removed_name); static ExpressionAction copyColumn(const std::string & from_name, const std::string & to_name); static ExpressionAction project(const NamesWithAliases & projected_columns_); static ExpressionAction project(const Names & projected_columns_); static ExpressionAction ordinaryJoin(std::shared_ptr<const Join> join_, const NamesAndTypesList & columns_added_by_join_); static ExpressionAction expandSource(GroupingSets grouping_sets); this one： static ExpressionAction convertToNullable(const std::string & col_name);

how about convertToNullable？maybe more regular here

static ExpressionAction addColumn(const ColumnWithTypeAndName & added_column_); static ExpressionAction removeColumn(const std::string & removed_name); static ExpressionAction copyColumn(const std::string & from_name, const std::string & to_name); static ExpressionAction project(const NamesWithAliases & projected_columns_); static ExpressionAction project(const Names & projected_columns_); static ExpressionAction ordinaryJoin(std::shared_ptr<const Join> join_, const NamesAndTypesList & columns_added_by_join_); static ExpressionAction expandSource(GroupingSets grouping_sets); this one： static ExpressionAction convertToNullable(const std::string & col_name);

how about convertToNullable？maybe more regular here

okk

dbms/src/Flash/Planner/Plans/PhysicalExpand2.cpp

dbms/src/Interpreters/ExpressionActions.h

dbms/src/Interpreters/Expand2.cpp

xzhangxian1008 · 2023-03-30T09:15:23Z

dbms/src/Interpreters/Expand2.cpp

+    MutableColumns res(num_columns);
+    for (size_t i = 0; i < num_columns; ++i)
+    {
+        res[i] = block_cache.getColumns()[i]->cloneResized(block_cache.rows());


For the columns that must be null, I think we should not clone their full content,

make sense.
we should dig out which col in this level is projected as null (varies from levels), then, do something elimination of copy work here. For now, multi_level_actions is seen as black box here, no too much analyzing work, just use level_action.execute(block) to get more simplicity

make sense. we should dig out which col in this level is projected as null (varies from levels), then, do something elimination of copy work here. For now, multi_level_actions is seen as black box here, no too much analyzing work, just use level_action.execute(block) to get more simplicity

projection action will add null attribute on some columns in L46. I think we can record which columns are null before and after executing. By comparing the difference we can know which columns are the columns that is projected to null and we can avoid the cloning on these columns. How about this method?

Please add todo for further performance optimization.

dbms/src/Interpreters/Expand2.cpp

dbms/src/Interpreters/tests/gtest_expand_projection.cpp

dbms/src/Interpreters/Expand2.cpp

xzhangxian1008 · 2023-04-27T01:32:52Z

dbms/src/Interpreters/Expand2.cpp

+        const std::string & name = one_alias.first;
+        const std::string & alias = one_alias.second;
+        ColumnWithTypeAndName column = cloned_block.getByName(name);
+        if (!alias.empty())
+            column.name = alias;
+        new_block.insert(std::move(column));


Maybe we can defer the operation of cloning the original columns' content. We could know which columns are needed at here, and unnecessary cloning mentioned at the previous comments could be avoided, I think.

now the case is:
clone a brand-new origin block, apply the projection actions on them, we don't take too much role into what the projection actions do in detail (and at last, pick the result columns and rename)

we have already got beforeExpandActions, whch is quite like prependProjectionActions to prune those unneeded columns as a whole, then doing each level projection individually.

suggestions indicates we may need a fine-granted copyFilter before each level projection to find what column-ref in this level projection real need (for example, literal null didn't need a real col ref in current level projection), then copy those necessary source columns.

good suggestion, a bit complicated, I will left it to next pull request as an enhancement.

xzhangxian1008 · 2023-05-10T02:43:37Z

Any update?

dbms/src/TestUtils/ProjectionTestUtil.h

dbms/src/TestUtils/ProjectionTestUtil.cpp

dbms/src/TestUtils/ProjectionTestUtil.h

dbms/src/Interpreters/ExpressionActions.cpp

dbms/src/Interpreters/Expand2.h

dbms/src/Interpreters/Expand2.cpp

dbms/src/Interpreters/Expand2.h

dbms/src/DataStreams/ExpandTransformAction.cpp

xzhangxian1008 · 2023-07-03T03:45:43Z

Any update?

guo-shaoge · 2023-07-03T11:16:24Z

dbms/src/DataStreams/ExpandBlockInputStream.cpp

+    , log(Logger::get(req_id))
+{
+    children.push_back(input);
+    i_th_project = 0;


init i_th_project in init list

Not exactly, when pipeline fetch a new block from child below Expand OP, we should cache this new mother block and rewind the current projection offset for the convenience of later level projections (each time output a new projected block in function tryOutput(...))

ti-chi-bot · 2023-07-06T05:55:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guo-shaoge, xzhangxian1008

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [guo-shaoge]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2023-07-06T05:55:32Z

[LGTM Timeline notifier]

Timeline:

2023-07-04 01:48:24.298382691 +0000 UTC m=+82136.232016110: ☑️ agreed by xzhangxian1008.
2023-07-06 05:55:30.341690612 +0000 UTC m=+1009.223166555: ☑️ agreed by guo-shaoge.

AilinKid · 2023-07-06T07:09:26Z

/merge

AilinKid · 2023-07-06T07:09:58Z

/run-all-tests

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

AilinKid · 2023-07-07T08:37:07Z

/run-all-tests

AilinKid · 2023-07-07T08:37:33Z

/rebuild

AilinKid · 2023-07-07T08:38:15Z

/run-all-tests

AilinKid · 2023-07-07T08:55:54Z

/merge

AilinKid force-pushed the local_expand2 branch from 98967e5 to faa5d54 Compare March 27, 2023 11:56

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

AilinKid force-pushed the local_expand2 branch from faa5d54 to 26375af Compare March 28, 2023 07:53

AilinKid mentioned this pull request Mar 28, 2023

Implement Underlying Grouping Sets pingcap/tidb#42631

Closed

SeaRise self-requested a review March 30, 2023 07:14

SeaRise reviewed Mar 30, 2023

View reviewed changes

xzhangxian1008 reviewed Mar 30, 2023

View reviewed changes

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

ti-chi-bot bot deleted a comment from ti-chi-bot Apr 24, 2023

AilinKid force-pushed the local_expand2 branch from 2f2f325 to 4b74cf8 Compare April 24, 2023 09:35

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

ti-chi-bot bot deleted a comment from ti-chi-bot Apr 24, 2023

xzhangxian1008 reviewed Apr 27, 2023

View reviewed changes

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

AilinKid force-pushed the local_expand2 branch from 4f361ef to 65aaf80 Compare June 13, 2023 08:01

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

This was referenced Jun 28, 2023

function: different grouping function will be resolved to a same one because expression action re-usage #7717

Closed

function: fix grouping function result name #7718

Merged

xzhangxian1008 reviewed Jun 30, 2023

View reviewed changes

dbms/src/DataStreams/ExpandTransformAction.cpp Show resolved Hide resolved

guo-shaoge reviewed Jul 3, 2023

View reviewed changes

ti-chi-bot bot added the approved label Jul 6, 2023

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Jul 6, 2023

AilinKid added 17 commits July 7, 2023 16:02

refactor expand logic from analyzing grouping sets to level projections

0be8367

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

make fmt

95b9fd1

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

fix header file linking problem

8b8e2fe

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

change the source columns to func call

1e8614b

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

fix clangd fmt

8e00561

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

build header from sub-stream, child-header

6bf2423

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

address comment

1528c0e

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

.

15dbaf2

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

fix the const col to be nullable

4817299

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

make fmt

27b9fc2

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

.

88c2f42

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

add const for func addNullableActionForColumnRef

8e22b66

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

address zhangxian's comment

58f8d21

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

address zhangxian's comment

f2c6e2e

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

add todo

a7d716d

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

git rebase fix

2c9e21d

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

fix test after rebase

6f08dd7

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

AilinKid force-pushed the local_expand2 branch from 94fd21e to 6f08dd7 Compare July 7, 2023 08:04

fix test

b602ec7

Signed-off-by: AilinKid <ailinsilence4@gmail.com>

ti-chi-bot bot merged commit bab9685 into pingcap:master Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner, executor: refactor expand logic from analyzing grouping sets to level projections #7169

planner, executor: refactor expand logic from analyzing grouping sets to level projections #7169

AilinKid commented Mar 27, 2023 •

edited

Loading

ti-chi-bot commented Mar 27, 2023

sre-bot commented Mar 27, 2023 •

edited

Loading

SeaRise Mar 30, 2023

SeaRise Mar 30, 2023

AilinKid Apr 24, 2023

xzhangxian1008 Mar 30, 2023

AilinKid Apr 24, 2023

xzhangxian1008 Apr 25, 2023

xzhangxian1008 Mar 30, 2023

AilinKid Apr 24, 2023 •

edited

Loading

xzhangxian1008 Jun 15, 2023 •

edited

Loading

xzhangxian1008 Jun 29, 2023

xzhangxian1008 Apr 27, 2023

AilinKid Jun 13, 2023

xzhangxian1008 commented May 10, 2023

xzhangxian1008 commented Jul 3, 2023

guo-shaoge Jul 3, 2023

AilinKid Jul 3, 2023 •

edited

Loading

ti-chi-bot bot commented Jul 6, 2023

ti-chi-bot bot commented Jul 6, 2023

AilinKid commented Jul 6, 2023

AilinKid commented Jul 6, 2023

AilinKid commented Jul 7, 2023

AilinKid commented Jul 7, 2023

AilinKid commented Jul 7, 2023

AilinKid commented Jul 7, 2023

	Block getHeader() const override
	{
	return Block(columns_vector[0]).cloneEmpty();

planner, executor: refactor expand logic from analyzing grouping sets to level projections #7169

planner, executor: refactor expand logic from analyzing grouping sets to level projections #7169

Conversation

AilinKid commented Mar 27, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 27, 2023

sre-bot commented Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

xzhangxian1008 Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhangxian1008 commented May 10, 2023

xzhangxian1008 commented Jul 3, 2023

Choose a reason for hiding this comment

AilinKid Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 6, 2023

ti-chi-bot bot commented Jul 6, 2023

[LGTM Timeline notifier]

AilinKid commented Jul 6, 2023

AilinKid commented Jul 6, 2023

AilinKid commented Jul 7, 2023

AilinKid commented Jul 7, 2023

AilinKid commented Jul 7, 2023

AilinKid commented Jul 7, 2023

AilinKid commented Mar 27, 2023 •

edited

Loading

sre-bot commented Mar 27, 2023 •

edited

Loading

AilinKid Apr 24, 2023 •

edited

Loading

xzhangxian1008 Jun 15, 2023 •

edited

Loading

AilinKid Jul 3, 2023 •

edited

Loading