refactor(optimizer): refactor logicalAgg::create #3533

st1page · 2022-06-29T02:35:29Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

add the LogicalProjectBuilder for dedup the expression and build a project
refactor and clean the LogicalAgg::create

Checklist

I have written necessary docs and comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

btw, it was for distinct agg rewriting, but now I think maybe we can do the distinct rewriting as a rule?

codecov · 2022-06-29T03:23:32Z

Codecov Report

Merging #3533 (36e5413) into main (9d99aa5) will increase coverage by 0.01%.
The diff coverage is 92.70%.

@@            Coverage Diff             @@
##             main    #3533      +/-   ##
==========================================
+ Coverage   74.29%   74.30%   +0.01%     
==========================================
  Files         773      773              
  Lines      109399   109438      +39     
==========================================
+ Hits        81273    81323      +50     
+ Misses      28126    28115      -11

Flag	Coverage Δ
rust	`74.30% <92.70%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/frontend/src/optimizer/plan_node/mod.rs	`97.91% <ø> (ø)`
...rontend/src/optimizer/plan_node/logical_project.rs	`98.07% <85.00%> (-0.90%)`	⬇️
...rc/frontend/src/optimizer/plan_node/logical_agg.rs	`92.88% <93.93%> (-0.05%)`	⬇️
src/frontend/src/utils/column_index_mapping.rs	`79.64% <100.00%> (+0.94%)`	⬆️
src/meta/src/manager/id.rs	`95.50% <0.00%> (-0.57%)`	⬇️
src/frontend/src/expr/utils.rs	`98.99% <0.00%> (-0.26%)`	⬇️
src/storage/src/hummock/local_version_manager.rs	`81.08% <0.00%> (+1.23%)`	⬆️
src/meta/src/hummock/mock_hummock_meta_client.rs	`41.50% <0.00%> (+5.66%)`	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

xiangjinwu · 2022-06-29T04:18:30Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+struct LogicalAggBuilder {
+    /// the builder of the input Project
+    input_proj_builder: LogicalProjectBuilder,
+    /// the column [0..input_group_key_num) in the project's output is the group key


This comment may not be true when there are duplicates in group_exprs, and can cause a panic:

with t(v1) as (values (1), (2), (1)) select v1 from t group by v1, v1;

More details in a prior discussion:
#1492 (comment)

(tbh I have not read the code yet. Just tested this case and got a panic. Maybe the root cause is another issue, as we should have added a regression test on duplicate group_exprs ...)

It is because the colprune can not handle the plan with same group key and I have fixed it. Also add some regression test.

…r_logical_agg_prepare_for_distinct

…or_distinct

likg227

LGTM

src/frontend/src/optimizer/plan_node/logical_project.rs

src/frontend/src/optimizer/plan_node/logical_agg.rs

likg227 · 2022-06-29T03:58:54Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+/// having clause.
+struct LogicalAggBuilder {
+    /// the builder of the input Project
+    input_proj_builder: LogicalProjectBuilder,


Suggested change

input_proj_builder: LogicalProjectBuilder,

proj_builder: LogicalProjectBuilder,

likg227 · 2022-06-29T03:59:11Z

src/frontend/src/optimizer/plan_node/logical_agg.rs

+    /// the builder of the input Project
+    input_proj_builder: LogicalProjectBuilder,
+    /// the column [0..input_group_key_num) in the project's output is the group key
+    input_group_key_num: usize,


Suggested change

input_group_key_num: usize,

group_key_num: usize,

Co-authored-by: Kaige Li <55606560+likg227@users.noreply.github.com>

…or_distinct

skyzh · 2022-07-02T09:55:28Z

@mergify requeue

mergify · 2022-07-02T09:55:54Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

…or_distinct

xiangjinwu · 2022-07-04T06:06:43Z

src/frontend/test_runner/tests/testdata/agg.yaml

+    LogicalProject { exprs: [$1, $2, $0, $3] }
+      LogicalAgg { group_keys: [0, 1, 1], agg_calls: [min($2), max($2)] }


Still incorrect. The proj exprs should be [$1, $3, $0, $4], or we need to dedup group_keys to [0, 1].

Repro:

create table t(v1 int, v2 int, v3 int) with ('appendonly' = false); insert into t values (1, 2, 3); select v2, min(v1) as min_v1, v3, max(v1) as max_v1 from t group by v3, v2, v2;

Actual incorrect output:

v2 | min_v1 | v3 | max_v1 ----+--------+----+-------- 2 | 2 | 3 | 1 (1 row)

Or even panic:

explain select v2, sum(v1) from t group by v3, v2, v2;

* add logical project builder for the expression dedup * refactor logicalAgg::create with logical builder * add clippy fix * add comments * add planner test for dup group key * fix when exist duplicate group key * fix prune col for logical agg with dup group key * conflict * clippy fix * Update src/frontend/src/optimizer/plan_node/logical_project.rs Co-authored-by: Kaige Li <55606560+likg227@users.noreply.github.com> Co-authored-by: Kaige Li <55606560+likg227@users.noreply.github.com> Co-authored-by: Alex Chi <iskyzh@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

st1page added 3 commits June 28, 2022 23:53

add logical project builder for the expression dedup

d3c088e

refactor logicalAgg::create with logical builder

f711ec8

add clippy fix

218cb3b

github-actions bot added the type/refactor label Jun 29, 2022

add comments

163a2c6

st1page requested a review from likg227 June 29, 2022 02:50

likg227 requested a review from xiangjinwu June 29, 2022 03:56

xiangjinwu reviewed Jun 29, 2022

View reviewed changes

add planner test for dup group key

c2e30d2

st1page changed the title ~~refactor(optimizer): refactor logicalAgg::create to prepare for distinct~~ refactor(optimizer): refactor logicalAgg::create Jun 29, 2022

st1page and others added 6 commits July 1, 2022 10:09

fix when exist duplicate group key

22b333b

fix prune col for logical agg with dup group key

68038df

Merge remote-tracking branch 'origin/main' into sts/optimizer_refacto…

05cee29

…r_logical_agg_prepare_for_distinct

conflict

7ff9bee

clippy fix

c580023

Merge branch 'main' into sts/optimizer_refactor_logical_agg_prepare_f…

ffefd96

…or_distinct

st1page requested a review from xiangjinwu July 1, 2022 07:10

likg227 approved these changes Jul 1, 2022

View reviewed changes

st1page and others added 2 commits July 1, 2022 15:52

Update src/frontend/src/optimizer/plan_node/logical_project.rs

bde541d

Co-authored-by: Kaige Li <55606560+likg227@users.noreply.github.com>

Merge branch 'main' into sts/optimizer_refactor_logical_agg_prepare_f…

a5c740e

…or_distinct

st1page added component/frontend Protocol, parsing, binder. mergify/can-merge Indicates that the PR can be added to the merge queue and removed mergify/can-merge Indicates that the PR can be added to the merge queue labels Jul 1, 2022

skyzh and others added 2 commits July 2, 2022 17:57

Merge branch 'main' into sts/optimizer_refactor_logical_agg_prepare_f…

c0ee7d1

…or_distinct

Merge branch 'main' into sts/optimizer_refactor_logical_agg_prepare_f…

36e5413

…or_distinct

mergify bot merged commit d187c37 into main Jul 2, 2022

mergify bot deleted the sts/optimizer_refactor_logical_agg_prepare_for_distinct branch July 2, 2022 10:22

xiangjinwu reviewed Jul 4, 2022

View reviewed changes

xiangjinwu mentioned this pull request Jul 4, 2022

fix(optimizer): LogicalAgg::prune_col with duplicate group_keys #3632

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(optimizer): refactor logicalAgg::create #3533

refactor(optimizer): refactor logicalAgg::create #3533

st1page commented Jun 29, 2022 •

edited

codecov bot commented Jun 29, 2022 •

edited

xiangjinwu Jun 29, 2022 •

edited

st1page Jul 1, 2022

likg227 left a comment

likg227 Jun 29, 2022

likg227 Jun 29, 2022

skyzh commented Jul 2, 2022

mergify bot commented Jul 2, 2022

xiangjinwu Jul 4, 2022 •

edited

	input_proj_builder: LogicalProjectBuilder,
	proj_builder: LogicalProjectBuilder,

		LogicalProject { exprs: [$1, $2, $0, $3] }
		LogicalAgg { group_keys: [0, 1, 1], agg_calls: [min($2), max($2)] }

refactor(optimizer): refactor logicalAgg::create #3533

refactor(optimizer): refactor logicalAgg::create #3533

Conversation

st1page commented Jun 29, 2022 • edited

What's changed and what's your intention?

Checklist

codecov bot commented Jun 29, 2022 • edited

Codecov Report

xiangjinwu Jun 29, 2022 • edited

Choose a reason for hiding this comment

st1page Jul 1, 2022

Choose a reason for hiding this comment

likg227 left a comment

Choose a reason for hiding this comment

likg227 Jun 29, 2022

Choose a reason for hiding this comment

likg227 Jun 29, 2022

Choose a reason for hiding this comment

skyzh commented Jul 2, 2022

mergify bot commented Jul 2, 2022

❌ This pull request head commit has not been previously disembarked from queue.

xiangjinwu Jul 4, 2022 • edited

Choose a reason for hiding this comment

st1page commented Jun 29, 2022 •

edited

codecov bot commented Jun 29, 2022 •

edited

xiangjinwu Jun 29, 2022 •

edited

xiangjinwu Jul 4, 2022 •

edited